Replace ScriptValue's value with Member<WorldSafeV8ReferenceWrapper>

Since we can't trace WorldSafeV8Reference on stack,
we use Member<WorldSafeV8ReferenceWrapper>.
WorldSafeV8ReferenceWrapper class wraps WorldSafeV8Reference.

Note that we remove ScriptValue::ToWorldSafeV8Reference() for now,
but revive this once we ensure that TraceWrapperV8Reference's copy assignment
is not buggy.

Bug: 1007881, 1008258, 1008215, 1007881, 1007868
Bug: 1007865, 1007341
Change-Id: Ief22c6138394a512df9454d3c5bfad0bf35f8425
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1828783
Commit-Queue: Rika Fujimaki <rikaf@google.com>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Reviewed-by: Keishi Hattori <keishi@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#701030}

third_party/blink/renderer/bindings/core/v8/script_value.cc

@@ -41,7 +41,7 @@ v8::Local<v8::Value> ScriptValue::V8Value() const {
     return v8::Local<v8::Value>();
 
   DCHECK(GetIsolate()->InContext());
-  return value_.Get(ScriptState::From(isolate_->GetCurrentContext()));
+  return value_->Get(ScriptState::From(isolate_->GetCurrentContext()));
 }
 
 v8::Local<v8::Value> ScriptValue::V8ValueFor(
@@ -49,7 +49,7 @@ v8::Local<v8::Value> ScriptValue::V8ValueFor(
   if (IsEmpty())
     return v8::Local<v8::Value>();
 
-  return value_.GetAcrossWorld(target_script_state);
+  return value_->GetAcrossWorld(target_script_state);
 }
 
 bool ScriptValue::ToString(String& result) const {

third_party/blink/renderer/bindings/core/v8/script_value.h

@@ -73,12 +73,12 @@ class CORE_EXPORT ScriptValue final {
 
   // TODO(rikaf): Forbid passing empty v8::Local<v8::Value> to ScriptValue's
   // ctor.
-
   ScriptValue(v8::Isolate* isolate, v8::Local<v8::Value> value)
       : isolate_(isolate),
         value_(value.IsEmpty()
-                   ? WorldSafeV8Reference<v8::Value>()
-                   : WorldSafeV8Reference<v8::Value>(isolate, value)) {
+                   ? MakeGarbageCollected<WorldSafeV8ReferenceWrapper>()
+                   : MakeGarbageCollected<WorldSafeV8ReferenceWrapper>(isolate,
+                                                                       value)) {
     DCHECK(isolate_);
   }
 
@@ -86,30 +86,17 @@ class CORE_EXPORT ScriptValue final {
   ScriptValue(v8::Isolate* isolate, v8::MaybeLocal<T> value)
       : isolate_(isolate),
         value_(value.IsEmpty()
-                   ? WorldSafeV8Reference<v8::Value>()
-                   : WorldSafeV8Reference<v8::Value>(isolate,
-                                                     value.ToLocalChecked())) {
-    DCHECK(isolate_);
-  }
-
-  ScriptValue(v8::Isolate* isolate, WorldSafeV8Reference<v8::Value> value)
-      : isolate_(isolate), value_(value) {
+                   ? MakeGarbageCollected<WorldSafeV8ReferenceWrapper>()
+                   : MakeGarbageCollected<WorldSafeV8ReferenceWrapper>(
+                         isolate,
+                         value.ToLocalChecked())) {
     DCHECK(isolate_);
   }
 
-  ScriptValue(const ScriptValue& value) {
-    // TODO(crbug.com/v8/9773): Deal with null value at the side of v8.
-    if (value.IsEmpty()) {
-      isolate_ = nullptr;
-      value_.Reset();
-    } else {
-      isolate_ = value.isolate_;
-      value_ = value.value_;
-      DCHECK(isolate_);
-    }
-  }
+  ScriptValue(const ScriptValue& value) = default;
 
-  // Use this GetIsolate() to do DCHECK inside ScriptValue.
+  // TODO(riakf): Use this GetIsolate() only when doing DCHECK inside
+  // ScriptValue.
   v8::Isolate* GetIsolate() const {
     DCHECK(isolate_);
     return isolate_;
@@ -118,7 +105,11 @@ class CORE_EXPORT ScriptValue final {
   ScriptValue& operator=(const ScriptValue& value) = default;
 
   bool operator==(const ScriptValue& value) const {
-    return value_ == value.value_;
+    if (IsEmpty())
+      return value.IsEmpty();
+    if (value.IsEmpty())
+      return false;
+    return *value_ == *value.value_;
   }
 
   bool operator!=(const ScriptValue& value) const { return !operator==(value); }
@@ -155,11 +146,11 @@ class CORE_EXPORT ScriptValue final {
     return !value.IsEmpty() && value->IsObject();
   }
 
-  bool IsEmpty() const { return value_.IsEmpty(); }
+  bool IsEmpty() const { return !value_ || value_->IsEmpty(); }
 
   void Clear() {
     isolate_ = nullptr;
-    value_.Reset();
+    value_.Clear();
   }
 
   v8::Local<v8::Value> V8Value() const;
@@ -168,10 +159,6 @@ class CORE_EXPORT ScriptValue final {
   // this "clones" the v8 value and returns it.
   v8::Local<v8::Value> V8ValueFor(ScriptState*) const;
 
-  const WorldSafeV8Reference<v8::Value>& ToWorldSafeV8Reference() const {
-    return value_;
-  }
-
   bool ToString(String&) const;
 
   static ScriptValue CreateNull(v8::Isolate*);
@@ -179,8 +166,41 @@ class CORE_EXPORT ScriptValue final {
   void Trace(Visitor* visitor) { visitor->Trace(value_); }
 
  private:
+  // WorldSafeV8ReferenceWrapper wraps a WorldSafeV8Reference so that it can be
+  // used on both the stack and heaps. WorldSafeV8Reference cannot be used on
+  // the stack because the conservative scanning does not know how to trace
+  // TraceWrapperV8Reference.
+  class CORE_EXPORT WorldSafeV8ReferenceWrapper
+      : public GarbageCollected<WorldSafeV8ReferenceWrapper> {
+   public:
+    WorldSafeV8ReferenceWrapper() = default;
+    WorldSafeV8ReferenceWrapper(v8::Isolate* isolate,
+                                v8::Local<v8::Value> value)
+        : value_(isolate, value) {}
+
+    virtual ~WorldSafeV8ReferenceWrapper() = default;
+    void Trace(blink::Visitor* visitor) { visitor->Trace(value_); }
+
+    v8::Local<v8::Value> Get(ScriptState* script_state) const {
+      return value_.Get(script_state);
+    }
+
+    v8::Local<v8::Value> GetAcrossWorld(ScriptState* script_state) const {
+      return value_.GetAcrossWorld(script_state);
+    }
+
+    bool IsEmpty() const { return value_.IsEmpty(); }
+
+    bool operator==(const WorldSafeV8ReferenceWrapper& other) const {
+      return value_ == other.value_;
+    }
+
+   private:
+    WorldSafeV8Reference<v8::Value> value_;
+  };
+
   v8::Isolate* isolate_ = nullptr;
-  WorldSafeV8Reference<v8::Value> value_;
+  Member<WorldSafeV8ReferenceWrapper> value_;
 };
 
 template <>

third_party/blink/renderer/core/events/error_event.cc

@@ -66,7 +66,7 @@ ErrorEvent::ErrorEvent(ScriptState* script_state,
       initializer->hasLineno() ? initializer->lineno() : 0,
       initializer->hasColno() ? initializer->colno() : 0, nullptr);
   if (initializer->hasError()) {
-    error_ = initializer->error().ToWorldSafeV8Reference();
+    error_.Set(script_state->GetIsolate(), initializer->error().V8Value());
   }
 }
 
@@ -79,7 +79,7 @@ ErrorEvent::ErrorEvent(const String& message,
       location_(std::move(location)),
       world_(world) {
   if (!error.IsEmpty())
-    error_ = error.ToWorldSafeV8Reference();
+    error_.Set(error.GetIsolate(), error.V8Value());
 }
 
 void ErrorEvent::SetUnsanitizedMessage(const String& message) {

third_party/blink/renderer/core/events/message_event.cc

@@ -73,7 +73,8 @@ MessageEvent::MessageEvent(const AtomicString& type,
       data_type_(kDataTypeScriptValue),
       source_(nullptr) {
   if (initializer->hasData()) {
-    data_as_v8_value_ = initializer->data().ToWorldSafeV8Reference();
+    data_as_v8_value_.Set(initializer->data().GetIsolate(),
+                          initializer->data().V8Value());
   }
   if (initializer->hasOrigin())
     origin_ = initializer->origin();
@@ -194,7 +195,7 @@ void MessageEvent::initMessageEvent(const AtomicString& type,
   initEvent(type, bubbles, cancelable);
 
   data_type_ = kDataTypeScriptValue;
-  data_as_v8_value_ = data.ToWorldSafeV8Reference();
+  data_as_v8_value_.Set(data.GetIsolate(), data.V8Value());
   is_data_dirty_ = true;
   origin_ = origin;
   last_event_id_ = last_event_id;

third_party/blink/renderer/core/script/module_script.cc

@@ -50,23 +50,29 @@ void ModuleScript::SetParseErrorAndClearRecord(ScriptValue error) {
   DCHECK(!error.IsEmpty());
 
   record_.Clear();
-  parse_error_ = error.ToWorldSafeV8Reference();
+  parse_error_.Set(settings_object_->GetScriptState()->GetIsolate(),
+                   error.V8Value());
 }
 
 ScriptValue ModuleScript::CreateParseError() const {
-  v8::Isolate* isolate = settings_object_->GetScriptState()->GetIsolate();
-  ScriptValue error(isolate, parse_error_);
+  ScriptState* script_state = settings_object_->GetScriptState();
+  ScriptState::Scope scope(script_state);
+  ScriptValue error(script_state->GetIsolate(), parse_error_.Get(script_state));
   DCHECK(!error.IsEmpty());
   return error;
 }
 
 void ModuleScript::SetErrorToRethrow(ScriptValue error) {
-  error_to_rethrow_ = error.ToWorldSafeV8Reference();
+  ScriptState* script_state = settings_object_->GetScriptState();
+  ScriptState::Scope scope(script_state);
+  error_to_rethrow_.Set(script_state->GetIsolate(), error.V8Value());
 }
 
 ScriptValue ModuleScript::CreateErrorToRethrow() const {
-  v8::Isolate* isolate = settings_object_->GetScriptState()->GetIsolate();
-  ScriptValue error(isolate, error_to_rethrow_);
+  ScriptState* script_state = settings_object_->GetScriptState();
+  ScriptState::Scope scope(script_state);
+  ScriptValue error(script_state->GetIsolate(),
+                    error_to_rethrow_.Get(script_state));
   DCHECK(!error.IsEmpty());
   return error;
 }

third_party/blink/renderer/core/script/pending_import_map.cc

@@ -18,18 +18,20 @@ PendingImportMap* PendingImportMap::CreateInline(ScriptElementBase& element,
                                                  const KURL& base_url) {
   Document& element_document = element.GetDocument();
   Document* context_document = element_document.ContextDocument();
-  Modulator* modulator =
-      Modulator::From(ToScriptStateForMainWorld(context_document->GetFrame()));
+  ScriptState* script_state =
+      ToScriptStateForMainWorld(context_document->GetFrame());
+  Modulator* modulator = Modulator::From(script_state);
 
   ScriptValue error_to_rethrow;
   ImportMap* import_map =
       ImportMap::Parse(*modulator, import_map_text, base_url, *context_document,
                        &error_to_rethrow);
   return MakeGarbageCollected<PendingImportMap>(
-      element, import_map, error_to_rethrow, *context_document);
+      script_state, element, import_map, error_to_rethrow, *context_document);
 }
 
-PendingImportMap::PendingImportMap(ScriptElementBase& element,
+PendingImportMap::PendingImportMap(ScriptState* script_state,
+                                   ScriptElementBase& element,
                                    ImportMap* import_map,
                                    ScriptValue error_to_rethrow,
                                    const Document& original_context_document)
@@ -37,7 +39,9 @@ PendingImportMap::PendingImportMap(ScriptElementBase& element,
       import_map_(import_map),
       original_context_document_(&original_context_document) {
   if (!error_to_rethrow.IsEmpty()) {
-    error_to_rethrow_ = error_to_rethrow.ToWorldSafeV8Reference();
+    ScriptState::Scope scope(script_state);
+    error_to_rethrow_.Set(script_state->GetIsolate(),
+                          error_to_rethrow.V8Value());
   }
 }
 
@@ -81,8 +85,13 @@ void PendingImportMap::RegisterImportMap() const {
 
   Modulator* modulator = Modulator::From(ToScriptStateForMainWorld(frame));
 
-  v8::Isolate* isolate = modulator->GetScriptState()->GetIsolate();
-  ScriptValue error(isolate, error_to_rethrow_);
+  ScriptState* script_state = modulator->GetScriptState();
+  ScriptState::Scope scope(script_state);
+  ScriptValue error;
+  if (!error_to_rethrow_.IsEmpty()) {
+    error = ScriptValue(script_state->GetIsolate(),
+                        error_to_rethrow_.Get(script_state));
+  }
   modulator->RegisterImportMap(import_map_, error);
 
   // <spec step="9">If element is from an external file, then fire an event

third_party/blink/renderer/core/script/pending_import_map.h

@@ -40,7 +40,8 @@ class CORE_EXPORT PendingImportMap final
                                         const String& import_map_text,
                                         const KURL& base_url);
 
-  PendingImportMap(ScriptElementBase&,
+  PendingImportMap(ScriptState* script_state,
+                   ScriptElementBase&,
                    ImportMap*,
                    ScriptValue error_to_rethrow,
                    const Document& original_context_document);

third_party/blink/renderer/modules/wake_lock/wake_lock_controller_test.cc

@@ -276,13 +276,13 @@ TEST(WakeLockControllerTest, LossOfDocumentActivity) {
   // First, acquire a handful of locks of different types.
   auto* screen_resolver1 =
       MakeGarbageCollected<ScriptPromiseResolver>(context.GetScriptState());
-  ScriptPromise screen_promise1 = screen_resolver1->Promise();
+  screen_resolver1->Promise();
   auto* screen_resolver2 =
       MakeGarbageCollected<ScriptPromiseResolver>(context.GetScriptState());
-  ScriptPromise screen_promise2 = screen_resolver2->Promise();
+  screen_resolver2->Promise();
   auto* system_resolver1 =
       MakeGarbageCollected<ScriptPromiseResolver>(context.GetScriptState());
-  ScriptPromise system_promise1 = system_resolver1->Promise();
+  system_resolver1->Promise();
   controller.RequestWakeLock(WakeLockType::kScreen, screen_resolver1,
                              /*signal=*/nullptr);
   controller.RequestWakeLock(WakeLockType::kScreen, screen_resolver2,