Keep custom element callbacks alive with wrapper tracing.

Bug: 710184
Change-Id: I3d1d506888657c5bd60a114609247c5f92c0edd7
Reviewed-on: https://chromium-review.googlesource.com/520443
Commit-Queue: Dominic Cooney <dominicc@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#476237}

third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp

@@ -84,23 +84,6 @@ ScriptCustomElementDefinition* ScriptCustomElementDefinition::ForConstructor(
   return static_cast<ScriptCustomElementDefinition*>(definition);
 }
 
-using SymbolGetter = V8PrivateProperty::Symbol (*)(v8::Isolate*);
-
-template <typename T>
-static void KeepAlive(v8::Local<v8::Object>& object,
-                      SymbolGetter symbol_getter,
-                      const v8::Local<T>& value,
-                      ScopedPersistent<T>& persistent,
-                      ScriptState* script_state) {
-  if (value.IsEmpty())
-    return;
-
-  v8::Isolate* isolate = script_state->GetIsolate();
-  symbol_getter(isolate).Set(object, value);
-  persistent.Set(isolate, value);
-  persistent.SetPhantom();
-}
-
 ScriptCustomElementDefinition* ScriptCustomElementDefinition::Create(
     ScriptState* script_state,
     CustomElementRegistry* registry,
@@ -123,22 +106,6 @@ ScriptCustomElementDefinition* ScriptCustomElementDefinition::Create(
       EnsureCustomElementRegistryMap(script_state, registry);
   map->Set(script_state->GetContext(), constructor, name_value)
       .ToLocalChecked();
-  definition->constructor_.SetPhantom();
-
-  // We add the callbacks here to keep them alive. We use the name as
-  // the key because it is unique per-registry.
-  v8::Local<v8::Object> object = v8::Object::New(script_state->GetIsolate());
-  KeepAlive(object, V8PrivateProperty::GetCustomElementConnectedCallback,
-            connected_callback, definition->connected_callback_, script_state);
-  KeepAlive(object, V8PrivateProperty::GetCustomElementDisconnectedCallback,
-            disconnected_callback, definition->disconnected_callback_,
-            script_state);
-  KeepAlive(object, V8PrivateProperty::GetCustomElementAdoptedCallback,
-            adopted_callback, definition->adopted_callback_, script_state);
-  KeepAlive(object, V8PrivateProperty::GetCustomElementAttributeChangedCallback,
-            attribute_changed_callback, definition->attribute_changed_callback_,
-            script_state);
-  map->Set(script_state->GetContext(), name_value, object).ToLocalChecked();
 
   return definition;
 }
@@ -154,7 +121,29 @@ ScriptCustomElementDefinition::ScriptCustomElementDefinition(
     HashSet<AtomicString>&& observed_attributes)
     : CustomElementDefinition(descriptor, std::move(observed_attributes)),
       script_state_(script_state),
-      constructor_(script_state->GetIsolate(), constructor) {}
+      constructor_(script_state->GetIsolate(), this, constructor),
+      connected_callback_(this),
+      disconnected_callback_(this),
+      adopted_callback_(this),
+      attribute_changed_callback_(this) {
+  v8::Isolate* isolate = script_state->GetIsolate();
+  if (!connected_callback.IsEmpty())
+    connected_callback_.Set(isolate, connected_callback);
+  if (!disconnected_callback.IsEmpty())
+    disconnected_callback_.Set(isolate, disconnected_callback);
+  if (!adopted_callback.IsEmpty())
+    adopted_callback_.Set(isolate, adopted_callback);
+  if (!attribute_changed_callback.IsEmpty())
+    attribute_changed_callback_.Set(isolate, attribute_changed_callback);
+}
+
+DEFINE_TRACE_WRAPPERS(ScriptCustomElementDefinition) {
+  visitor->TraceWrappers(constructor_.Cast<v8::Value>());
+  visitor->TraceWrappers(connected_callback_.Cast<v8::Value>());
+  visitor->TraceWrappers(disconnected_callback_.Cast<v8::Value>());
+  visitor->TraceWrappers(adopted_callback_.Cast<v8::Value>());
+  visitor->TraceWrappers(attribute_changed_callback_.Cast<v8::Value>());
+}
 
 static void DispatchErrorEvent(v8::Isolate* isolate,
                                v8::Local<v8::Value> exception,

third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.h

@@ -7,8 +7,8 @@
 
 #include "core/CoreExport.h"
 #include "core/dom/custom/CustomElementDefinition.h"
-#include "platform/bindings/ScopedPersistent.h"
 #include "platform/bindings/ScriptState.h"
+#include "platform/bindings/TraceWrapperV8Reference.h"
 #include "platform/wtf/Noncopyable.h"
 #include "platform/wtf/RefPtr.h"
 #include "v8.h"
@@ -41,6 +41,8 @@ class CORE_EXPORT ScriptCustomElementDefinition final
 
   virtual ~ScriptCustomElementDefinition() = default;
 
+  DECLARE_VIRTUAL_TRACE_WRAPPERS();
+
   v8::Local<v8::Object> Constructor() const;
 
   HTMLElement* CreateElementSync(Document&, const QualifiedName&) override;
@@ -88,11 +90,11 @@ class CORE_EXPORT ScriptCustomElementDefinition final
                                                 ExceptionState&);
 
   RefPtr<ScriptState> script_state_;
-  ScopedPersistent<v8::Object> constructor_;
-  ScopedPersistent<v8::Function> connected_callback_;
-  ScopedPersistent<v8::Function> disconnected_callback_;
-  ScopedPersistent<v8::Function> adopted_callback_;
-  ScopedPersistent<v8::Function> attribute_changed_callback_;
+  TraceWrapperV8Reference<v8::Object> constructor_;
+  TraceWrapperV8Reference<v8::Function> connected_callback_;
+  TraceWrapperV8Reference<v8::Function> disconnected_callback_;
+  TraceWrapperV8Reference<v8::Function> adopted_callback_;
+  TraceWrapperV8Reference<v8::Function> attribute_changed_callback_;
 };
 
 }  // namespace blink

third_party/WebKit/Source/core/dom/custom/CustomElementDefinition.h

@@ -8,6 +8,7 @@
 #include "bindings/core/v8/ScriptValue.h"
 #include "core/CoreExport.h"
 #include "core/dom/custom/CustomElementDescriptor.h"
+#include "platform/bindings/ScriptWrappable.h"  // For TraceWrapperBase
 #include "platform/heap/Handle.h"
 #include "platform/wtf/HashSet.h"
 #include "platform/wtf/Noncopyable.h"
@@ -23,7 +24,8 @@ class HTMLElement;
 class QualifiedName;
 
 class CORE_EXPORT CustomElementDefinition
-    : public GarbageCollectedFinalized<CustomElementDefinition> {
+    : public GarbageCollectedFinalized<CustomElementDefinition>,
+      public TraceWrapperBase {
   WTF_MAKE_NONCOPYABLE(CustomElementDefinition);
 
  public:
@@ -33,6 +35,7 @@ class CORE_EXPORT CustomElementDefinition
   virtual ~CustomElementDefinition();
 
   DECLARE_VIRTUAL_TRACE();
+  DECLARE_VIRTUAL_TRACE_WRAPPERS() {}
 
   const CustomElementDescriptor& Descriptor() { return descriptor_; }
 

third_party/WebKit/Source/core/dom/custom/CustomElementRegistry.cpp

@@ -93,6 +93,8 @@ DEFINE_TRACE(CustomElementRegistry) {
 
 DEFINE_TRACE_WRAPPERS(CustomElementRegistry) {
   visitor->TraceWrappers(&CustomElementReactionStack::Current());
+  for (auto definition : definitions_.Values())
+    visitor->TraceWrappers(definition);
 }
 
 CustomElementDefinition* CustomElementRegistry::define(
@@ -186,8 +188,9 @@ CustomElementDefinition* CustomElementRegistry::define(
   CustomElementDefinition* definition = builder.Build(descriptor);
   CHECK(!exception_state.HadException());
   CHECK(definition->Descriptor() == descriptor);
-  DefinitionMap::AddResult result =
-      definitions_.insert(descriptor.GetName(), definition);
+  DefinitionMap::AddResult result = definitions_.insert(
+      descriptor.GetName(),
+      TraceWrapperMember<CustomElementDefinition>(this, definition));
   CHECK(result.is_new_entry);
 
   HeapVector<Member<Element>> candidates;

third_party/WebKit/Source/core/dom/custom/CustomElementRegistry.h

@@ -8,7 +8,9 @@
 #include "base/gtest_prod_util.h"
 #include "bindings/core/v8/ScriptPromise.h"
 #include "core/CoreExport.h"
+#include "core/dom/custom/CustomElementDefinition.h"
 #include "platform/bindings/ScriptWrappable.h"
+#include "platform/bindings/TraceWrapperMember.h"
 #include "platform/heap/Handle.h"
 #include "platform/wtf/Noncopyable.h"
 #include "platform/wtf/text/AtomicString.h"
@@ -84,7 +86,7 @@ class CORE_EXPORT CustomElementRegistry final
   bool element_definition_is_running_;
 
   using DefinitionMap =
-      HeapHashMap<AtomicString, Member<CustomElementDefinition>>;
+      HeapHashMap<AtomicString, TraceWrapperMember<CustomElementDefinition>>;
   DefinitionMap definitions_;
 
   Member<const LocalDOMWindow> owner_;

third_party/WebKit/Source/platform/bindings/V8PrivateProperty.h

@@ -24,10 +24,6 @@ class ScriptWrappable;
 // FetchEvent.Request.
 // Apply |X| for each pair of (InterfaceName, PrivateKeyName).
 #define V8_PRIVATE_PROPERTY_FOR_EACH(X)               \
-  X(CustomElement, AdoptedCallback)                   \
-  X(CustomElement, AttributeChangedCallback)          \
-  X(CustomElement, ConnectedCallback)                 \
-  X(CustomElement, DisconnectedCallback)              \
   X(CustomElement, Document)                          \
   X(CustomElement, IsInterfacePrototypeObject)        \
   X(CustomElement, NamespaceURI)                      \