Replace SharedPersistent with WorldSafeV8Reference

Replace ScriptState* with Isolate*

We replace SharedPersistent with WorldSafeV8Reference to remove
persistent, and to ensure v8::Local<v8::Value> is world safe. (ref. V8Value())

We also replace ScriptState* with Isolate* because ScriptValue doesn't
depend on context any more.

Bug: 1000152, 998994
Change-Id: If232d1da9579cf73157e400bf0c40950b2f3a095
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1787672
Commit-Queue: Rika Fujimaki <rikaf@google.com>
Reviewed-by: Keishi Hattori <keishi@chromium.org>
Reviewed-by: Kouhei Ueno <kouhei@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#699288}

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

@@ -35,43 +35,13 @@
 #include "third_party/blink/renderer/platform/bindings/script_state.h"
 
 namespace blink {
-namespace {
-
-v8::Local<v8::Value> ToWorldSafeValue(ScriptState* target_script_state,
-                                      v8::Local<v8::Value> value) {
-  if (value.IsEmpty() || !value->IsObject())
-    return value;
-
-  v8::Local<v8::Context> creation_context =
-      value.As<v8::Object>()->CreationContext();
-  v8::Isolate* isolate = target_script_state->GetIsolate();
-  if (&ScriptState::From(creation_context)->World() ==
-      &target_script_state->World()) {
-    return value;
-  }
-
-  v8::Context::Scope target_context_scope(target_script_state->GetContext());
-  scoped_refptr<SerializedScriptValue> serialized =
-      SerializedScriptValue::SerializeAndSwallowExceptions(isolate, value);
-  return serialized->Deserialize(isolate);
-}
-
-}  // namespace
 
 v8::Local<v8::Value> ScriptValue::V8Value() const {
   if (IsEmpty())
     return v8::Local<v8::Value>();
 
   DCHECK(GetIsolate()->InContext());
-
-  // This is a check to validate that you don't return a ScriptValue to a world
-  // different from the world that created the ScriptValue.
-  // Probably this could be:
-  //   if (&script_state_->world() == &DOMWrapperWorld::current(isolate()))
-  //       return v8::Local<v8::Value>();
-  // instead of triggering CHECK.
-  CHECK_EQ(&script_state_->World(), &DOMWrapperWorld::Current(GetIsolate()));
-  return value_->NewLocal(GetIsolate());
+  return value_.Get(ScriptState::From(isolate_->GetCurrentContext()));
 }
 
 v8::Local<v8::Value> ScriptValue::V8ValueFor(
@@ -79,15 +49,13 @@ v8::Local<v8::Value> ScriptValue::V8ValueFor(
   if (IsEmpty())
     return v8::Local<v8::Value>();
 
-  return ToWorldSafeValue(target_script_state,
-                          value_->NewLocal(target_script_state->GetIsolate()));
+  return value_.GetAcrossWorld(target_script_state);
 }
 
 bool ScriptValue::ToString(String& result) const {
   if (IsEmpty())
     return false;
 
-  ScriptState::Scope scope(script_state_);
   v8::Local<v8::Value> string = V8Value();
   if (string.IsEmpty() || !string->IsString())
     return false;
@@ -95,8 +63,10 @@ bool ScriptValue::ToString(String& result) const {
   return true;
 }
 
+// TODO(rikaf): Replace with CreateNull(v8::Isolate*).
 ScriptValue ScriptValue::CreateNull(ScriptState* script_state) {
-  return ScriptValue(script_state, v8::Null(script_state->GetIsolate()));
+  return ScriptValue(script_state->GetIsolate(),
+                     v8::Null(script_state->GetIsolate()));
 }
 
 }  // namespace blink

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

@@ -33,6 +33,7 @@
 
 #include "base/memory/scoped_refptr.h"
 #include "third_party/blink/renderer/bindings/core/v8/native_value_traits.h"
+#include "third_party/blink/renderer/bindings/core/v8/world_safe_v8_reference.h"
 #include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/platform/bindings/script_state.h"
 #include "third_party/blink/renderer/platform/bindings/shared_persistent.h"
@@ -69,51 +70,66 @@ class CORE_EXPORT ScriptValue final {
 
   ScriptValue() = default;
 
+  // TODO(rikaf): Forbid passing empty v8::Local<v8::Value> to ScriptValue's
+  // ctor.
+
+  // TODO(crbug.com/998994): Remove ScriptValue(ScriptState*,
+  // v8::Local<v8::Value>) once we finish replacing with ScriptValue(Isolate*,
+  // v8::Local<v8::Value>)
   ScriptValue(ScriptState* script_state, v8::Local<v8::Value> value)
-      : script_state_(script_state),
-        value_(value.IsEmpty() ? nullptr
-                               : SharedPersistent<v8::Value>::Create(
-                                     value,
-                                     script_state->GetIsolate())) {
-    DCHECK(IsEmpty() || script_state_);
+      : isolate_(script_state->GetIsolate()),
+        value_(value.IsEmpty()
+                   ? WorldSafeV8Reference<v8::Value>()
+                   : WorldSafeV8Reference<v8::Value>(script_state->GetIsolate(),
+                                                     value)) {
+    DCHECK(isolate_);
+  }
+
+  ScriptValue(v8::Isolate* isolate, v8::Local<v8::Value> value)
+      : isolate_(isolate),
+        value_(value.IsEmpty()
+                   ? WorldSafeV8Reference<v8::Value>()
+                   : WorldSafeV8Reference<v8::Value>(isolate, value)) {
+    DCHECK(isolate_);
   }
 
   template <typename T>
   ScriptValue(ScriptState* script_state, v8::MaybeLocal<T> value)
-      : script_state_(script_state),
-        value_(value.IsEmpty() ? nullptr
-                               : SharedPersistent<v8::Value>::Create(
-                                     value.ToLocalChecked(),
-                                     script_state->GetIsolate())) {
-    DCHECK(IsEmpty() || script_state_);
+      : isolate_(script_state->GetIsolate()),
+        value_(value.IsEmpty()
+                   ? WorldSafeV8Reference<v8::Value>()
+                   : WorldSafeV8Reference<v8::Value>(script_state->GetIsolate(),
+                                                     value.ToLocalChecked())) {
+    DCHECK(isolate_);
   }
 
-  ScriptValue(const ScriptValue& value)
-      : script_state_(value.script_state_), value_(value.value_) {
-    DCHECK(IsEmpty() || script_state_);
+  ScriptValue(v8::Isolate* isolate, WorldSafeV8Reference<v8::Value> value)
+      : isolate_(isolate), value_(value) {
+    DCHECK(isolate_);
   }
 
-  ScriptState* GetScriptState() const { return script_state_; }
+  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_);
+    }
+  }
 
+  // Use this GetIsolate() to do DCHECK inside ScriptValue.
   v8::Isolate* GetIsolate() const {
-    return script_state_ ? script_state_->GetIsolate()
-                         : v8::Isolate::GetCurrent();
+    DCHECK(isolate_);
+    return isolate_;
   }
 
-  ScriptValue& operator=(const ScriptValue& value) {
-    if (this != &value) {
-      script_state_ = value.script_state_;
-      value_ = value.value_;
-    }
-    return *this;
-  }
+  ScriptValue& operator=(const ScriptValue& value) = default;
 
   bool operator==(const ScriptValue& value) const {
-    if (IsEmpty())
-      return value.IsEmpty();
-    if (value.IsEmpty())
-      return false;
-    return *value_ == *value.value_;
+    return value_ == value.value_;
   }
 
   bool operator!=(const ScriptValue& value) const { return !operator==(value); }
@@ -150,9 +166,12 @@ class CORE_EXPORT ScriptValue final {
     return !value.IsEmpty() && value->IsObject();
   }
 
-  bool IsEmpty() const { return !value_.get() || value_->IsEmpty(); }
+  bool IsEmpty() const { return value_.IsEmpty(); }
 
-  void Clear() { value_ = nullptr; }
+  void Clear() {
+    isolate_ = nullptr;
+    value_.Reset();
+  }
 
   v8::Local<v8::Value> V8Value() const;
   // Returns v8Value() if a given ScriptState is the same as the
@@ -160,15 +179,19 @@ 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(ScriptState*);
 
-  void Trace(Visitor* visitor) { visitor->Trace(script_state_); }
+  void Trace(Visitor* visitor) { visitor->Trace(value_); }
 
  private:
-  Member<ScriptState> script_state_;
-  scoped_refptr<SharedPersistent<v8::Value>> value_;
+  v8::Isolate* isolate_ = nullptr;
+  WorldSafeV8Reference<v8::Value> value_;
 };
 
 template <>

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

@@ -107,14 +107,19 @@ class WorldSafeV8Reference final {
 
   void Trace(blink::Visitor* visitor) { visitor->Trace(v8_reference_); }
 
+  WorldSafeV8Reference& operator=(const WorldSafeV8Reference<V8Type>& other) =
+      default;
+
+  bool operator==(const WorldSafeV8Reference<V8Type>& other) const {
+    return v8_reference_ == other.v8_reference_;
+  }
+
  private:
   TraceWrapperV8Reference<V8Type> v8_reference_;
   // The world of the current context at the time when |v8_reference_| was set.
   // It's guaranteed that, if |v8_reference_| is a v8::Object, the world of the
   // creation context of |v8_reference_| is the same as |world_|.
   scoped_refptr<const DOMWrapperWorld> world_;
-
-  DISALLOW_COPY_AND_ASSIGN(WorldSafeV8Reference);
 };
 
 }  // namespace blink

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

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

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

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

third_party/blink/renderer/core/loader/modulescript/module_tree_linker.cc

@@ -342,6 +342,8 @@ void ModuleTreeLinker::NotifyModuleLoadFinished(ModuleScript* module_script) {
 void ModuleTreeLinker::FetchDescendants(const ModuleScript* module_script) {
   DCHECK(module_script);
 
+  v8::Isolate* isolate = modulator_->GetScriptState()->GetIsolate();
+  v8::HandleScope scope(isolate);
   // [nospec] Abort the steps if the browsing context is discarded.
   if (!modulator_->HasValidContext()) {
     result_ = nullptr;
@@ -349,9 +351,6 @@ void ModuleTreeLinker::FetchDescendants(const ModuleScript* module_script) {
     return;
   }
 
-  // TODO(crbug.com/1000152): Replace ScriptState::Scope with v8::HandleScope
-  ScriptState::Scope scope(modulator_->GetScriptState());
-
   // <spec step="2">Let record be module script's record.</spec>
   v8::Local<v8::Module> record = module_script->V8Module();
 

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

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

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

@@ -7,6 +7,7 @@
 
 #include "third_party/blink/renderer/bindings/core/v8/module_record.h"
 #include "third_party/blink/renderer/bindings/core/v8/script_value.h"
+#include "third_party/blink/renderer/bindings/core/v8/world_safe_v8_reference.h"
 #include "third_party/blink/renderer/core/core_export.h"
 #include "third_party/blink/renderer/core/script/modulator.h"
 #include "third_party/blink/renderer/core/script/script.h"
@@ -115,10 +116,10 @@ class CORE_EXPORT ModuleScript : public Script {
   //   https://github.com/whatwg/html/pull/2991. This shouldn't cause any
   //   observable functional changes, and updating the classic script handling
   //   will require moderate code changes (e.g. to move compilation timing).
-  TraceWrapperV8Reference<v8::Value> parse_error_;
+  WorldSafeV8Reference<v8::Value> parse_error_;
 
   // https://html.spec.whatwg.org/C/#concept-script-error-to-rethrow
-  TraceWrapperV8Reference<v8::Value> error_to_rethrow_;
+  WorldSafeV8Reference<v8::Value> error_to_rethrow_;
 
   mutable HashMap<String, KURL> specifier_to_url_cache_;
   KURL source_url_;

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

@@ -37,9 +37,7 @@ PendingImportMap::PendingImportMap(ScriptElementBase& element,
       import_map_(import_map),
       original_context_document_(&original_context_document) {
   if (!error_to_rethrow.IsEmpty()) {
-    ScriptState::Scope scope(error_to_rethrow.GetScriptState());
-    error_to_rethrow_.Set(error_to_rethrow.GetIsolate(),
-                          error_to_rethrow.V8Value());
+    error_to_rethrow_ = error_to_rethrow.ToWorldSafeV8Reference();
   }
 }
 
@@ -83,10 +81,8 @@ void PendingImportMap::RegisterImportMap() const {
 
   Modulator* modulator = Modulator::From(ToScriptStateForMainWorld(frame));
 
-  ScriptState* script_state = modulator->GetScriptState();
-  ScriptState::Scope scope(script_state);
-  ScriptValue error(script_state,
-                    error_to_rethrow_.NewLocal(script_state->GetIsolate()));
+  v8::Isolate* isolate = modulator->GetScriptState()->GetIsolate();
+  ScriptValue error(isolate, error_to_rethrow_);
   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

@@ -5,8 +5,8 @@
 #ifndef THIRD_PARTY_BLINK_RENDERER_CORE_SCRIPT_PENDING_IMPORT_MAP_H_
 #define THIRD_PARTY_BLINK_RENDERER_CORE_SCRIPT_PENDING_IMPORT_MAP_H_
 
+#include "third_party/blink/renderer/bindings/core/v8/world_safe_v8_reference.h"
 #include "third_party/blink/renderer/core/core_export.h"
-#include "third_party/blink/renderer/platform/bindings/trace_wrapper_v8_reference.h"
 #include "third_party/blink/renderer/platform/heap/garbage_collected.h"
 #include "third_party/blink/renderer/platform/heap/member.h"
 #include "third_party/blink/renderer/platform/wtf/text/wtf_string.h"
@@ -57,7 +57,7 @@ class CORE_EXPORT PendingImportMap final
 
   // https://wicg.github.io/import-maps/#import-map-parse-result-error-to-rethrow
   // The error is TypeError if the string is non-null, or null otherwise.
-  TraceWrapperV8Reference<v8::Value> error_to_rethrow_;
+  WorldSafeV8Reference<v8::Value> error_to_rethrow_;
 
   // https://wicg.github.io/import-maps/#import-map-parse-result-settings-object
   // The context document at the time when PrepareScript() is executed.

third_party/blink/renderer/core/streams/writable_stream_test.cc

@@ -52,7 +52,7 @@ TEST_P(WritableStreamTest, GetWriter) {
   EXPECT_EQ(stream->IsLocked(script_state, ASSERT_NO_EXCEPTION),
             base::make_optional(false));
 
-  ScriptValue writer = stream->getWriter(script_state, ASSERT_NO_EXCEPTION);
+  stream->getWriter(script_state, ASSERT_NO_EXCEPTION);
 
   EXPECT_TRUE(stream->locked(script_state, ASSERT_NO_EXCEPTION));
   EXPECT_EQ(stream->IsLocked(script_state, ASSERT_NO_EXCEPTION),

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

@@ -315,8 +315,7 @@ String ScriptPromiseUtils::GetPromiseResolutionAsString(
   if (v8_promise->State() == v8::Promise::kPending) {
     return g_empty_string;
   }
-  ScriptValue promise_result(promise.GetScriptValue().GetScriptState(),
-                             v8_promise->Result());
+  ScriptValue promise_result(promise.GetIsolate(), v8_promise->Result());
   String value;
   if (!promise_result.ToString(value)) {
     return g_empty_string;