Fix V8 bindings for named constructors to set prototype object correctly

third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl

Index: third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl
diff --git a/third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl b/third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl
index a7e7b2ecbddb5cbcc66f4c711f6351ebb845ecc9..213b41fe3825a301d2f267f17b863ad5aadf1668 100644
--- a/third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl
+++ b/third_party/WebKit/Source/bindings/templates/interface.cpp.tmpl
@@ -522,6 +522,8 @@ static void {{cpp_class}}OriginSafeMethodSetter(v8::Local<v8::Name> name, v8::Lo
 {% block named_constructor %}
 {% from 'methods.cpp.tmpl' import generate_constructor with context %}
 {% if named_constructor %}
+{% set parent_wrapper_type_info = '&V8%s::wrapperTypeInfo' % parent_interface
+                                  if parent_interface else '0' %}
 {% set active_scriptwrappable_inheritance =
     'InheritFromActiveScriptWrappable'
     if active_scriptwrappable else
@@ -532,7 +534,7 @@ static void {{cpp_class}}OriginSafeMethodSetter(v8::Local<v8::Name> name, v8::Lo
 #pragma clang diagnostic push
 #pragma clang diagnostic ignored "-Wglobal-constructors"
 #endif
-const WrapperTypeInfo {{v8_class}}Constructor::wrapperTypeInfo = { gin::kEmbedderBlink, {{v8_class}}Constructor::domTemplate, {{v8_class}}::trace, {{v8_class}}::traceWrappers, 0, {{prepare_prototype_and_interface_object_func}}, "{{interface_name}}", 0, WrapperTypeInfo::WrapperTypeObjectPrototype, WrapperTypeInfo::{{wrapper_class_id}}, WrapperTypeInfo::{{active_scriptwrappable_inheritance}}, WrapperTypeInfo::{{event_target_inheritance}}, WrapperTypeInfo::{{lifetime}} };
+const WrapperTypeInfo {{v8_class}}Constructor::wrapperTypeInfo = { gin::kEmbedderBlink, {{v8_class}}Constructor::domTemplate, {{v8_class}}::trace, {{v8_class}}::traceWrappers, 0, {{prepare_prototype_and_interface_object_func}}, "{{interface_name}}", {{parent_wrapper_type_info}}, WrapperTypeInfo::WrapperTypeObjectPrototype, WrapperTypeInfo::{{wrapper_class_id}}, WrapperTypeInfo::{{active_scriptwrappable_inheritance}}, WrapperTypeInfo::{{event_target_inheritance}}, WrapperTypeInfo::{{lifetime}} };

[Yuki, 2017/01/26 07:42:26]

I'm very sorry that I was wrong about this.

We should specify |nullptr| (i.e. 0) here as the parent wrapper type info.  I
was misunderstanding that Image.__proto__ should be HTMLElement, but it
shouldn't because Image is not considered as an interface object.

I'm very sorry, but could you revert this point (and the above definition of
parent_wrapper_type_info, and the corresponding layout test)?

[sashab, 2017/02/21 06:37:29]

No prob :) Done.

 #if defined(COMPONENT_BUILD) && defined(WIN32) && COMPILER(CLANG)
 #pragma clang diagnostic pop
 #endif
@@ -548,12 +550,39 @@ v8::Local<v8::FunctionTemplate> {{v8_class}}Constructor::domTemplate(v8::Isolate
   result = v8::FunctionTemplate::New(isolate, {{v8_class}}ConstructorCallback);
   v8::Local<v8::ObjectTemplate> instanceTemplate = result->InstanceTemplate();
   instanceTemplate->SetInternalFieldCount({{v8_class}}::internalFieldCount);
-  result->SetClassName(v8AtomicString(isolate, "{{cpp_class}}"));
+  result->SetClassName(v8AtomicString(isolate, "{{named_constructor.name}}"));
   result->Inherit({{v8_class}}::domTemplate(isolate, world));
   data->setInterfaceTemplate(world, &domTemplateKey, result);
   return result;
 }
 
+void {{v8_class}}Constructor::NamedConstructorAttributeGetter(
+    v8::Local<v8::Name> propertyName,
+    const v8::PropertyCallbackInfo<v8::Value>& info) {
+  v8::Local<v8::Context> creationContext = info.Holder()->CreationContext();
+  V8PerContextData* perContextData = V8PerContextData::from(creationContext);

[Yuki, 2017/01/26 07:08:53]

This is okay that you're correctly using the *creation context*.

+  if (!perContextData) {
+    // TODO(yukishiino): Return a valid named constructor even after the context is detached
+    return;
+  }
+
+  v8::Local<v8::Function> namedConstructor = perContextData->constructorForType(&{{v8_class}}Constructor::wrapperTypeInfo);
+
+  // Set the prototype of named constructors to the regular constructor.
+  auto privateProperty = V8PrivateProperty::getNamedConstructorInitialized(info.GetIsolate());
+  v8::Local<v8::Value> privateValue = privateProperty.get(creationContext, namedConstructor);

[Yuki, 2017/01/26 07:08:53]

It's quite confusing, but *current context* can be different from *creation
context*.  So, would you use *current context* here and below?

v8::Local<v8::Context> currentContext = info.GetIsolate()->GetCurrentContext();

and you should use the |currentContext| here and below.

[haraken, 2017/01/26 09:43:55]

How can |creationContext| be different from |currentContext| here?

If they're different, we'll end up with caching a named constructor of context X
on a private property of a context Y. Which seems wrong to me.
 

[Yuki, 2017/01/26 10:07:11]

Access to |anotherWindow.Image|, then you're in the current context (by
definition), and accessing to |aotherWindow|, which is created in a different
context (this is called |creationContext| in this code because |Image| should be
created in the same context of |anotherWindow|).
The passed context here is NOT used to create a private key, value to be stored
in a private key, or anything else.

Players here are:
- object (or holder) of the private property
- the key (symbol) of the private property
- the value of the private property
i.e. we're performing
    obj[privateSymbol] = value
and there are three things.

|obj| is the named constructor, which is already created in |creationContext|.

|privateSymbol| does NOT depend on any v8::Context because it's common among
contexts, it only depends on v8::Isolate.

|value| is the prototype object of the interface (|interfacePrototype|), which
will be retrieved below (line 575 to 576).  Note that it's extracted from
|perContextData|, it's created in |creationContext|.

Nothing is created in |currentContext|.  |currentContext| is just used to make
V8 do "Get" and "Set" properties.  These operations should be done in
currentContext, and creationContext shouldn't be abused.

I think that haraken@ remembers a discussion that V8 may not need to take a
context in these APIs (except for throwing an exception, but an exception must
always be thrown in the current context).  Anyway, V8 decided to take a context
(even if it's not needed), then we should pass the current context.

[sashab, 2017/02/21 06:37:29]

I don't understand the conversation, but changed to use currentContext. :)

+  if (privateValue.IsEmpty()) {
+    v8::Local<v8::Function> interface = perContextData->constructorForType(&{{v8_class}}::wrapperTypeInfo);
+    v8::Local<v8::Value> interfacePrototype = interface->Get(creationContext, v8AtomicString(info.GetIsolate(), "prototype")).ToLocalChecked();

[Yuki, 2017/01/26 07:08:53]

These interface-Get and namedConstructor-Set should be done on *currentContext*
instead of creationContext.

[sashab, 2017/02/21 06:37:29]

Done.

+    bool result = namedConstructor->Set(creationContext, v8AtomicString(info.GetIsolate(), "prototype"), interfacePrototype).ToChecked();
+    if (!result)
+      return;
+    privateProperty.set(creationContext, namedConstructor, v8::True(info.GetIsolate()));

[Yuki, 2017/01/26 07:08:53]

This also should be currentContext instead of creationContext.

[sashab, 2017/02/21 06:37:29]

Done.

+  }
+
+  v8SetReturnValue(info, namedConstructor);
+}
+
 {% endif %}
 {% endblock %}