Don't hash custom element names when looking up definitions.

Mapping a custom element constructor to its definition used to look in
a JavaScript map from constructor to custom element name string, and
then hash that name to look up the custom element definition in the
registry.

After this change the JavaScript map values are IDs so the definition
can be retrieved directly from a vector. This avoids marshaling the
name string from V8 to C++, and avoids hashing the string, to look up
a definition. (There's still a map from name to ID on the side so that
CustomElementRegistry.get can look up definitions by name, but it is
not used in common operations like creating a custom element.)

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

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

@@ -49,14 +49,14 @@ ScriptCustomElementDefinition* ScriptCustomElementDefinition::ForConstructor(
     const v8::Local<v8::Value>& constructor) {
   v8::Local<v8::Map> map =
       EnsureCustomElementRegistryMap(script_state, registry);
-  v8::Local<v8::Value> name_value =
+  v8::Local<v8::Value> id_value =
       map->Get(script_state->GetContext(), constructor).ToLocalChecked();
-  if (!name_value->IsString())
+  if (!id_value->IsUint32())
     return nullptr;
-  AtomicString name = ToCoreAtomicString(name_value.As<v8::String>());
+  uint32_t id = id_value.As<v8::Uint32>()->Value();
 
   // This downcast is safe because only
-  // ScriptCustomElementDefinitions have a name associated with a V8
+  // ScriptCustomElementDefinitions have an ID associated with a V8
   // constructor in the map; see
   // ScriptCustomElementDefinition::create. This relies on three
   // things:
@@ -65,10 +65,8 @@ ScriptCustomElementDefinition* ScriptCustomElementDefinition::ForConstructor(
   //    Audit the use of private properties in general and how the
   //    map is handled--it should never be leaked to script.
   //
-  // 2. CustomElementRegistry does not overwrite definitions with a
-  //    given name--see the CHECK in CustomElementRegistry::define
-  //    --and adds ScriptCustomElementDefinitions to the map without
-  //    fail.
+  // 2. CustomElementRegistry adds ScriptCustomElementDefinitions
+  //    assigned an ID to the lis tof definitions without fail.
   //
   // 3. The relationship between the CustomElementRegistry and its
   //    map is never mixed up; this is guaranteed by the bindings
@@ -79,7 +77,7 @@ ScriptCustomElementDefinition* ScriptCustomElementDefinition::ForConstructor(
   // currently only one implementation of CustomElementDefinition in
   // product code and that is ScriptCustomElementDefinition. But
   // that may change in the future.
-  CustomElementDefinition* definition = registry->DefinitionForName(name);
+  CustomElementDefinition* definition = registry->DefinitionForId(id);
   CHECK(definition);
   return static_cast<ScriptCustomElementDefinition*>(definition);
 }
@@ -88,6 +86,7 @@ ScriptCustomElementDefinition* ScriptCustomElementDefinition::Create(
     ScriptState* script_state,
     CustomElementRegistry* registry,
     const CustomElementDescriptor& descriptor,
+    CustomElementDefinition::Id id,
     const v8::Local<v8::Object>& constructor,
     const v8::Local<v8::Function>& connected_callback,
     const v8::Local<v8::Function>& disconnected_callback,
@@ -99,13 +98,12 @@ ScriptCustomElementDefinition* ScriptCustomElementDefinition::Create(
       disconnected_callback, adopted_callback, attribute_changed_callback,
       std::move(observed_attributes));
 
-  // Add a constructor -> name mapping to the registry.
-  v8::Local<v8::Value> name_value =
-      V8String(script_state->GetIsolate(), descriptor.GetName());
+  // Add a constructor -> ID mapping to the registry.
+  v8::Local<v8::Value> id_value =
+      v8::Integer::New(script_state->GetIsolate(), id);
   v8::Local<v8::Map> map =
       EnsureCustomElementRegistryMap(script_state, registry);
-  map->Set(script_state->GetContext(), constructor, name_value)
-      .ToLocalChecked();
+  map->Set(script_state->GetContext(), constructor, id_value).ToLocalChecked();
 
   return definition;
 }

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

@@ -32,6 +32,7 @@ class CORE_EXPORT ScriptCustomElementDefinition final
       ScriptState*,
       CustomElementRegistry*,
       const CustomElementDescriptor&,
+      CustomElementDefinition::Id,
       const v8::Local<v8::Object>& constructor,
       const v8::Local<v8::Function>& connected_callback,
       const v8::Local<v8::Function>& disconnected_callback,

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

@@ -90,7 +90,7 @@ bool ScriptCustomElementDefinitionBuilder::CallableForName(
   v8::Local<v8::Value> value;
   if (!ValueForName(prototype_, name, value))
     return false;
-  // "undefined" means "omitted", so return true.
+  // "undefined" means "omitted", which is valid.
   if (value->IsUndefined())
     return true;
   if (!value->IsFunction()) {
@@ -134,9 +134,10 @@ bool ScriptCustomElementDefinitionBuilder::RememberOriginalProperties() {
 }
 
 CustomElementDefinition* ScriptCustomElementDefinitionBuilder::Build(
-    const CustomElementDescriptor& descriptor) {
+    const CustomElementDescriptor& descriptor,
+    CustomElementDefinition::Id id) {
   return ScriptCustomElementDefinition::Create(
-      script_state_.Get(), registry_, descriptor, constructor_,
+      script_state_.Get(), registry_, descriptor, id, constructor_,
       connected_callback_, disconnected_callback_, adopted_callback_,
       attribute_changed_callback_, std::move(observed_attributes_));
 }

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

@@ -41,7 +41,8 @@ class CORE_EXPORT ScriptCustomElementDefinitionBuilder
   bool CheckConstructorNotRegistered() override;
   bool CheckPrototype() override;
   bool RememberOriginalProperties() override;
-  CustomElementDefinition* Build(const CustomElementDescriptor&) override;
+  CustomElementDefinition* Build(const CustomElementDescriptor&,
+                                 CustomElementDefinition::Id) override;
 
  private:
   static ScriptCustomElementDefinitionBuilder* stack_;

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

@@ -29,6 +29,10 @@ class CORE_EXPORT CustomElementDefinition
   WTF_MAKE_NONCOPYABLE(CustomElementDefinition);
 
  public:
+  // Each definition has an ID that is unique within the
+  // CustomElementRegistry that created it.
+  using Id = uint32_t;
+
   CustomElementDefinition(const CustomElementDescriptor&);
   CustomElementDefinition(const CustomElementDescriptor&,
                           const HashSet<AtomicString>&);

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

@@ -6,12 +6,12 @@
 #define CustomElementDefinitionBuilder_h
 
 #include "core/CoreExport.h"
+#include "core/dom/custom/CustomElementDefinition.h"
 #include "platform/wtf/Allocator.h"
 #include "platform/wtf/Noncopyable.h"
 
 namespace blink {
 
-class CustomElementDefinition;
 class CustomElementDescriptor;
 class CustomElementRegistry;
 
@@ -46,7 +46,8 @@ class CORE_EXPORT CustomElementDefinitionBuilder {
   virtual bool RememberOriginalProperties() = 0;
 
   // Produce the definition. This must produce a definition.
-  virtual CustomElementDefinition* Build(const CustomElementDescriptor&) = 0;
+  virtual CustomElementDefinition* Build(const CustomElementDescriptor&,
+                                         CustomElementDefinition::Id) = 0;
 };
 
 }  // namespace blink

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

@@ -26,6 +26,8 @@
 #include "platform/instrumentation/tracing/TraceEvent.h"
 #include "platform/wtf/Allocator.h"
 
+#include <limits>
+
 namespace blink {
 
 // Returns true if |name| is invalid.
@@ -93,7 +95,7 @@ DEFINE_TRACE(CustomElementRegistry) {
 
 DEFINE_TRACE_WRAPPERS(CustomElementRegistry) {
   visitor->TraceWrappers(&CustomElementReactionStack::Current());
-  for (auto definition : definitions_.Values())
+  for (auto definition : definitions_)
     visitor->TraceWrappers(definition);
 }
 
@@ -185,12 +187,16 @@ CustomElementDefinition* CustomElementRegistry::define(
   }
 
   CustomElementDescriptor descriptor(name, local_name);
-  CustomElementDefinition* definition = builder.Build(descriptor);
+  if (UNLIKELY(definitions_.size() >=
+               std::numeric_limits<CustomElementDefinition::Id>::max()))
+    return nullptr;
+  CustomElementDefinition::Id id = definitions_.size() + 1;
+  CustomElementDefinition* definition = builder.Build(descriptor, id);
   CHECK(!exception_state.HadException());
   CHECK(definition->Descriptor() == descriptor);
-  DefinitionMap::AddResult result = definitions_.insert(
-      descriptor.GetName(),
+  definitions_.emplace_back(
       TraceWrapperMember<CustomElementDefinition>(this, definition));
+  NameIdMap::AddResult result = name_id_map_.insert(descriptor.GetName(), id);
   CHECK(result.is_new_entry);
 
   HeapVector<Member<Element>> candidates;
@@ -237,7 +243,7 @@ CustomElementDefinition* CustomElementRegistry::DefinitionFor(
 }
 
 bool CustomElementRegistry::NameIsDefined(const AtomicString& name) const {
-  return definitions_.Contains(name);
+  return name_id_map_.Contains(name);
 }
 
 void CustomElementRegistry::Entangle(V0CustomElementRegistrationContext* v0) {
@@ -255,7 +261,12 @@ bool CustomElementRegistry::V0NameIsDefined(const AtomicString& name) {
 
 CustomElementDefinition* CustomElementRegistry::DefinitionForName(
     const AtomicString& name) const {
-  return definitions_.at(name);
+  return DefinitionForId(name_id_map_.at(name));
+}
+
+CustomElementDefinition* CustomElementRegistry::DefinitionForId(
+    CustomElementDefinition::Id id) const {
+  return id ? definitions_[id - 1].Get() : nullptr;
 }
 
 void CustomElementRegistry::AddCandidate(Element* candidate) {

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

@@ -18,7 +18,6 @@
 
 namespace blink {
 
-class CustomElementDefinition;
 class CustomElementDefinitionBuilder;
 class CustomElementDescriptor;
 class Element;
@@ -55,6 +54,7 @@ class CORE_EXPORT CustomElementRegistry final
   ScriptValue get(const AtomicString& name);
   bool NameIsDefined(const AtomicString& name) const;
   CustomElementDefinition* DefinitionForName(const AtomicString& name) const;
+  CustomElementDefinition* DefinitionForId(CustomElementDefinition::Id) const;
 
   // TODO(dominicc): Switch most callers of definitionForName to
   // definitionFor when implementing type extensions.
@@ -85,9 +85,12 @@ class CORE_EXPORT CustomElementRegistry final
   class ElementDefinitionIsRunning;
   bool element_definition_is_running_;
 
-  using DefinitionMap =
-      HeapHashMap<AtomicString, TraceWrapperMember<CustomElementDefinition>>;
-  DefinitionMap definitions_;
+  using DefinitionList =
+      HeapVector<TraceWrapperMember<CustomElementDefinition>>;
+  DefinitionList definitions_;
+
+  using NameIdMap = HashMap<AtomicString, size_t>;
+  NameIdMap name_id_map_;
 
   Member<const LocalDOMWindow> owner_;
 

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

@@ -265,8 +265,8 @@ class LogUpgradeBuilder final : public TestCustomElementDefinitionBuilder {
  public:
   LogUpgradeBuilder() {}
 
-  CustomElementDefinition* Build(
-      const CustomElementDescriptor& descriptor) override {
+  CustomElementDefinition* Build(const CustomElementDescriptor& descriptor,
+                                 CustomElementDefinition::Id) override {
     return new LogUpgradeDefinition(descriptor);
   }
 };

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

@@ -7,7 +7,8 @@
 namespace blink {
 
 CustomElementDefinition* TestCustomElementDefinitionBuilder::Build(
-    const CustomElementDescriptor& descriptor) {
+    const CustomElementDescriptor& descriptor,
+    CustomElementDefinition::Id) {
   return new TestCustomElementDefinition(descriptor);
 }
 

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

@@ -35,7 +35,8 @@ class TestCustomElementDefinitionBuilder
   bool CheckConstructorNotRegistered() override { return true; }
   bool CheckPrototype() override { return true; }
   bool RememberOriginalProperties() override { return true; }
-  CustomElementDefinition* Build(const CustomElementDescriptor&) override;
+  CustomElementDefinition* Build(const CustomElementDescriptor&,
+                                 CustomElementDefinition::Id) override;
 };
 
 class TestCustomElementDefinition : public CustomElementDefinition {