diff --git a/src/ast/ast-value-factory.cc b/src/ast/ast-value-factory.cc index 26d688d5ea..a2b3ae03ef 100644 --- a/src/ast/ast-value-factory.cc +++ b/src/ast/ast-value-factory.cc @@ -34,6 +34,7 @@ #include "src/heap/local-factory-inl.h" #include "src/objects/objects-inl.h" #include "src/objects/objects.h" +#include "src/objects/string.h" #include "src/strings/char-predicates-inl.h" #include "src/strings/string-hasher.h" #include "src/utils/utils-inl.h" @@ -231,14 +232,17 @@ Handle AstConsString::AllocateFlat(LocalIsolate* isolate) const { ->NewRawOneByteString(result_length, AllocationType::kOld) .ToHandleChecked(); DisallowHeapAllocation no_gc; - uint8_t* dest = result->GetChars(no_gc) + result_length; + uint8_t* dest = + result->GetChars(no_gc, SharedStringAccessGuardIfNeeded::NotNeeded()) + + result_length; for (const AstConsString::Segment* current = &segment_; current != nullptr; current = current->next) { int length = current->string->length(); dest -= length; CopyChars(dest, current->string->raw_data(), length); } - DCHECK_EQ(dest, result->GetChars(no_gc)); + DCHECK_EQ(dest, result->GetChars( + no_gc, SharedStringAccessGuardIfNeeded::NotNeeded())); return result; } @@ -247,7 +251,9 @@ Handle AstConsString::AllocateFlat(LocalIsolate* isolate) const { ->NewRawTwoByteString(result_length, AllocationType::kOld) .ToHandleChecked(); DisallowHeapAllocation no_gc; - uint16_t* dest = result->GetChars(no_gc) + result_length; + uint16_t* dest = + result->GetChars(no_gc, SharedStringAccessGuardIfNeeded::NotNeeded()) + + result_length; for (const AstConsString::Segment* current = &segment_; current != nullptr; current = current->next) { int length = current->string->length(); @@ -260,7 +266,8 @@ Handle AstConsString::AllocateFlat(LocalIsolate* isolate) const { length); } } - DCHECK_EQ(dest, result->GetChars(no_gc)); + DCHECK_EQ(dest, result->GetChars( + no_gc, SharedStringAccessGuardIfNeeded::NotNeeded())); return result; } template EXPORT_TEMPLATE_DEFINE(V8_EXPORT_PRIVATE) diff --git a/src/heap/factory-base.cc b/src/heap/factory-base.cc index 432a5ac28d..439bf4ae57 100644 --- a/src/heap/factory-base.cc +++ b/src/heap/factory-base.cc @@ -21,6 +21,7 @@ #include "src/objects/shared-function-info-inl.h" #include "src/objects/source-text-module.h" #include "src/objects/string-inl.h" +#include "src/objects/string.h" #include "src/objects/template-objects-inl.h" namespace v8 { @@ -508,7 +509,8 @@ Handle FactoryBase::NewOneByteInternalizedString( Handle result = AllocateRawOneByteInternalizedString(str.length(), raw_hash_field); DisallowHeapAllocation no_gc; - MemCopy(result->GetChars(no_gc), str.begin(), str.length()); + MemCopy(result->GetChars(no_gc, SharedStringAccessGuardIfNeeded::NotNeeded()), + str.begin(), str.length()); return result; } @@ -518,7 +520,8 @@ Handle FactoryBase::NewTwoByteInternalizedString( Handle result = AllocateRawTwoByteInternalizedString(str.length(), raw_hash_field); DisallowHeapAllocation no_gc; - MemCopy(result->GetChars(no_gc), str.begin(), str.length() * kUC16Size); + MemCopy(result->GetChars(no_gc, SharedStringAccessGuardIfNeeded::NotNeeded()), + str.begin(), str.length() * kUC16Size); return result; } @@ -606,21 +609,31 @@ MaybeHandle FactoryBase::NewConsString( Handle result = NewRawOneByteString(length, allocation).ToHandleChecked(); DisallowHeapAllocation no_gc; - uint8_t* dest = result->GetChars(no_gc); + uint8_t* dest = + result->GetChars(no_gc, SharedStringAccessGuardIfNeeded::NotNeeded()); // Copy left part. - const uint8_t* src = left->template GetChars(no_gc); - CopyChars(dest, src, left_length); + { + SharedStringAccessGuardIfNeeded access_guard(*left); + const uint8_t* src = + left->template GetChars(no_gc, access_guard); + CopyChars(dest, src, left_length); + } // Copy right part. - src = right->template GetChars(no_gc); - CopyChars(dest + left_length, src, right_length); + { + SharedStringAccessGuardIfNeeded access_guard(*right); + const uint8_t* src = + right->template GetChars(no_gc, access_guard); + CopyChars(dest + left_length, src, right_length); + } return result; } Handle result = NewRawTwoByteString(length, allocation).ToHandleChecked(); - DisallowHeapAllocation pointer_stays_valid; - uc16* sink = result->GetChars(pointer_stays_valid); + DisallowHeapAllocation no_gc; + uc16* sink = + result->GetChars(no_gc, SharedStringAccessGuardIfNeeded::NotNeeded()); String::WriteToFlat(*left, sink, 0, left->length()); String::WriteToFlat(*right, sink + left->length(), 0, right->length()); return result; diff --git a/src/objects/string-inl.h b/src/objects/string-inl.h index ca779174e4..cc5c120043 100644 --- a/src/objects/string-inl.h +++ b/src/objects/string-inl.h @@ -32,15 +32,34 @@ namespace internal { class SharedStringAccessGuardIfNeeded { public: explicit SharedStringAccessGuardIfNeeded(String str) { - // If we can get the isolate from the String, it means it is not a read only - // string. Isolate* isolate; - if (GetIsolateFromHeapObject(str, &isolate) && - ThreadId::Current() != isolate->thread_id()) - mutex_guard.emplace(isolate->string_access()); + if (IsNeeded(str, &isolate)) mutex_guard.emplace(isolate->string_access()); + } + + static SharedStringAccessGuardIfNeeded NotNeeded() { + return SharedStringAccessGuardIfNeeded(); + } + + static bool IsNeeded(String str, Isolate** out_isolate = nullptr) { + Isolate* isolate; + if (!GetIsolateFromHeapObject(str, &isolate)) { + // If we can't get the isolate from the String, it must be read-only. + DCHECK(ReadOnlyHeap::Contains(str)); + return false; + } + if (out_isolate) *out_isolate = isolate; + return ThreadId::Current() != isolate->thread_id(); } private: + // Default constructor and move constructor required for the NotNeeded() + // static constructor. + constexpr SharedStringAccessGuardIfNeeded() = default; + constexpr SharedStringAccessGuardIfNeeded(SharedStringAccessGuardIfNeeded&&) + V8_NOEXCEPT { + DCHECK(!mutex_guard.has_value()); + } + base::Optional> mutex_guard; }; @@ -282,12 +301,13 @@ class SequentialStringKey final : public StringTableKey { convert_(convert) {} bool IsMatch(String s) override { + SharedStringAccessGuardIfNeeded access_guard(s); DisallowHeapAllocation no_gc; if (s.IsOneByteRepresentation()) { - const uint8_t* chars = s.GetChars(no_gc); + const uint8_t* chars = s.GetChars(no_gc, access_guard); return CompareChars(chars, chars_.begin(), chars_.length()) == 0; } - const uint16_t* chars = s.GetChars(no_gc); + const uint16_t* chars = s.GetChars(no_gc, access_guard); return CompareChars(chars, chars_.begin(), chars_.length()) == 0; } @@ -413,6 +433,16 @@ const Char* String::GetChars(const DisallowHeapAllocation& no_gc) { : CharTraits::String::cast(*this).GetChars(no_gc); } +template +const Char* String::GetChars( + const DisallowHeapAllocation& no_gc, + const SharedStringAccessGuardIfNeeded& access_guard) { + return StringShape(*this).IsExternal() + ? CharTraits::ExternalString::cast(*this).GetChars() + : CharTraits::String::cast(*this).GetChars(no_gc, + access_guard); +} + Handle String::Flatten(Isolate* isolate, Handle string, AllocationType allocation) { if (string->IsConsString()) { @@ -582,6 +612,15 @@ Address SeqOneByteString::GetCharsAddress() { uint8_t* SeqOneByteString::GetChars(const DisallowHeapAllocation& no_gc) { USE(no_gc); + DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(*this)); + return reinterpret_cast(GetCharsAddress()); +} + +uint8_t* SeqOneByteString::GetChars( + const DisallowHeapAllocation& no_gc, + const SharedStringAccessGuardIfNeeded& access_guard) { + USE(no_gc); + USE(access_guard); return reinterpret_cast(GetCharsAddress()); } @@ -591,6 +630,15 @@ Address SeqTwoByteString::GetCharsAddress() { uc16* SeqTwoByteString::GetChars(const DisallowHeapAllocation& no_gc) { USE(no_gc); + DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(*this)); + return reinterpret_cast(GetCharsAddress()); +} + +uc16* SeqTwoByteString::GetChars( + const DisallowHeapAllocation& no_gc, + const SharedStringAccessGuardIfNeeded& access_guard) { + USE(no_gc); + USE(access_guard); return reinterpret_cast(GetCharsAddress()); } diff --git a/src/objects/string-table.cc b/src/objects/string-table.cc index 9413d53f74..9c129e0d78 100644 --- a/src/objects/string-table.cc +++ b/src/objects/string-table.cc @@ -358,7 +358,10 @@ class InternalizedStringKey final : public StringTableKey { set_raw_hash_field(string->raw_hash_field()); } - bool IsMatch(String string) override { return string_->SlowEquals(string); } + bool IsMatch(String string) override { + DCHECK(!SharedStringAccessGuardIfNeeded::IsNeeded(string)); + return string_->SlowEquals(string); + } Handle AsHandle(Isolate* isolate) { // Internalize the string if possible. diff --git a/src/objects/string.cc b/src/objects/string.cc index 405f0911c0..445f3cc4ca 100644 --- a/src/objects/string.cc +++ b/src/objects/string.cc @@ -640,11 +640,9 @@ std::unique_ptr String::ToCString(AllowNullsFlag allow_nulls, } template -void String::WriteToFlat(String src, sinkchar* sink, int f, int t) { +void String::WriteToFlat(String source, sinkchar* sink, int from, int to) { DisallowHeapAllocation no_gc; - String source = src; - int from = f; - int to = t; + SharedStringAccessGuardIfNeeded access_guard(source); while (from < to) { DCHECK_LE(0, from); DCHECK_LE(to, source.length()); @@ -660,13 +658,17 @@ void String::WriteToFlat(String src, sinkchar* sink, int f, int t) { return; } case kOneByteStringTag | kSeqStringTag: { - CopyChars(sink, SeqOneByteString::cast(source).GetChars(no_gc) + from, - to - from); + CopyChars( + sink, + SeqOneByteString::cast(source).GetChars(no_gc, access_guard) + from, + to - from); return; } case kTwoByteStringTag | kSeqStringTag: { - CopyChars(sink, SeqTwoByteString::cast(source).GetChars(no_gc) + from, - to - from); + CopyChars( + sink, + SeqTwoByteString::cast(source).GetChars(no_gc, access_guard) + from, + to - from); return; } case kOneByteStringTag | kConsStringTag: @@ -699,9 +701,10 @@ void String::WriteToFlat(String src, sinkchar* sink, int f, int t) { if (to - boundary == 1) { sink[boundary - from] = static_cast(second.Get(0)); } else if (second.IsSeqOneByteString()) { - CopyChars(sink + boundary - from, - SeqOneByteString::cast(second).GetChars(no_gc), - to - boundary); + CopyChars( + sink + boundary - from, + SeqOneByteString::cast(second).GetChars(no_gc, access_guard), + to - boundary); } else { WriteToFlat(second, sink + boundary - from, 0, to - boundary); } diff --git a/src/objects/string.h b/src/objects/string.h index 8d211bae5a..dc4381b39d 100644 --- a/src/objects/string.h +++ b/src/objects/string.h @@ -21,6 +21,8 @@ namespace v8 { namespace internal { +class SharedStringAccessGuardIfNeeded; + enum InstanceType : uint16_t; enum AllowNullsFlag { ALLOW_NULLS, DISALLOW_NULLS }; @@ -174,10 +176,18 @@ class String : public TorqueGeneratedString { V8_INLINE Vector GetCharVector( const DisallowHeapAllocation& no_gc); - // Get chars from sequential or external strings. + // Get chars from sequential or external strings. May only be called when a + // SharedStringAccessGuard is not needed (i.e. on the main thread or on + // read-only strings). template inline const Char* GetChars(const DisallowHeapAllocation& no_gc); + // Get chars from sequential or external strings. + template + inline const Char* GetChars( + const DisallowHeapAllocation& no_gc, + const SharedStringAccessGuardIfNeeded& access_guard); + // Returns the address of the character at an offset into this string. // Requires: this->IsFlat() const byte* AddressOfCharacterAt(int start_index, @@ -575,8 +585,15 @@ class SeqOneByteString // Get the address of the characters in this string. inline Address GetCharsAddress(); + // Get a pointer to the characters of the string. May only be called when a + // SharedStringAccessGuard is not needed (i.e. on the main thread or on + // read-only strings). inline uint8_t* GetChars(const DisallowHeapAllocation& no_gc); + // Get a pointer to the characters of the string. + inline uint8_t* GetChars(const DisallowHeapAllocation& no_gc, + const SharedStringAccessGuardIfNeeded& access_guard); + // Clear uninitialized padding space. This ensures that the snapshot content // is deterministic. void clear_padding(); @@ -613,8 +630,15 @@ class SeqTwoByteString // Get the address of the characters in this string. inline Address GetCharsAddress(); + // Get a pointer to the characters of the string. May only be called when a + // SharedStringAccessGuard is not needed (i.e. on the main thread or on + // read-only strings). inline uc16* GetChars(const DisallowHeapAllocation& no_gc); + // Get a pointer to the characters of the string. + inline uc16* GetChars(const DisallowHeapAllocation& no_gc, + const SharedStringAccessGuardIfNeeded& access_guard); + // Clear uninitialized padding space. This ensures that the snapshot content // is deterministic. void clear_padding(); diff --git a/test/inspector/isolate-data.h b/test/inspector/isolate-data.h index a3f14b5a29..6cab3de108 100644 --- a/test/inspector/isolate-data.h +++ b/test/inspector/isolate-data.h @@ -38,6 +38,13 @@ class IsolateData : public v8_inspector::V8InspectorClient { v8::StartupData* startup_data, WithInspector with_inspector); static IsolateData* FromContext(v8::Local context); + ~IsolateData() override { + // Enter the isolate before destructing this IsolateData, so that + // destructors that run before the Isolate's destructor still see it as + // entered. + isolate()->Enter(); + } + v8::Isolate* isolate() const { return isolate_.get(); } TaskRunner* task_runner() const { return task_runner_; } @@ -128,7 +135,11 @@ class IsolateData : public v8_inspector::V8InspectorClient { // call {Dispose}. We have to use the unique_ptr so that the isolate get // disposed in the right order, relative to other member variables. struct IsolateDeleter { - void operator()(v8::Isolate* isolate) const { isolate->Dispose(); } + void operator()(v8::Isolate* isolate) const { + // Exit the isolate after it was entered by ~IsolateData. + isolate->Exit(); + isolate->Dispose(); + } }; TaskRunner* task_runner_;