bindings: Move DOMDataStore on Oilpan's heap

This change moves DOMDataStore on Oilpan's heap and models wrappable->wrapper
connections using an ephemeron hash map for non-main worlds.

Previously, DOMDataStore would be off heap and the that maintains the references
would use Untraced<> as well as custom Trace() methods for non-GCed objects.
That also required callbacks on the references to manually remove the entries to
compact the hash map.

With ephemerons:
- The hashmap is kept compact (unused buckets are deleted) by the GC which
  allows us to avoid callbacks on references.
- There are no more GC plugin exceptions.
- All trace methods will be on garbage collection objects.
- There's no need to trace through all DOM worlds for each ScriptWrappable in
  its trace method.
- References will show up in heap snapshots.

Bug: 1013212
Change-Id: Id3475707d34191f9f3b6b099c2495ee0150d982e
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1849677
Commit-Queue: Michael Lippautz <mlippautz@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#705062}

third_party/blink/renderer/platform/BUILD.gn

@@ -401,6 +401,7 @@ jumbo_component("platform") {
     "bindings/callback_method_retriever.cc",
     "bindings/callback_method_retriever.h",
     "bindings/custom_wrappable.h",
+    "bindings/dom_data_store.cc",
     "bindings/dom_data_store.h",
     "bindings/dom_wrapper_world.cc",
     "bindings/dom_wrapper_world.h",

third_party/blink/renderer/platform/bindings/dom_data_store.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "third_party/blink/renderer/platform/bindings/dom_data_store.h"
+
+namespace blink {
+
+DOMDataStore::DOMDataStore(v8::Isolate* isolate, bool is_main_world)
+    : is_main_world_(is_main_world) {}
+
+void DOMDataStore::Dispose() {
+  for (const auto& it : wrapper_map_) {
+    // Explicitly reset references so that a following V8 GC will not find them
+    // and treat them as roots. There's optimizations (see
+    // EmbedderHeapTracer::IsRootForNonTracingGC) that would not treat them as
+    // roots and then Blink would not be able to find and remove them from a DOM
+    // world. Explicitly resetting on disposal avoids that problem
+    it.value->ref.Clear();
+  }
+}
+
+void DOMDataStore::WrappedReference::Trace(Visitor* visitor) {
+  visitor->Trace(ref);
+}
+
+void DOMDataStore::Trace(Visitor* visitor) {
+  visitor->Trace(wrapper_map_);
+}
+
+}  // namespace blink

third_party/blink/renderer/platform/bindings/dom_data_store.h

@@ -48,9 +48,7 @@ namespace blink {
 // wrappers and provides an API to perform common operations with this map and
 // manage wrappers in a single world. Each world (DOMWrapperWorld) holds a
 // single map instance to hold wrappers only for that world.
-class DOMDataStore {
-  USING_FAST_MALLOC(DOMDataStore);
-
+class DOMDataStore final : public GarbageCollected<DOMDataStore> {
  public:
   static DOMDataStore& Current(v8::Isolate* isolate) {
     return DOMWrapperWorld::Current(isolate).DomDataStore();
@@ -110,20 +108,17 @@ class DOMDataStore {
     return Current(isolate).ContainsWrapper(object);
   }
 
-  DOMDataStore(v8::Isolate* isolate, bool is_main_world)
-      : is_main_world_(is_main_world) {
-    // We never use |wrapper_map_| when it's the main world.
-    if (!is_main_world) {
-      wrapper_map_.emplace();
-    }
-  }
+  DOMDataStore(v8::Isolate* isolate, bool is_main_world);
+
+  // Clears all references.
+  void Dispose();
 
   v8::Local<v8::Object> Get(ScriptWrappable* object, v8::Isolate* isolate) {
     if (is_main_world_)
       return object->MainWorldWrapper(isolate);
-    auto it = wrapper_map_->find(object);
-    if (it != wrapper_map_->end())
-      return it->value.NewLocal(isolate);
+    auto it = wrapper_map_.find(object);
+    if (it != wrapper_map_.end())
+      return it->value->ref.NewLocal(isolate);
     return v8::Local<v8::Object>();
   }
 
@@ -136,41 +131,40 @@ class DOMDataStore {
     if (is_main_world_)
       return object->SetWrapper(isolate, wrapper_type_info, wrapper);
 
-    auto result = wrapper_map_->insert(object, Value(isolate, wrapper));
+    auto result = wrapper_map_.insert(
+        object, MakeGarbageCollected<WrappedReference>(isolate, wrapper));
     if (LIKELY(result.is_new_entry)) {
-      wrapper_type_info->ConfigureWrapper(&result.stored_value->value.Get());
-      result.stored_value->value.Get().SetFinalizationCallback(
-          this, RemoveEntryFromMap);
+      wrapper_type_info->ConfigureWrapper(
+          &result.stored_value->value->ref.Get());
     } else {
-      DCHECK(!result.stored_value->value.IsEmpty());
-      wrapper = result.stored_value->value.NewLocal(isolate);
+      DCHECK(!result.stored_value->value->ref.IsEmpty());
+      wrapper = result.stored_value->value->ref.NewLocal(isolate);
     }
     return result.is_new_entry;
   }
 
-  void Trace(const ScriptWrappable* script_wrappable, Visitor* visitor) {
-    DCHECK(wrapper_map_);
-    auto it =
-        wrapper_map_->find(const_cast<ScriptWrappable*>(script_wrappable));
-    if (it != wrapper_map_->end()) {
-      visitor->Trace(
-          static_cast<TraceWrapperV8Reference<v8::Object>&>(it->value));
+  bool UnsetSpecificWrapperIfSet(
+      ScriptWrappable* object,
+      const v8::TracedReference<v8::Object>& handle) {
+    DCHECK(!is_main_world_);
+    const auto& it = wrapper_map_.find(object);
+    if (it != wrapper_map_.end()) {
+      if (it->value->ref.Get() == handle) {
+        it->value->ref.Clear();
+        wrapper_map_.erase(it);
+        return true;
       }
     }
-
-  // Dissociates a wrapper, if any, from |script_wrappable|.
-  void UnsetWrapperIfAny(ScriptWrappable* script_wrappable) {
-    DCHECK(!is_main_world_);
-    wrapper_map_->erase(script_wrappable);
+    return false;
   }
 
   bool SetReturnValueFrom(v8::ReturnValue<v8::Value> return_value,
                           ScriptWrappable* object) {
     if (is_main_world_)
       return object->SetReturnValue(return_value);
-    auto it = wrapper_map_->find(object);
-    if (it != wrapper_map_->end()) {
-      return_value.Set(it->value.Get());
+    auto it = wrapper_map_.find(object);
+    if (it != wrapper_map_.end()) {
+      return_value.Set(it->value->ref.Get());
       return true;
     }
     return false;
@@ -179,10 +173,12 @@ class DOMDataStore {
   bool ContainsWrapper(const ScriptWrappable* object) {
     if (is_main_world_)
       return object->ContainsWrapper();
-    return wrapper_map_->find(const_cast<ScriptWrappable*>(object)) !=
-           wrapper_map_->end();
+    return wrapper_map_.find(const_cast<ScriptWrappable*>(object)) !=
+           wrapper_map_.end();
   }
 
+  virtual void Trace(Visitor*);
+
  private:
   // We can use a wrapper stored in a ScriptWrappable when we're in the main
   // world.  This method does the fast check if we're in the main world. If this
@@ -203,58 +199,25 @@ class DOMDataStore {
     return wrappable->IsEqualTo(holder);
   }
 
-  static void RemoveEntryFromMap(const v8::WeakCallbackInfo<void>& data) {
-    DOMDataStore* store = reinterpret_cast<DOMDataStore*>(data.GetParameter());
-    ScriptWrappable* key = reinterpret_cast<ScriptWrappable*>(
-        data.GetInternalField(kV8DOMWrapperObjectIndex));
-    const auto& it = store->wrapper_map_->find(key);
-    DCHECK_NE(store->wrapper_map_->end(), it);
-    store->wrapper_map_->erase(it);
-    WrapperTypeInfo::WrapperDestroyed();
-  }
-
-  // Specialization of TraceWrapperV8Reference to avoid write barriers on move
-  // operations. This is correct as entries are never moved out of the storage
-  // but only moved for rehashing purposes.
-  //
-  // We need to avoid write barriers to allow resizing of the hashmap backing
-  // during V8 garbage collections. The resize is triggered when entries are
-  // removed which happens in a phase where V8 prohibits any API calls. To work
-  // around that we just don't emit write barriers for moving.
-  class DOMWorldWrapperReference : public TraceWrapperV8Reference<v8::Object> {
+  // Wrapper around TraceWrapperV8Reference to allow use in ephemeron hash map
+  // below.
+  class PLATFORM_EXPORT WrappedReference final
+      : public GarbageCollected<WrappedReference> {
    public:
-    DOMWorldWrapperReference() = default;
-    // Regular write barrier for constructor is emitted by
-    // TraceWrapperV8Reference.
-    DOMWorldWrapperReference(v8::Isolate* isolate, v8::Local<v8::Object> handle)
-        : TraceWrapperV8Reference(isolate, handle) {}
+    WrappedReference() = default;
+    WrappedReference(v8::Isolate* isolate, v8::Local<v8::Object> handle)
+        : ref(isolate, handle) {}
 
-    ~DOMWorldWrapperReference() {
-      // Destruction of a reference should clear it immediately.
-      Clear();
-    }
+    virtual void Trace(Visitor*);
 
-    // Move support without write barrier.
-    DOMWorldWrapperReference(DOMWorldWrapperReference&& other)
-        : TraceWrapperV8Reference() {
-      handle_ = std::move(other.handle_);
-    }
-    DOMWorldWrapperReference& operator=(DOMWorldWrapperReference&& rhs) {
-      handle_ = std::move(rhs.handle_);
-      return *this;
-    }
+    TraceWrapperV8Reference<v8::Object> ref;
   };
 
-  // UntracedMember is safe here as the map is not keeping ScriptWrappable alive
-  // but merely adding additional edges from Blink to V8.
-  using Key = UntracedMember<ScriptWrappable>;
-  using Value = DOMWorldWrapperReference;
-  using MapType = WTF::HashMap<Key, Value>;
-
   bool is_main_world_;
-  GC_PLUGIN_IGNORE(
-      "Avoid dispatch on Visitor by looking up value in DOMDataStore::Trace.")
-  base::Optional<MapType> wrapper_map_;
+  // Ephemeron map: WrappedReference will be kept alive as long as
+  // ScriptWrappable is alive.
+  HeapHashMap<WeakMember<ScriptWrappable>, Member<WrappedReference>>
+      wrapper_map_;
 
   DISALLOW_COPY_AND_ASSIGN(DOMDataStore);
 };

third_party/blink/renderer/platform/bindings/dom_wrapper_world.cc

@@ -73,7 +73,8 @@ DOMWrapperWorld::DOMWrapperWorld(v8::Isolate* isolate,
                                  int32_t world_id)
     : world_type_(world_type),
       world_id_(world_id),
-      dom_data_store_(std::make_unique<DOMDataStore>(isolate, IsMainWorld())) {
+      dom_data_store_(
+          MakeGarbageCollected<DOMDataStore>(isolate, IsMainWorld())) {
   switch (world_type_) {
     case WorldType::kMain:
       // The main world is managed separately from worldMap(). See worldMap().
@@ -109,16 +110,6 @@ void DOMWrapperWorld::AllWorldsInCurrentThread(
     worlds.push_back(world);
 }
 
-void DOMWrapperWorld::Trace(const ScriptWrappable* script_wrappable,
-                            Visitor* visitor) {
-  // Marking for worlds other than the main world.
-  for (DOMWrapperWorld* world : GetWorldMap().Values()) {
-    DOMDataStore& data_store = world->DomDataStore();
-    if (data_store.ContainsWrapper(script_wrappable))
-      data_store.Trace(script_wrappable, visitor);
-  }
-}
-
 DOMWrapperWorld::~DOMWrapperWorld() {
   DCHECK(!IsMainWorld());
   if (IsMainThread())
@@ -131,7 +122,8 @@ DOMWrapperWorld::~DOMWrapperWorld() {
 }
 
 void DOMWrapperWorld::Dispose() {
-  dom_data_store_.reset();
+  dom_data_store_->Dispose();
+  dom_data_store_.Clear();
   DCHECK(GetWorldMap().Contains(world_id_));
   GetWorldMap().erase(world_id_);
 }
@@ -238,17 +230,6 @@ int DOMWrapperWorld::GenerateWorldIdForType(WorldType world_type) {
   return kInvalidWorldId;
 }
 
-void DOMWrapperWorld::DissociateDOMWindowWrappersInAllWorlds(
-    ScriptWrappable* script_wrappable) {
-  DCHECK(script_wrappable);
-  DCHECK(IsMainThread());
-
-  script_wrappable->UnsetWrapperIfAny();
-
-  for (auto*& world : GetWorldMap().Values())
-    world->DomDataStore().UnsetWrapperIfAny(script_wrappable);
-}
-
 bool DOMWrapperWorld::HasWrapperInAnyWorldInMainThread(
     ScriptWrappable* script_wrappable) {
   DCHECK(IsMainThread());
@@ -263,4 +244,16 @@ bool DOMWrapperWorld::HasWrapperInAnyWorldInMainThread(
   return false;
 }
 
+// static
+bool DOMWrapperWorld::UnsetNonMainWorldWrapperIfSet(
+    ScriptWrappable* object,
+    const v8::TracedReference<v8::Object>& handle) {
+  for (DOMWrapperWorld* world : GetWorldMap().Values()) {
+    DOMDataStore& data_store = world->DomDataStore();
+    if (data_store.UnsetSpecificWrapperIfSet(object, handle))
+      return true;
+  }
+  return false;
+}
+
 }  // namespace blink

third_party/blink/renderer/platform/bindings/dom_wrapper_world.h

@@ -37,6 +37,7 @@
 #include "base/memory/scoped_refptr.h"
 #include "third_party/blink/public/platform/web_isolated_world_ids.h"
 #include "third_party/blink/renderer/platform/bindings/script_state.h"
+#include "third_party/blink/renderer/platform/bindings/script_wrappable.h"
 #include "third_party/blink/renderer/platform/platform_export.h"
 #include "third_party/blink/renderer/platform/wtf/allocator/allocator.h"
 #include "third_party/blink/renderer/platform/wtf/hash_set.h"
@@ -104,9 +105,6 @@ class PLATFORM_EXPORT DOMWrapperWorld : public RefCounted<DOMWrapperWorld> {
   static void AllWorldsInCurrentThread(
       Vector<scoped_refptr<DOMWrapperWorld>>& worlds);
 
-  // Traces wrappers corresponding to the ScriptWrappable in DOM data stores.
-  static void Trace(const ScriptWrappable*, Visitor*);
-
   static DOMWrapperWorld& World(v8::Local<v8::Context> context) {
     return ScriptState::From(context)->World();
   }
@@ -142,7 +140,16 @@ class PLATFORM_EXPORT DOMWrapperWorld : public RefCounted<DOMWrapperWorld> {
   int GetWorldId() const { return world_id_; }
   DOMDataStore& DomDataStore() const { return *dom_data_store_; }
 
+  // Clear the reference pointing from |object| to |handle| in any world.
+  static bool UnsetSpecificWrapperIfSet(
+      ScriptWrappable* object,
+      const v8::TracedReference<v8::Object>& handle);
+
  private:
+  static bool UnsetNonMainWorldWrapperIfSet(
+      ScriptWrappable* object,
+      const v8::TracedReference<v8::Object>& handle);
+
   DOMWrapperWorld(v8::Isolate*, WorldType, int32_t world_id);
 
   static unsigned number_of_non_main_worlds_in_main_thread_;
@@ -152,35 +159,23 @@ class PLATFORM_EXPORT DOMWrapperWorld : public RefCounted<DOMWrapperWorld> {
   // out of DOMWrapperWorld.
   static int GenerateWorldIdForType(WorldType);
 
-  // Dissociates all wrappers in all worlds associated with |script_wrappable|.
-  //
-  // Do not use this function except for DOMWindow.  Only DOMWindow needs to
-  // dissociate wrappers from the ScriptWrappable because of the following two
-  // reasons.
-  //
-  // Reason 1) Case of the main world
-  // A DOMWindow may be collected by Blink GC *before* V8 GC collects the
-  // wrapper because the wrapper object associated with a DOMWindow is a global
-  // proxy, which remains after navigations.  We don't want V8 GC to reset the
-  // weak persistent handle to a wrapper within the DOMWindow
-  // (ScriptWrappable::main_world_wrapper_) *after* Blink GC collects the
-  // DOMWindow because it's use-after-free.  Thus, we need to dissociate the
-  // wrapper in advance.
-  //
-  // Reason 2) Case of isolated worlds
-  // As same, a DOMWindow may be collected before the wrapper gets collected.
-  // A DOMWrapperMap supports mapping from ScriptWrappable* to v8::Global<T>,
-  // and we don't want to leave an entry of an already-dead DOMWindow* to the
-  // persistent handle for the global proxy object, especially considering that
-  // the address to the already-dead DOMWindow* may be re-used.
-  friend class DOMWindow;
-  static void DissociateDOMWindowWrappersInAllWorlds(ScriptWrappable*);
-
   const WorldType world_type_;
   const int32_t world_id_;
-  std::unique_ptr<DOMDataStore> dom_data_store_;
+  Persistent<DOMDataStore> dom_data_store_;
 };
 
+// static
+inline bool DOMWrapperWorld::UnsetSpecificWrapperIfSet(
+    ScriptWrappable* object,
+    const v8::TracedReference<v8::Object>& handle) {
+  // Fast path for main world.
+  if (object->UnsetMainWorldWrapperIfSet(handle))
+    return true;
+
+  // Slow path: |object| may point to |handle| in any non-main DOM world.
+  return DOMWrapperWorld::UnsetNonMainWorldWrapperIfSet(object, handle);
+}
+
 }  // namespace blink
 
 #endif  // THIRD_PARTY_BLINK_RENDERER_PLATFORM_BINDINGS_DOM_WRAPPER_WORLD_H_

third_party/blink/renderer/platform/bindings/script_wrappable.cc

@@ -40,7 +40,6 @@ v8::Local<v8::Object> ScriptWrappable::AssociateWithWrapper(
 
 void ScriptWrappable::Trace(Visitor* visitor) {
   visitor->Trace(main_world_wrapper_);
-  DOMWrapperWorld::Trace(this, visitor);
 }
 
 const char* ScriptWrappable::NameInHeapSnapshot() const {

third_party/blink/renderer/platform/bindings/script_wrappable.h

@@ -128,14 +128,6 @@ class PLATFORM_EXPORT ScriptWrappable
     return true;
   }
 
-  // Dissociates the wrapper, if any, from this instance.
-  void UnsetWrapperIfAny() {
-    if (ContainsWrapper()) {
-      main_world_wrapper_.Get().Reset();
-      WrapperTypeInfo::WrapperDestroyed();
-    }
-  }
-
   bool IsEqualTo(const v8::Local<v8::Object>& other) const {
     return main_world_wrapper_.Get() == other;
   }
@@ -151,16 +143,14 @@ class PLATFORM_EXPORT ScriptWrappable
   ScriptWrappable() = default;
 
  private:
-  // These classes are exceptionally allowed to use MainWorldWrapper().
-  friend class DOMDataStore;
-  friend class HeapSnaphotWrapperVisitor;
-  friend class V8HiddenValue;
-  friend class V8PrivateProperty;
-
   v8::Local<v8::Object> MainWorldWrapper(v8::Isolate* isolate) const {
     return main_world_wrapper_.NewLocal(isolate);
   }
 
+  // Clear the main world wrapper if it is set to |handle|.
+  bool UnsetMainWorldWrapperIfSet(
+      const v8::TracedReference<v8::Object>& handle);
+
   static_assert(
       std::is_trivially_destructible<
           TraceWrapperV8Reference<v8::Object>>::value,
@@ -168,9 +158,26 @@ class PLATFORM_EXPORT ScriptWrappable
 
   TraceWrapperV8Reference<v8::Object> main_world_wrapper_;
 
+  // These classes are exceptionally allowed to directly interact with the main
+  // world wrapper.
+  friend class DOMDataStore;
+  friend class DOMWrapperWorld;
+  friend class HeapSnaphotWrapperVisitor;
+  friend class V8HiddenValue;
+  friend class V8PrivateProperty;
+
   DISALLOW_COPY_AND_ASSIGN(ScriptWrappable);
 };
 
+inline bool ScriptWrappable::UnsetMainWorldWrapperIfSet(
+    const v8::TracedReference<v8::Object>& handle) {
+  if (main_world_wrapper_.Get() == handle) {
+    main_world_wrapper_.Clear();
+    return true;
+  }
+  return false;
+}
+
 // Defines |GetWrapperTypeInfo| virtual method which returns the WrapperTypeInfo
 // of the instance. Also declares a static member of type WrapperTypeInfo, of
 // which the definition is given by the IDL code generator.

third_party/blink/renderer/platform/heap/unified_heap_controller.cc

@@ -4,7 +4,9 @@
 
 #include "third_party/blink/renderer/platform/heap/unified_heap_controller.h"
 
+#include "base/macros.h"
 #include "third_party/blink/public/common/features.h"
+#include "third_party/blink/renderer/platform/bindings/dom_wrapper_world.h"
 #include "third_party/blink/renderer/platform/bindings/script_forbidden_scope.h"
 #include "third_party/blink/renderer/platform/bindings/script_wrappable.h"
 #include "third_party/blink/renderer/platform/bindings/wrapper_type_info.h"
@@ -143,9 +145,7 @@ bool UnifiedHeapController::IsTracingDone() {
   return is_tracing_done_;
 }
 
-namespace {
-
-bool IsRootForNonTracingGCInternal(
+bool UnifiedHeapController::IsRootForNonTracingGC(
     const v8::TracedReference<v8::Value>& handle) {
   const uint16_t class_id = handle.WrapperClassId();
   // Stand-alone reference or kCustomWrappableId. Keep as root as
@@ -168,8 +168,6 @@ bool IsRootForNonTracingGCInternal(
   return false;
 }
 
-}  // namespace
-
 void UnifiedHeapController::ResetHandleInNonTracingGC(
     const v8::TracedReference<v8::Value>& handle) {
   const uint16_t class_id = handle.WrapperClassId();
@@ -180,12 +178,11 @@ void UnifiedHeapController::ResetHandleInNonTracingGC(
     return;
 
   const v8::TracedReference<v8::Object>& traced = handle.As<v8::Object>();
-  ToScriptWrappable(traced)->UnsetWrapperIfAny();
-}
-
-bool UnifiedHeapController::IsRootForNonTracingGC(
-    const v8::TracedReference<v8::Value>& handle) {
-  return IsRootForNonTracingGCInternal(handle);
+  bool success = DOMWrapperWorld::UnsetSpecificWrapperIfSet(
+      ToScriptWrappable(traced), traced);
+  // Since V8 found a handle, Blink needs to find it as well when trying to
+  // remove it.
+  CHECK(success);
 }
 
 bool UnifiedHeapController::IsRootForNonTracingGC(