Replace the old GetSymbol which gets Symbol for [CachedAttribute] attributes with the new API

The new API in V8PrivateProperty to get Symbol was created in CL1810511.

The new API needs a key for uniquely identifying v8::Private as an argument and we need to
use the same key for each CachedAttribute in AttributeGetter and AttributeSetter.

There are two types of CachedAttribute. One has a setter and the other has no setter.
Since a CachedAttribute that has no setter only needs to define the key in the getter function, this CL divided the location of the key definition depending on whether or not CachedAttribute has a setter.

Bug: 715418
Change-Id: I7d994d4f804e22c5221463067a6957052808d4f6
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1855151
Commit-Queue: Marina Sakai <marinasakai@google.com>
Reviewed-by: Hitoshi Yoshida <peria@chromium.org>
Reviewed-by: Yuki Shiino <yukishiino@chromium.org>
Cr-Commit-Position: refs/heads/master@{#707723}

third_party/blink/renderer/bindings/scripts/v8_attributes.py

@@ -49,6 +49,7 @@ import v8_utilities
 from v8_utilities import (cpp_name_or_partial, capitalize, cpp_name, has_extended_attribute,
                           has_extended_attribute_value, scoped_name, strip_suffix,
                           uncapitalize, extended_attribute_value_as_list, is_unforgeable)
+from blinkbuild.name_style_converter import NameStyleConverter
 
 
 def attribute_context(interface, attribute, interfaces, component_info):
@@ -109,17 +110,50 @@ def attribute_context(interface, attribute, interfaces, component_info):
     is_save_same_object = (
         'SameObject' in attribute.extended_attributes and
         'SaveSameObject' in attribute.extended_attributes)
-    if is_save_same_object:
-        includes.add('platform/bindings/v8_private_property.h')
 
+    # [CachedAccessor]
+    is_cached_accessor = 'CachedAccessor' in extended_attributes
+
+    # [LenientSetter]
+    is_lenient_setter = 'LenientSetter' in extended_attributes
+
+    # [CachedAttribute]
     cached_attribute_validation_method = extended_attributes.get('CachedAttribute')
+
     keep_alive_for_gc = is_keep_alive_for_gc(interface, attribute)
-    if cached_attribute_validation_method or keep_alive_for_gc:
-        includes.add('platform/bindings/v8_private_property.h')
 
-    # [CachedAccessor]
-    is_cached_accessor = 'CachedAccessor' in extended_attributes
-    if is_cached_accessor:
+    does_generate_getter = (
+        not has_custom_getter(attribute) and
+        not constructor_type
+    )
+    does_generate_setter = (
+        has_setter(interface, attribute) and
+        not (has_custom_setter(attribute) or is_lenient_setter)
+    )
+
+    use_private_property_in_getter = (
+        does_generate_getter and (
+            cached_attribute_validation_method or
+            is_save_same_object or
+            keep_alive_for_gc
+        )
+    )
+    use_private_property_in_setter = (
+        does_generate_setter and
+        cached_attribute_validation_method
+    )
+    private_property_is_shared_between_getter_and_setter = (
+        use_private_property_in_getter and
+        use_private_property_in_setter
+    )
+    private_property_key_name = create_private_property_key_name(attribute)
+
+    does_use_private_property = (
+        use_private_property_in_getter or
+        use_private_property_in_setter or
+        is_cached_accessor
+    )
+    if does_use_private_property:
         includes.add('platform/bindings/v8_private_property.h')
 
     # [LogActivity]
@@ -143,6 +177,7 @@ def attribute_context(interface, attribute, interfaces, component_info):
         'activity_logging_world_list_for_getter': v8_utilities.activity_logging_world_list(attribute, 'Getter'),  # [ActivityLogging]
         'activity_logging_world_list_for_setter': v8_utilities.activity_logging_world_list(attribute, 'Setter'),  # [ActivityLogging]
         'activity_logging_world_check': v8_utilities.activity_logging_world_check(attribute),  # [ActivityLogging]
+        'cached_accessor_name': '%s%sCachedAccessor' % (interface.name, attribute.name.capitalize()),
         'cached_attribute_validation_method': cached_attribute_validation_method,
         'camel_case_name': NameStyleConverter(attribute.name).to_upper_camel_case(),
         'constructor_type': constructor_type,
@@ -150,8 +185,9 @@ def attribute_context(interface, attribute, interfaces, component_info):
         'cpp_name': cpp_name(attribute),
         'cpp_type': idl_type.cpp_type,
         'cpp_type_initializer': idl_type.cpp_type_initializer,
-        'high_entropy': high_entropy,
         'deprecate_as': deprecate_as,
+        'does_generate_getter': does_generate_getter,
+        'does_generate_setter': does_generate_setter,
         'enum_type': idl_type.enum_type,
         'enum_values': idl_type.enum_values,
         'exposed_test': v8_utilities.exposed(attribute, interface),  # [Exposed]
@@ -164,6 +200,7 @@ def attribute_context(interface, attribute, interfaces, component_info):
         'has_custom_setter': has_custom_setter(attribute),
         'has_promise_type': idl_type.name == 'Promise',
         'has_setter': has_setter(interface, attribute),
+        'high_entropy': high_entropy,
         'idl_type': str(idl_type),
         'is_cached_accessor': is_cached_accessor,
         'is_call_with_execution_context': has_extended_attribute_value(attribute, 'CallWith', 'ExecutionContext'),
@@ -179,7 +216,7 @@ def attribute_context(interface, attribute, interfaces, component_info):
             extended_attributes['RaisesException'] in (None, 'Getter'),
         'is_keep_alive_for_gc': keep_alive_for_gc,
         'is_lazy_data_attribute': is_lazy_data_attribute,
-        'is_lenient_setter': 'LenientSetter' in extended_attributes,
+        'is_lenient_setter': is_lenient_setter,
         'is_lenient_this': 'LenientThis' in extended_attributes,
         'is_nullable': idl_type.is_nullable,
         'is_explicit_nullable': idl_type.is_explicit_nullable,
@@ -195,14 +232,15 @@ def attribute_context(interface, attribute, interfaces, component_info):
         'is_static': attribute.is_static,
         'is_url': 'URL' in extended_attributes,
         'is_unforgeable': is_unforgeable(attribute),
+        'measure_as': measure_as,
+        'name': attribute.name,
         'on_instance': v8_utilities.on_instance(interface, attribute),
         'on_interface': v8_utilities.on_interface(interface, attribute),
         'on_prototype': v8_utilities.on_prototype(interface, attribute),
         'origin_trial_feature_name':
             v8_utilities.origin_trial_feature_name(attribute, runtime_features),  # [RuntimeEnabled] for origin trial
-        'use_output_parameter_for_result': idl_type.use_output_parameter_for_result,
-        'measure_as': measure_as,
-        'name': attribute.name,
+        'private_property_is_shared_between_getter_and_setter': private_property_is_shared_between_getter_and_setter,
+        'private_property_key_name': private_property_key_name,
         'property_attributes': property_attributes(interface, attribute),
         'reflect_empty': extended_attributes.get('ReflectEmpty'),
         'reflect_invalid': extended_attributes.get('ReflectInvalid', ''),
@@ -211,7 +249,7 @@ def attribute_context(interface, attribute, interfaces, component_info):
         'runtime_enabled_feature_name':
             v8_utilities.runtime_enabled_feature_name(attribute, runtime_features),  # [RuntimeEnabled] if not in origin trial
         'secure_context_test': v8_utilities.secure_context(attribute, interface),  # [SecureContext]
-        'cached_accessor_name': '%s%sCachedAccessor' % (interface.name, attribute.name.capitalize()),
+        'use_output_parameter_for_result': idl_type.use_output_parameter_for_result,
         'world_suffixes': (
             ['', 'ForMainWorld']
             if 'PerWorldBindings' in extended_attributes
@@ -239,6 +277,10 @@ def attribute_context(interface, attribute, interfaces, component_info):
     return context
 
 
+def create_private_property_key_name(attribute):
+    return NameStyleConverter(attribute.name + '_private_property_key').to_snake_case()
+
+
 def runtime_call_stats_context(interface, attribute, context):
     includes.add('platform/bindings/runtime_call_stats.h')
     generic_counter_name = 'Blink_' + v8_utilities.cpp_name(interface) + '_' + attribute.name

third_party/blink/renderer/bindings/templates/attributes.cc.tmpl

@@ -79,8 +79,13 @@ const v8::FunctionCallbackInfo<v8::Value>& info
   V8PrivateProperty::Symbol property_symbol =
       V8PrivateProperty::GetHistoryStateSymbol(info.GetIsolate());
   {% else %}
+  {% if not attribute.private_property_is_shared_between_getter_and_setter %}
+  static int {{attribute.private_property_key_name}};
+
+  {% endif %}
   V8PrivateProperty::Symbol property_symbol =
       V8PrivateProperty::GetSymbol(info.GetIsolate(),
+          &{{attribute.private_property_key_name}},
           "{{cpp_class}}#{{attribute.camel_case_name}}");
   {% endif %}
   if (!static_cast<const {{cpp_class}}*>(impl)->{{attribute.cached_attribute_validation_method}}()) {
@@ -472,7 +477,9 @@ static void {{attribute.camel_case_name}}AttributeSetter{{world_suffix}}(
   // [CachedAttribute]
   // Invalidate the cached value.
   V8PrivateProperty::GetSymbol(
-      isolate, "{{cpp_class}}#{{attribute.camel_case_name}}")
+      isolate,
+      &{{attribute.private_property_key_name}},
+      "{{cpp_class}}#{{attribute.camel_case_name}}")
       .DeleteProperty(holder, v8::Undefined(isolate));
   {% endif %}
   {% endif %}{# attribute.is_put_forwards #}

third_party/blink/renderer/bindings/templates/interface_base.cc.tmpl

@@ -86,15 +86,19 @@ static void (*{{method.name}}MethodForPartialInterface)(const v8::FunctionCallba
 
 {##############################################################################}
 {# Attributes #}
-{% from 'attributes.cc.tmpl' import attribute_getter,
-       attribute_setter,
+{% from 'attributes.cc.tmpl' import attribute_getter, attribute_setter,
        with context %}
 {% for attribute in attributes %}
 {% for world_suffix in attribute.world_suffixes %}
-{% if not attribute.has_custom_getter and not attribute.constructor_type %}
+{% if attribute.private_property_is_shared_between_getter_and_setter %}
+// Define a private property key shared between getter and setter.
+static int {{attribute.private_property_key_name}};
+{% endif %}
+
+{% if attribute.does_generate_getter %}
 {{attribute_getter(attribute, world_suffix)}}
 {% endif %}
-{% if attribute.has_setter and not (attribute.has_custom_setter or attribute.is_lenient_setter) %}
+{% if attribute.does_generate_setter %}
 {{attribute_setter(attribute, world_suffix)}}
 {% endif %}
 {% endfor %}

third_party/blink/renderer/bindings/tests/idls/core/test_object.idl

@@ -135,6 +135,7 @@ interface TestObject {
     [LogActivity=SetterOnly, LogAllWorlds] attribute long activityLoggingSetterForAllWorldsLongAttribute;
     [CachedAttribute=isValueDirty] attribute any cachedAttributeAnyAttribute;
     [CachedAttribute=isFrozenArrayDirty] attribute FrozenArray<DOMString> cachedArrayAttribute;
+    [CachedAttribute=isStringDirty] readonly attribute DOMString readonlyCachedAttribute;
     [CachedAttribute=isStringDirty] attribute DOMString? cachedStringOrNoneAttribute;
     [CallWith=ExecutionContext] attribute any callWithExecutionContextAnyAttribute;
     [CallWith=ScriptState] attribute any callWithScriptStateAnyAttribute;

third_party/blink/renderer/bindings/tests/results/core/v8_test_object.cc

@@ -1992,6 +1992,9 @@ static void ActivityLoggingSetterForAllWorldsLongAttributeAttributeSetter(
   impl->setActivityLoggingSetterForAllWorldsLongAttribute(cpp_value);
 }
 
+// Define a private property key shared between getter and setter.
+static int cached_attribute_any_attribute_private_property_key;
+
 static void CachedAttributeAnyAttributeAttributeGetter(const v8::FunctionCallbackInfo<v8::Value>& info) {
   v8::Local<v8::Object> holder = info.Holder();
 
@@ -2000,6 +2003,7 @@ static void CachedAttributeAnyAttributeAttributeGetter(const v8::FunctionCallbac
   // [CachedAttribute]
   V8PrivateProperty::Symbol property_symbol =
       V8PrivateProperty::GetSymbol(info.GetIsolate(),
+          &cached_attribute_any_attribute_private_property_key,
           "TestObject#CachedAttributeAnyAttribute");
   if (!static_cast<const TestObject*>(impl)->isValueDirty()) {
     v8::Local<v8::Value> v8_value;
@@ -2036,10 +2040,15 @@ static void CachedAttributeAnyAttributeAttributeSetter(
   // [CachedAttribute]
   // Invalidate the cached value.
   V8PrivateProperty::GetSymbol(
-      isolate, "TestObject#CachedAttributeAnyAttribute")
+      isolate,
+      &cached_attribute_any_attribute_private_property_key,
+      "TestObject#CachedAttributeAnyAttribute")
       .DeleteProperty(holder, v8::Undefined(isolate));
 }
 
+// Define a private property key shared between getter and setter.
+static int cached_array_attribute_private_property_key;
+
 static void CachedArrayAttributeAttributeGetter(const v8::FunctionCallbackInfo<v8::Value>& info) {
   v8::Local<v8::Object> holder = info.Holder();
 
@@ -2048,6 +2057,7 @@ static void CachedArrayAttributeAttributeGetter(const v8::FunctionCallbackInfo<v
   // [CachedAttribute]
   V8PrivateProperty::Symbol property_symbol =
       V8PrivateProperty::GetSymbol(info.GetIsolate(),
+          &cached_array_attribute_private_property_key,
           "TestObject#CachedArrayAttribute");
   if (!static_cast<const TestObject*>(impl)->isFrozenArrayDirty()) {
     v8::Local<v8::Value> v8_value;
@@ -2088,10 +2098,44 @@ static void CachedArrayAttributeAttributeSetter(
   // [CachedAttribute]
   // Invalidate the cached value.
   V8PrivateProperty::GetSymbol(
-      isolate, "TestObject#CachedArrayAttribute")
+      isolate,
+      &cached_array_attribute_private_property_key,
+      "TestObject#CachedArrayAttribute")
       .DeleteProperty(holder, v8::Undefined(isolate));
 }
 
+static void ReadonlyCachedAttributeAttributeGetter(const v8::FunctionCallbackInfo<v8::Value>& info) {
+  v8::Local<v8::Object> holder = info.Holder();
+
+  TestObject* impl = V8TestObject::ToImpl(holder);
+
+  // [CachedAttribute]
+  static int readonly_cached_attribute_private_property_key;
+
+  V8PrivateProperty::Symbol property_symbol =
+      V8PrivateProperty::GetSymbol(info.GetIsolate(),
+          &readonly_cached_attribute_private_property_key,
+          "TestObject#ReadonlyCachedAttribute");
+  if (!static_cast<const TestObject*>(impl)->isStringDirty()) {
+    v8::Local<v8::Value> v8_value;
+    if (property_symbol.GetOrUndefined(holder).ToLocal(&v8_value) && !v8_value->IsUndefined()) {
+      V8SetReturnValue(info, v8_value);
+      return;
+    }
+  }
+
+  String cpp_value(impl->readonlyCachedAttribute());
+
+  // [CachedAttribute]
+  v8::Local<v8::Value> v8_value(V8String(info.GetIsolate(), cpp_value));
+  property_symbol.Set(holder, v8_value);
+
+  V8SetReturnValue(info, v8_value);
+}
+
+// Define a private property key shared between getter and setter.
+static int cached_string_or_none_attribute_private_property_key;
+
 static void CachedStringOrNoneAttributeAttributeGetter(const v8::FunctionCallbackInfo<v8::Value>& info) {
   v8::Local<v8::Object> holder = info.Holder();
 
@@ -2100,6 +2144,7 @@ static void CachedStringOrNoneAttributeAttributeGetter(const v8::FunctionCallbac
   // [CachedAttribute]
   V8PrivateProperty::Symbol property_symbol =
       V8PrivateProperty::GetSymbol(info.GetIsolate(),
+          &cached_string_or_none_attribute_private_property_key,
           "TestObject#CachedStringOrNoneAttribute");
   if (!static_cast<const TestObject*>(impl)->isStringDirty()) {
     v8::Local<v8::Value> v8_value;
@@ -2138,7 +2183,9 @@ static void CachedStringOrNoneAttributeAttributeSetter(
   // [CachedAttribute]
   // Invalidate the cached value.
   V8PrivateProperty::GetSymbol(
-      isolate, "TestObject#CachedStringOrNoneAttribute")
+      isolate,
+      &cached_string_or_none_attribute_private_property_key,
+      "TestObject#CachedStringOrNoneAttribute")
       .DeleteProperty(holder, v8::Undefined(isolate));
 }
 
@@ -3100,6 +3147,9 @@ static void RaisesExceptionTestInterfaceEmptyAttributeAttributeSetter(
   impl->setRaisesExceptionTestInterfaceEmptyAttribute(cpp_value, exception_state);
 }
 
+// Define a private property key shared between getter and setter.
+static int cached_attribute_raises_exception_getter_any_attribute_private_property_key;
+
 static void CachedAttributeRaisesExceptionGetterAnyAttributeAttributeGetter(const v8::FunctionCallbackInfo<v8::Value>& info) {
   v8::Local<v8::Object> holder = info.Holder();
 
@@ -3108,6 +3158,7 @@ static void CachedAttributeRaisesExceptionGetterAnyAttributeAttributeGetter(cons
   // [CachedAttribute]
   V8PrivateProperty::Symbol property_symbol =
       V8PrivateProperty::GetSymbol(info.GetIsolate(),
+          &cached_attribute_raises_exception_getter_any_attribute_private_property_key,
           "TestObject#CachedAttributeRaisesExceptionGetterAnyAttribute");
   if (!static_cast<const TestObject*>(impl)->isValueDirty()) {
     v8::Local<v8::Value> v8_value;
@@ -3151,7 +3202,9 @@ static void CachedAttributeRaisesExceptionGetterAnyAttributeAttributeSetter(
   // [CachedAttribute]
   // Invalidate the cached value.
   V8PrivateProperty::GetSymbol(
-      isolate, "TestObject#CachedAttributeRaisesExceptionGetterAnyAttribute")
+      isolate,
+      &cached_attribute_raises_exception_getter_any_attribute_private_property_key,
+      "TestObject#CachedAttributeRaisesExceptionGetterAnyAttribute")
       .DeleteProperty(holder, v8::Undefined(isolate));
 }
 
@@ -10283,6 +10336,12 @@ void V8TestObject::CachedArrayAttributeAttributeSetterCallback(
   test_object_v8_internal::CachedArrayAttributeAttributeSetter(v8_value, info);
 }
 
+void V8TestObject::ReadonlyCachedAttributeAttributeGetterCallback(const v8::FunctionCallbackInfo<v8::Value>& info) {
+  RUNTIME_CALL_TIMER_SCOPE_DISABLED_BY_DEFAULT(info.GetIsolate(), "Blink_TestObject_readonlyCachedAttribute_Getter");
+
+  test_object_v8_internal::ReadonlyCachedAttributeAttributeGetter(info);
+}
+
 void V8TestObject::CachedStringOrNoneAttributeAttributeGetterCallback(const v8::FunctionCallbackInfo<v8::Value>& info) {
   RUNTIME_CALL_TIMER_SCOPE_DISABLED_BY_DEFAULT(info.GetIsolate(), "Blink_TestObject_cachedStringOrNoneAttribute_Getter");
 
@@ -13346,6 +13405,7 @@ static void InstallV8TestObjectTemplate(
       { "activityLoggingSetterForAllWorldsLongAttribute", V8TestObject::ActivityLoggingSetterForAllWorldsLongAttributeAttributeGetterCallback, V8TestObject::ActivityLoggingSetterForAllWorldsLongAttributeAttributeSetterCallback, V8PrivateProperty::kNoCachedAccessor, static_cast<v8::PropertyAttribute>(v8::None), V8DOMConfiguration::kOnPrototype, V8DOMConfiguration::kCheckHolder, V8DOMConfiguration::kHasSideEffect, V8DOMConfiguration::kAlwaysCallGetter, V8DOMConfiguration::kAllWorlds },
       { "cachedAttributeAnyAttribute", V8TestObject::CachedAttributeAnyAttributeAttributeGetterCallback, V8TestObject::CachedAttributeAnyAttributeAttributeSetterCallback, V8PrivateProperty::kNoCachedAccessor, static_cast<v8::PropertyAttribute>(v8::None), V8DOMConfiguration::kOnPrototype, V8DOMConfiguration::kCheckHolder, V8DOMConfiguration::kHasSideEffect, V8DOMConfiguration::kAlwaysCallGetter, V8DOMConfiguration::kAllWorlds },
       { "cachedArrayAttribute", V8TestObject::CachedArrayAttributeAttributeGetterCallback, V8TestObject::CachedArrayAttributeAttributeSetterCallback, V8PrivateProperty::kNoCachedAccessor, static_cast<v8::PropertyAttribute>(v8::None), V8DOMConfiguration::kOnPrototype, V8DOMConfiguration::kCheckHolder, V8DOMConfiguration::kHasSideEffect, V8DOMConfiguration::kAlwaysCallGetter, V8DOMConfiguration::kAllWorlds },
+      { "readonlyCachedAttribute", V8TestObject::ReadonlyCachedAttributeAttributeGetterCallback, nullptr, V8PrivateProperty::kNoCachedAccessor, static_cast<v8::PropertyAttribute>(v8::ReadOnly), V8DOMConfiguration::kOnPrototype, V8DOMConfiguration::kCheckHolder, V8DOMConfiguration::kHasSideEffect, V8DOMConfiguration::kAlwaysCallGetter, V8DOMConfiguration::kAllWorlds },
       { "cachedStringOrNoneAttribute", V8TestObject::CachedStringOrNoneAttributeAttributeGetterCallback, V8TestObject::CachedStringOrNoneAttributeAttributeSetterCallback, V8PrivateProperty::kNoCachedAccessor, static_cast<v8::PropertyAttribute>(v8::None), V8DOMConfiguration::kOnPrototype, V8DOMConfiguration::kCheckHolder, V8DOMConfiguration::kHasSideEffect, V8DOMConfiguration::kAlwaysCallGetter, V8DOMConfiguration::kAllWorlds },
       { "callWithExecutionContextAnyAttribute", V8TestObject::CallWithExecutionContextAnyAttributeAttributeGetterCallback, V8TestObject::CallWithExecutionContextAnyAttributeAttributeSetterCallback, V8PrivateProperty::kNoCachedAccessor, static_cast<v8::PropertyAttribute>(v8::None), V8DOMConfiguration::kOnPrototype, V8DOMConfiguration::kCheckHolder, V8DOMConfiguration::kHasSideEffect, V8DOMConfiguration::kAlwaysCallGetter, V8DOMConfiguration::kAllWorlds },
       { "callWithScriptStateAnyAttribute", V8TestObject::CallWithScriptStateAnyAttributeAttributeGetterCallback, V8TestObject::CallWithScriptStateAnyAttributeAttributeSetterCallback, V8PrivateProperty::kNoCachedAccessor, static_cast<v8::PropertyAttribute>(v8::None), V8DOMConfiguration::kOnPrototype, V8DOMConfiguration::kCheckHolder, V8DOMConfiguration::kHasSideEffect, V8DOMConfiguration::kAlwaysCallGetter, V8DOMConfiguration::kAllWorlds },

third_party/blink/renderer/bindings/tests/results/core/v8_test_object.h

@@ -218,6 +218,7 @@ class V8TestObject {
   CORE_EXPORT static void CachedAttributeAnyAttributeAttributeSetterCallback(const v8::FunctionCallbackInfo<v8::Value>&);
   CORE_EXPORT static void CachedArrayAttributeAttributeGetterCallback(const v8::FunctionCallbackInfo<v8::Value>&);
   CORE_EXPORT static void CachedArrayAttributeAttributeSetterCallback(const v8::FunctionCallbackInfo<v8::Value>&);
+  CORE_EXPORT static void ReadonlyCachedAttributeAttributeGetterCallback(const v8::FunctionCallbackInfo<v8::Value>&);
   CORE_EXPORT static void CachedStringOrNoneAttributeAttributeGetterCallback(const v8::FunctionCallbackInfo<v8::Value>&);
   CORE_EXPORT static void CachedStringOrNoneAttributeAttributeSetterCallback(const v8::FunctionCallbackInfo<v8::Value>&);
   CORE_EXPORT static void CallWithExecutionContextAnyAttributeAttributeGetterCallback(const v8::FunctionCallbackInfo<v8::Value>&);