Split custom element script use and move it into bindings

Split custom element script use and move it into bindings

In the future Web Modules may define custom elements from C++. This
adds some abstract classes in preparation for that.

In addition, adds some context death tests.

BUG=594918
Committed: https://crrev.com/c4ccc6b681aba1870c8aed69b3a18201ee97ddd4
Cr-Commit-Position: refs/heads/master@{#396672}

[dominicc (has gone to gerrit), 2016-05-25 04:35:47]

dominicc@chromium.org changed reviewers:
+ esprehn@chromium.org

[dominicc (has gone to gerrit), 2016-05-25 04:37:00]

PTAL

[esprehn, 2016-05-26 04:50:28]

esprehn@chromium.org changed reviewers:
+ haraken@chromium.org

[esprehn, 2016-05-26 04:50:29]

This lgtm, haraken@ do you or someone on bindings want to make sure the
bindings/ stuff looks good?

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp
(right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistry.cpp:105:
CustomElementDefinition* def = builder.build(descriptor);
definition

[Yuki, 2016-05-26 07:48:39]

yukishiino@chromium.org changed reviewers:
+ yukishiino@chromium.org

[Yuki, 2016-05-26 07:48:40]

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:29:
, m_scriptState(ScriptState::from(m_registry->CreationContext()))
What script state are you expecting here?  It's the same as |current|, isn't it?

If it's not the same as |current|, could you add a comment?

[haraken, 2016-05-26 08:33:46]

Hmm, I'm a bit confused.

Form the perspective of providing web modules APIs, what matters is that we
remove script-dependent parameters from the APIs in core/. Whether we have
script-dependent implementations in core/ or not won't really matter.

- This CL moves all script-dependent implementations from core/ to bindings/
introducing abstraction classes, but is it really needed? As far as I
understand, the goal is to make the exposed APIs script-independent, not to make
the implementation in core/ script-independent.

- CustomElementsRegistry::define still depends on ScriptState and ScriptValue.
Is this okay?

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:74:
RegistryScriptStateScope scope(scriptState, registry);

Why do you need to enter ScriptState's scope here? You're already in the scope
of |scriptState|, so I don't think you need to enter something else.

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp
(right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:66:
v8::TryCatch tryCatch(isolate);

I think you can remove TryCatch. Checking the return value of v8Call would be
enough.

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:83:
return m_scriptState->contextIsValid();

What is this checking?

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h
(right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h:39:
ScriptState* m_scriptState;

Use RefPtr<ScriptState>.

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h (right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h:116:
Persistent<V8CustomElementPerContextData> m_customElements;

Does this need to be a member of V8PerContextData? I'm feeling that
V8CustomElementPerContextData is something that is more associated with
ExecutionContext.

[haraken, 2016-05-26 15:31:37]

On 2016/05/26 08:33:46, haraken wrote:
> Hmm, I'm a bit confused.
> 
> Form the perspective of providing web modules APIs, what matters is that we
> remove script-dependent parameters from the APIs in core/. Whether we have
> script-dependent implementations in core/ or not won't really matter.
> 
> - This CL moves all script-dependent implementations from core/ to bindings/
> introducing abstraction classes, but is it really needed? As far as I
> understand, the goal is to make the exposed APIs script-independent, not to
make
> the implementation in core/ script-independent.

^^^ Sorry, please ignore the above comment :) Looking at again, the class split
between core/ and bindings/ looks good.

[dominicc (has gone to gerrit), 2016-05-26 21:17:47]

On 2016/05/26 at 07:48:40, yukishiino wrote:
>
https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
> File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
(right):
> 
>
https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
>
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:29:
, m_scriptState(ScriptState::from(m_registry->CreationContext()))
> What script state are you expecting here?  It's the same as |current|, isn't
it?
> 
> If it's not the same as |current|, could you add a comment?

Could you give me some pointers what you think is missing from the comment on
line 18?

[haraken, 2016-05-26 22:42:47]

On 2016/05/26 21:17:47, dominicc wrote:
> On 2016/05/26 at 07:48:40, yukishiino wrote:
> >
>
https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
> > File
> third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
> >
>
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:29:
> , m_scriptState(ScriptState::from(m_registry->CreationContext()))
> > What script state are you expecting here?  It's the same as |current|, isn't
> it?
> > 
> > If it's not the same as |current|, could you add a comment?
> 
> Could you give me some pointers what you think is missing from the comment on
> line 18?

I don't understand. You call toV8(register, current) and use
register->CreationContext(). Then register->CreationContext() should be equal to
|current|.

Or is it possible that toV8(register, current) can be called for different
|current|s? If that happens, that's a bug.

[haraken, 2016-05-26 23:08:35]

haraken@chromium.org changed reviewers:
+ dglazkov@chromium.org

[haraken, 2016-05-26 23:08:36]

I've learned that there is some time pressure to ship Custom Elements.

As far as I understand, web modules APIs are not a mandatory part to ship Custom
Elements. Shall we suspend the work a bit for now and instead focus on shipping
and stabilizing?

[dominicc (has gone to gerrit), 2016-05-27 04:53:33]

On 2016/05/26 at 23:08:36, haraken wrote:
> I've learned that there is some time pressure to ship Custom Elements.
> 
> As far as I understand, web modules APIs are not a mandatory part to ship
Custom Elements. Shall we suspend the work a bit for now and instead focus on
shipping and stabilizing?

Noooooooooo... We've rebased our next patches onto this.

The toV8(registry, current) thing is just to get the wrapper for registry; I
would be fine using doing something else but I thought toV8 was the preferred
way to do it.

There's two cases of forConstructor I think:

1. In define, to check the constructor has not already been used for another
element.

2. In V8HTMLElement constructor, to find the definition given a constructor.

In the first case the registry's wrapper exists, because you had to use it to
call define. It may not be the same context as the current context, in the
frame.contentWindow.define case.

In the second case the registry's wrapper *may not exist* if an author did
Reflect.construct(HTMLElement, [], X) where the custom elements registry has
never been touched. (Not useful, not likely, but possible.) In this situation,
the current context and the registry's window/frame/etc. match, because we
retrieve the window from the context in V8HTMLElement constructor.

Does that make sense?

[dominicc (has gone to gerrit), 2016-05-27 04:54:17]

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:29:
, m_scriptState(ScriptState::from(m_registry->CreationContext()))
On 2016/05/26 at 07:48:40, Yuki wrote:
> What script state are you expecting here?  It's the same as |current|, isn't
it?
> 
> If it's not the same as |current|, could you add a comment?

OK, added some examples.

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:74:
RegistryScriptStateScope scope(scriptState, registry);
On 2016/05/26 at 08:33:46, haraken wrote:
> Why do you need to enter ScriptState's scope here? You're already in the scope
of |scriptState|, so I don't think you need to enter something else.

When the HTMLElement constructor uses forConstructor, yes, the context of
scriptState is the same.

However when you do frame.contentWindow.customElements.define('my-tag', X) then
scriptState is from the thing running script, but we have to look up X in the
map associated with frame.contentWindow.customElements. That is created in
frame's context. adamk advised me to always access these objects from the
context they were created in.

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp
(right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:66:
v8::TryCatch tryCatch(isolate);
On 2016/05/26 at 08:33:46, haraken wrote:
> I think you can remove TryCatch. Checking the return value of v8Call would be
enough.

Done.

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:83:
return m_scriptState->contextIsValid();
On 2016/05/26 at 08:33:46, haraken wrote:
> What is this checking?

Retrieving the prototype can run script and detach the window. See
third_party/WebKit/LayoutTests/custom-elements/define-context-dies-retrieving-prototype.html

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h
(right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.h:39:
ScriptState* m_scriptState;
On 2016/05/26 at 08:33:46, haraken wrote:
> Use RefPtr<ScriptState>.

Done. Good point--thanks.

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h (right):

https://codereview.chromium.org/2003033004/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/bindings/core/v8/V8PerContextData.h:116:
Persistent<V8CustomElementPerContextData> m_customElements;
On 2016/05/26 at 08:33:46, haraken wrote:
> Does this need to be a member of V8PerContextData? I'm feeling that
V8CustomElementPerContextData is something that is more associated with
ExecutionContext.

OK, I tried to do this. Because putting things in core/dom would mean making
them the abstract types, I simply stored these on CustomElementsRegistry in
keeping with the split esprehn wants (I think.)

[haraken, 2016-05-27 08:19:04]

On 2016/05/27 04:53:33, dominicc wrote:
> On 2016/05/26 at 23:08:36, haraken wrote:
> > I've learned that there is some time pressure to ship Custom Elements.
> > 
> > As far as I understand, web modules APIs are not a mandatory part to ship
> Custom Elements. Shall we suspend the work a bit for now and instead focus on
> shipping and stabilizing?
> 
> Noooooooooo... We've rebased our next patches onto this.
> 
> The toV8(registry, current) thing is just to get the wrapper for registry; I
> would be fine using doing something else but I thought toV8 was the preferred
> way to do it.
> 
> There's two cases of forConstructor I think:
> 
> 1. In define, to check the constructor has not already been used for another
> element.
> 
> 2. In V8HTMLElement constructor, to find the definition given a constructor.
> 
> In the first case the registry's wrapper exists, because you had to use it to
> call define. It may not be the same context as the current context, in the
> frame.contentWindow.define case.
> 
> In the second case the registry's wrapper *may not exist* if an author did
> Reflect.construct(HTMLElement, [], X) where the custom elements registry has
> never been touched. (Not useful, not likely, but possible.) In this situation,
> the current context and the registry's window/frame/etc. match, because we
> retrieve the window from the context in V8HTMLElement constructor.
> 
> Does that make sense?

When you call frame.contentWindow.customElements.define(), the current context
is set to the frame's context. So it should match the registry's wrapper's
context. If you really hit a scenario where the register's wrapper's context
does not match the current context, you're leaking objects over contexts. That
is a bug.

BTW, why do you need to introduce the ScriptState::Scope in this CL? I wonder
why you didn't need it before this CL but you need it after this CL.

> > I've learned that there is some time pressure to ship Custom Elements.
> > 
> > As far as I understand, web modules APIs are not a mandatory part to ship
> Custom Elements. Shall we suspend the work a bit for now and instead focus on
> shipping and stabilizing?
> 
> Noooooooooo... We've rebased our next patches onto this.

I'll defer the decision to you, but I guess we need a couple of more cycles to
get this CL landable...

[esprehn, 2016-05-27 20:44:37]

So I think the primary goals here are:

1) Ship Custom Elements v1
2) Enable rewriting <marquee> as a C++ custom element instead of blink in JS.

So whatever abstractions we introduce we need to make sure they enable (2). We
do need to create the class that is HTMLMarqueeElement, which means the function
template from the .idl file.

Does this split make sense if we assume the goal of C++ Custom Elements is that?
I don't want to make a bunch of extra work that doesn't get us there or just
slows you down. :)

[haraken, 2016-05-27 21:09:19]

On 2016/05/27 20:44:37, esprehn wrote:
> So I think the primary goals here are:
> 
> 1) Ship Custom Elements v1
> 2) Enable rewriting <marquee> as a C++ custom element instead of blink in JS.
> 
> So whatever abstractions we introduce we need to make sure they enable (2). We
> do need to create the class that is HTMLMarqueeElement, which means the
function
> template from the .idl file.
> 
> Does this split make sense if we assume the goal of C++ Custom Elements is
that?
> I don't want to make a bunch of extra work that doesn't get us there or just
> slows you down. :)

I think that this split is going in a right direction but is not perfect (e.g.,
the API still depends on ScriptState). Also this CL introduces complex usages of
V8 APIs and it will take time to get it right. So I'd recommend we focus on
shipping Custom Elements v1 before worrying about the API modularization (can we
work on it after shipping?).

[dominicc (has gone to gerrit), 2016-05-27 22:46:33]

On 2016/05/27 at 08:19:04, haraken wrote:
> On 2016/05/27 04:53:33, dominicc wrote:
> > On 2016/05/26 at 23:08:36, haraken wrote:
> > > I've learned that there is some time pressure to ship Custom Elements.
> > > 
> > > As far as I understand, web modules APIs are not a mandatory part to ship
> > Custom Elements. Shall we suspend the work a bit for now and instead focus
on
> > shipping and stabilizing?
> > 
> > Noooooooooo... We've rebased our next patches onto this.
> > 
> > The toV8(registry, current) thing is just to get the wrapper for registry; I
> > would be fine using doing something else but I thought toV8 was the
preferred
> > way to do it.
> > 
> > There's two cases of forConstructor I think:
> > 
> > 1. In define, to check the constructor has not already been used for another
> > element.
> > 
> > 2. In V8HTMLElement constructor, to find the definition given a constructor.
> > 
> > In the first case the registry's wrapper exists, because you had to use it
to
> > call define. It may not be the same context as the current context, in the
> > frame.contentWindow.define case.
> > 
> > In the second case the registry's wrapper *may not exist* if an author did
> > Reflect.construct(HTMLElement, [], X) where the custom elements registry has
> > never been touched. (Not useful, not likely, but possible.) In this
situation,
> > the current context and the registry's window/frame/etc. match, because we
> > retrieve the window from the context in V8HTMLElement constructor.
> > 
> > Does that make sense?
> 
> When you call frame.contentWindow.customElements.define(), the current context
is set to the frame's context. So it should match the registry's wrapper's
context. If you really hit a scenario where the register's wrapper's context
does not match the current context, you're leaking objects over contexts. That
is a bug.

Ah, it's true that frame.contentWindow.customElements.define enters the frame's
context; but it is not true in general. I think the context for these calls is
set to the holder's context. So in the following case we end up with the current
context and registry's context being different:

// script in frame A
frameB.contentWindow.CustomElementsRegistry.prototype.define.call(customElements
/* note: from frame A */, 'a-a', X);

This is because the define call uses the context of the holder (frame B) but the
registry is from the "entered" context (frame A).

In custom elements, the definitions live in a specific registry. As an
implementation detail, we need to map a script object (the constructor) to some
C++ state, and we need to keep a couple of objects alive (constructor and
prototype.) We're dealing with script objects; that generally has to happen in a
V8 context. What context should that happen in? The same context of the
registry--at a basic level, it's the only context that is guaranteed to exist.
Would it be OK to use multiple contexts? No; V8 team advises against this. Hence
we should always use the registry's context. Do the bindings guarantee calls to
'define' always match the context of the registry? No. Hence we should enter the
context associated with the registry.

I could drop the scope entering and make define check that its holder and this'
context are the same, and throw an exception if they are not. However I don't
think other bindings work this way; and the above example works in this
patch--it defines a-a in frame A and everything works.

> BTW, why do you need to introduce the ScriptState::Scope in this CL? I wonder
why you didn't need it before this CL but you need it after this CL.

I'm fixing bugs as I notice them. This added context death tests, for example.

> > > I've learned that there is some time pressure to ship Custom Elements.
> > > 
> > > As far as I understand, web modules APIs are not a mandatory part to ship
> > Custom Elements. Shall we suspend the work a bit for now and instead focus
on
> > shipping and stabilizing?
> > 
> > Noooooooooo... We've rebased our next patches onto this.
> 
> I'll defer the decision to you, but I guess we need a couple of more cycles to
get this CL landable...

Let's do the work. I'm hesitant to throw away all of this work and have to redo
it later plus make esprehn displeased. I really appreciate your reviews, I'm
learning a ton about bindings, I think the quality of custom elements is
improving. Could we just push through? If this is completely untenable it is
better to find that out early before shipping something.

[dominicc (has gone to gerrit), 2016-05-27 23:00:09]

On 2016/05/27 at 20:44:37, esprehn wrote:
> So I think the primary goals here are:
> 
> 1) Ship Custom Elements v1
> 2) Enable rewriting <marquee> as a C++ custom element instead of blink in JS.
> 
> So whatever abstractions we introduce we need to make sure they enable (2). We
do need to create the class that is HTMLMarqueeElement, which means the function
template from the .idl file.

Got it.

> Does this split make sense if we assume the goal of C++ Custom Elements is
that? I don't want to make a bunch of extra work that doesn't get us there or
just slows you down. :)

I hope that C++ unit tests should be enough to demonstrate the potential to use
custom elements without script. Ah hah! This doesn't add any C++ unit tests. You
got me. That is because all that works right now is "new X()/supercall
constructor" and how that magic process gets kicked off actually depends on
script--because the HTMLElement constructor is a script thing.

So it is very pertinent that you mention a HTMLMarqueeElement class generated
from an IDL file; that would be the C++ thing that kicks off construction of a
similar nature. But I don't think it is reasonable to start that work now; it's
too much extra work.

But when we have another factory for elements--eg parsing--working with custom
elements I will add unit tests to show that reaching callbacks of some kind in
C++, though not a fully worked out design for HTMLMarqueeElement. Does that
sound OK?

[dominicc (has gone to gerrit), 2016-05-27 23:09:10]

On 2016/05/27 at 21:09:19, haraken wrote:
> I think that this split is going in a right direction but is not perfect
(e.g., the API still depends on ScriptState).

Look at what core/dom/custom/CustomElement* does with ScriptState; it passes it
back to the bindings. That is why there are two CustomElementsRegistry::define;
one for the bindings to call; another one (which doesn't use ScriptState!) for
web modules/etc.

I could write a custom binding for define which creates the
ScriptCustomElementDefinitionBuilder on the bindings side, but custom bindings
are discouraged.

I think anything more than that is a bit of design and refactoring work of the
bindings system, which probably needs more time to get right. But when it is
done I will be happy to move the CustomElementsRegistry::define(ScriptState*,
...) method over to it.

[haraken, 2016-05-28 00:00:02]

On 2016/05/27 22:46:33, dominicc wrote:
> On 2016/05/27 at 08:19:04, haraken wrote:
> > On 2016/05/27 04:53:33, dominicc wrote:
> > > On 2016/05/26 at 23:08:36, haraken wrote:
> > > > I've learned that there is some time pressure to ship Custom Elements.
> > > > 
> > > > As far as I understand, web modules APIs are not a mandatory part to
ship
> > > Custom Elements. Shall we suspend the work a bit for now and instead focus
> on
> > > shipping and stabilizing?
> > > 
> > > Noooooooooo... We've rebased our next patches onto this.
> > > 
> > > The toV8(registry, current) thing is just to get the wrapper for registry;
I
> > > would be fine using doing something else but I thought toV8 was the
> preferred
> > > way to do it.
> > > 
> > > There's two cases of forConstructor I think:
> > > 
> > > 1. In define, to check the constructor has not already been used for
another
> > > element.
> > > 
> > > 2. In V8HTMLElement constructor, to find the definition given a
constructor.
> > > 
> > > In the first case the registry's wrapper exists, because you had to use it
> to
> > > call define. It may not be the same context as the current context, in the
> > > frame.contentWindow.define case.
> > > 
> > > In the second case the registry's wrapper *may not exist* if an author did
> > > Reflect.construct(HTMLElement, [], X) where the custom elements registry
has
> > > never been touched. (Not useful, not likely, but possible.) In this
> situation,
> > > the current context and the registry's window/frame/etc. match, because we
> > > retrieve the window from the context in V8HTMLElement constructor.
> > > 
> > > Does that make sense?
> > 
> > When you call frame.contentWindow.customElements.define(), the current
context
> is set to the frame's context. So it should match the registry's wrapper's
> context. If you really hit a scenario where the register's wrapper's context
> does not match the current context, you're leaking objects over contexts. That
> is a bug.
> 
> Ah, it's true that frame.contentWindow.customElements.define enters the
frame's
> context; but it is not true in general. I think the context for these calls is
> set to the holder's context. So in the following case we end up with the
current
> context and registry's context being different:
> 
> // script in frame A
>
frameB.contentWindow.CustomElementsRegistry.prototype.define.call(customElements
> /* note: from frame A */, 'a-a', X);
> 
> This is because the define call uses the context of the holder (frame B) but
the
> registry is from the "entered" context (frame A).

Did you confirm it?

In your example, I think the holder points to customElements. Thus holder's
context should be equal to the current context (frame A). Notice that
info.Holder() is not pointing to a "holder" object in a normal sense. Except for
some edge cases, info.Holder() is pointing to a "this" object; i.e.,
info.Holder() == info.This().

Also I don't think the entered context is relevant here.

> 
> In custom elements, the definitions live in a specific registry. As an
> implementation detail, we need to map a script object (the constructor) to
some
> C++ state, and we need to keep a couple of objects alive (constructor and
> prototype.) We're dealing with script objects; that generally has to happen in
a
> V8 context. What context should that happen in? The same context of the
> registry--at a basic level, it's the only context that is guaranteed to exist.
> Would it be OK to use multiple contexts? No; V8 team advises against this.
Hence
> we should always use the registry's context. Do the bindings guarantee calls
to
> 'define' always match the context of the registry? No. Hence we should enter
the
> context associated with the registry.

If that's the behavior you want, you should:

- cache the ScriptState that created the CustomElementRegistry object in
CustomElementRegistry::m_scriptState (RefPtr<ScriptState>). You can pass in the
ScriptState by [ConstructorCallWith=ScriptState].

- stop passing in a ScriptState to CustomeElementRegistry::define().
Alternately, you should use the cached ScriptState on the
CustomeElementRegistry.

Does it make sense?

> 
> I could drop the scope entering and make define check that its holder and
this'
> context are the same, and throw an exception if they are not. However I don't
> think other bindings work this way; and the above example works in this
> patch--it defines a-a in frame A and everything works.
> 
> > BTW, why do you need to introduce the ScriptState::Scope in this CL? I
wonder
> why you didn't need it before this CL but you need it after this CL.
> 
> I'm fixing bugs as I notice them. This added context death tests, for example.

I'd prefer making the functional change in a separate CL (before or after this
CL).

[dominicc (has gone to gerrit), 2016-05-28 02:27:32]

On 2016/05/28 at 00:00:02, haraken wrote:
> On 2016/05/27 22:46:33, dominicc wrote:
> > On 2016/05/27 at 08:19:04, haraken wrote:
> > > On 2016/05/27 04:53:33, dominicc wrote:
> > > > On 2016/05/26 at 23:08:36, haraken wrote:
> > > > > I've learned that there is some time pressure to ship Custom Elements.
> > > > > 
> > > > > As far as I understand, web modules APIs are not a mandatory part to
ship
> > > > Custom Elements. Shall we suspend the work a bit for now and instead
focus
> > on
> > > > shipping and stabilizing?
> > > > 
> > > > Noooooooooo... We've rebased our next patches onto this.
> > > > 
> > > > The toV8(registry, current) thing is just to get the wrapper for
registry; I
> > > > would be fine using doing something else but I thought toV8 was the
> > preferred
> > > > way to do it.
> > > > 
> > > > There's two cases of forConstructor I think:
> > > > 
> > > > 1. In define, to check the constructor has not already been used for
another
> > > > element.
> > > > 
> > > > 2. In V8HTMLElement constructor, to find the definition given a
constructor.
> > > > 
> > > > In the first case the registry's wrapper exists, because you had to use
it
> > to
> > > > call define. It may not be the same context as the current context, in
the
> > > > frame.contentWindow.define case.
> > > > 
> > > > In the second case the registry's wrapper *may not exist* if an author
did
> > > > Reflect.construct(HTMLElement, [], X) where the custom elements registry
has
> > > > never been touched. (Not useful, not likely, but possible.) In this
> > situation,
> > > > the current context and the registry's window/frame/etc. match, because
we
> > > > retrieve the window from the context in V8HTMLElement constructor.
> > > > 
> > > > Does that make sense?
> > > 
> > > When you call frame.contentWindow.customElements.define(), the current
context
> > is set to the frame's context. So it should match the registry's wrapper's
> > context. If you really hit a scenario where the register's wrapper's context
> > does not match the current context, you're leaking objects over contexts.
That
> > is a bug.
> > 
> > Ah, it's true that frame.contentWindow.customElements.define enters the
frame's
> > context; but it is not true in general. I think the context for these calls
is
> > set to the holder's context. So in the following case we end up with the
current
> > context and registry's context being different:
> > 
> > // script in frame A
> >
frameB.contentWindow.CustomElementsRegistry.prototype.define.call(customElements
> > /* note: from frame A */, 'a-a', X);
> > 
> > This is because the define call uses the context of the holder (frame B) but
the
> > registry is from the "entered" context (frame A).
> 
> Did you confirm it?
>
> In your example, I think the holder points to customElements.

You're right. I guess we don't need this scope hopping stuff! Removed.

> Thus holder's context should be equal to the current context (frame A). Notice
that info.Holder() is not pointing to a "holdmer" object in a normal sense.
Except for some edge cases, info.Holder() is pointing to a "this" object; i.e.,
info.Holder() == info.This().
> 
> Also I don't think the entered context is relevant here.
> 
> > 
> > In custom elements, the definitions live in a specific registry. As an
> > implementation detail, we need to map a script object (the constructor) to
some
> > C++ state, and we need to keep a couple of objects alive (constructor and
> > prototype.) We're dealing with script objects; that generally has to happen
in a
> > V8 context. What context should that happen in? The same context of the
> > registry--at a basic level, it's the only context that is guaranteed to
exist.
> > Would it be OK to use multiple contexts? No; V8 team advises against this.
Hence
> > we should always use the registry's context. Do the bindings guarantee calls
to
> > 'define' always match the context of the registry? No. 

For the fossil record, here ^^^ is the faulty assumption. V8+bindings *do*
guarantee calls to 'define' match the context of the registry; in particular
they unwrap info.Holder() and not info.This().

> > Hence we should enter the
> > context associated with the registry.
> 
> If that's the behavior you want, you should:
> 
> - cache the ScriptState that created the CustomElementRegistry object in
CustomElementRegistry::m_scriptState (RefPtr<ScriptState>). You can pass in the
ScriptState by [ConstructorCallWith=ScriptState].

esprehn did not like that idea; see the review thread starting with
https://codereview.chromium.org/2003593003#msg3

> - stop passing in a ScriptState to CustomeElementRegistry::define().
Alternately, you should use the cached ScriptState on the
CustomeElementRegistry.
> 
> Does it make sense?

I 100% understand the idea but I'm not sure if it fits with the direction
esprehn wants.

We may have broken the impasse by simply not switching contexts.
 
> > I could drop the scope entering and make define check that its holder and
this'
> > context are the same, and throw an exception if they are not. However I
don't
> > think other bindings work this way; and the above example works in this
> > patch--it defines a-a in frame A and everything works.
> > 
> > > BTW, why do you need to introduce the ScriptState::Scope in this CL? I
wonder
> > why you didn't need it before this CL but you need it after this CL.
> > 
> > I'm fixing bugs as I notice them. This added context death tests, for
example.
> 
> I'd prefer making the functional change in a separate CL (before or after this
CL).

OK. (FWIW that was in the first version of this patch; it is not like I'm
spamming the review with changes...)

[dominicc (has gone to gerrit), 2016-05-28 02:31:32]

On 2016/05/27 at 23:00:09, dominicc wrote:
> On 2016/05/27 at 20:44:37, esprehn wrote:
> > So I think the primary goals here are:
> > 
> > 1) Ship Custom Elements v1
> > 2) Enable rewriting <marquee> as a C++ custom element instead of blink in
JS.
> > 
> > So whatever abstractions we introduce we need to make sure they enable (2).
We do need to create the class that is HTMLMarqueeElement, which means the
function template from the .idl file.
> 
> Got it.
> 
> > Does this split make sense if we assume the goal of C++ Custom Elements is
that? I don't want to make a bunch of extra work that doesn't get us there or
just slows you down. :)
> 
> I hope that C++ unit tests should be enough to demonstrate the potential to
use custom elements without script. Ah hah! This doesn't add any C++ unit tests.
You got me. That is because all that works right now is "new X()/supercall
constructor" and how that magic process gets kicked off actually depends on
script--because the HTMLElement constructor is a script thing.
> 
> So it is very pertinent that you mention a HTMLMarqueeElement class generated
from an IDL file; that would be the C++ thing that kicks off construction of a
similar nature. But I don't think it is reasonable to start that work now; it's
too much extra work.
> 
> But when we have another factory for elements--eg parsing--working with custom
elements I will add unit tests to show that reaching callbacks of some kind in
C++, though not a fully worked out design for HTMLMarqueeElement. Does that
sound OK?

Bit more context: Not ready for review, but
https://codereview.chromium.org/1985623002 see
third_party/WebKit/Source/core/dom/custom/CustomElementsRegistryTest.cpp
TEST_F(CustomElementsRegistryFrameTest, define_upgradesInDocumentElements). That
defines and "upgrades" a thing with just C++.

[haraken, 2016-05-28 08:29:48]

LGTM

> > > Hence we should enter the
> > > context associated with the registry.
> > 
> > If that's the behavior you want, you should:
> > 
> > - cache the ScriptState that created the CustomElementRegistry object in
> CustomElementRegistry::m_scriptState (RefPtr<ScriptState>). You can pass in
the
> ScriptState by [ConstructorCallWith=ScriptState].
> 
> esprehn did not like that idea; see the review thread starting with
> https://codereview.chromium.org/2003593003#msg3
> 
> > - stop passing in a ScriptState to CustomeElementRegistry::define().
> Alternately, you should use the cached ScriptState on the
> CustomeElementRegistry.
> > 
> > Does it make sense?
> 
> I 100% understand the idea but I'm not sure if it fits with the direction
> esprehn wants.
> 

Note that this is not a problem of preference but a problem of correctness.

For example, if define() can be called asynchronously by DOM (just like event
listeners), you need to store the ScriptState when the registry object is
created and use the ScriptState in the define(). This is a well-established
programming pattern in the binding layer.

If define() is guaranteed to be called by JS, the binding layer already
guarantees that define() is called with the same ScriptState that created the
registry object. That's why you don't need to store ScriptState anywhere (and
you should just use the ScriptState passed into define() by the binding layer).
So the latest patchset looks correct.

How to remove ScriptState for modularization is a separate discussion. If we
need to store the ScriptState for correctness reasons, we must do that. Then if
you need to remove the dependency on ScriptState, you'll need to introduce some
abstraction class that wraps ScriptState in a way that doesn't depend on V8.

(This is why I want to separate a discussion to correctly implement the feature
from a discussion to modularize the feature.)

https://codereview.chromium.org/2003033004/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
(right):

https://codereview.chromium.org/2003033004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:40:
if (!scriptState->contextIsValid())

Nit: I'd remove this check.

We normally don't check scriptState->contextIsValid() when the C++ method is
being called by JS (which means that you have a valid scriptState). We only
check it when the C++ method is invoked asynchronously by DOM (e.g., event
listeners).

You're right in that even if the C++ method is called by JS (and thus it's
guaranteed that you have a valid scriptState at the point when the C++ method is
invoked), the scriptState can go invalid when you call some V8 API (e.g.,
Get()). However, we don't check it every time for the following (subtle)
reasons:

- You need to check it every time you call V8 APIs on stack. This messes up the
code base and slows down performance.

- The spec does not have a concept of scriptState->contextIsValid(). Per the
spec, scriptState must be valid as long as you have any reference to the
browsing context. So ideally, we should remove scriptState->contextIsValid()
entirely.

- Lifetime management around scriptState->contextIsValid() is already broken in
many ways in other browsers as well.

https://codereview.chromium.org/2003033004/diff/140001/third_party/WebKit/Sou...
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp
(right):

https://codereview.chromium.org/2003033004/diff/140001/third_party/WebKit/Sou...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:82:
return m_scriptState->contextIsValid();

Ditto. I'd just return true here.

[dominicc (has gone to gerrit), 2016-05-30 00:10:02]

On 2016/05/28 at 08:29:48, haraken wrote:
> LGTM
> 
> > > > Hence we should enter the
> > > > context associated with the registry.
> > > 
> > > If that's the behavior you want, you should:
> > > 
> > > - cache the ScriptState that created the CustomElementRegistry object in
> > CustomElementRegistry::m_scriptState (RefPtr<ScriptState>). You can pass in
the
> > ScriptState by [ConstructorCallWith=ScriptState].
> > 
> > esprehn did not like that idea; see the review thread starting with
> > https://codereview.chromium.org/2003593003#msg3
> > 
> > > - stop passing in a ScriptState to CustomeElementRegistry::define().
> > Alternately, you should use the cached ScriptState on the
> > CustomeElementRegistry.
> > > 
> > > Does it make sense?
> > 
> > I 100% understand the idea but I'm not sure if it fits with the direction
> > esprehn wants.
> > 
> 
> Note that this is not a problem of preference but a problem of correctness.

I grok what you are saying and am 100% with you; we need the ScriptState before
calling back into script for callbacks.

How I'm making this work is the 'define' entrypoint which takes a ScriptState
passes it to the ScriptCustomElementDefinitionBuilder which passes it to the
ScriptCustomElementDefinition.

We will add virtuals to CustomElementDefinition; when they reach
ScriptCustomElementDefinition it has the saved ScriptState it needs.

Not ready for review yet (feedback always welcome) but you can see this here:
https://codereview.chromium.org/1985623002 eg
ScriptCustomElementDefinition::upgrade and trace the history of m_scriptState
backwards from there to CustomElementsRegistry::define(ScriptState*, ...).

> For example, if define() can be called asynchronously by DOM (just like event
listeners), you need to store the ScriptState when the registry object is
created and use the ScriptState in the define(). This is a well-established
programming pattern in the binding layer.
> 
> If define() is guaranteed to be called by JS, the binding layer already
guarantees that define() is called with the same ScriptState that created the
registry object. That's why you don't need to store ScriptState anywhere (and
you should just use the ScriptState passed into define() by the binding layer).
So the latest patchset looks correct.
> 
> How to remove ScriptState for modularization is a separate discussion. If we
need to store the ScriptState for correctness reasons, we must do that. Then if
you need to remove the dependency on ScriptState, you'll need to introduce some
abstraction class that wraps ScriptState in a way that doesn't depend on V8.
> 
> (This is why I want to separate a discussion to correctly implement the
feature from a discussion to modularize the feature.)
> 
>
https://codereview.chromium.org/2003033004/diff/140001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
(right):
> 
>
https://codereview.chromium.org/2003033004/diff/140001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:40:
if (!scriptState->contextIsValid())
> 
> Nit: I'd remove this check.
> 
> We normally don't check scriptState->contextIsValid() when the C++ method is
being called by JS (which means that you have a valid scriptState). We only
check it when the C++ method is invoked asynchronously by DOM (e.g., event
listeners).
> 
> You're right in that even if the C++ method is called by JS (and thus it's
guaranteed that you have a valid scriptState at the point when the C++ method is
invoked), the scriptState can go invalid when you call some V8 API (e.g.,
Get()). However, we don't check it every time for the following (subtle)
reasons:
> 
> - You need to check it every time you call V8 APIs on stack. This messes up
the code base and slows down performance.
> 
> - The spec does not have a concept of scriptState->contextIsValid(). Per the
spec, scriptState must be valid as long as you have any reference to the
browsing context. So ideally, we should remove scriptState->contextIsValid()
entirely.
> 
> - Lifetime management around scriptState->contextIsValid() is already broken
in many ways in other browsers as well.
> 
>
https://codereview.chromium.org/2003033004/diff/140001/third_party/WebKit/Sou...
> File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/2003033004/diff/140001/third_party/WebKit/Sou...
>
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:82:
return m_scriptState->contextIsValid();
> 
> Ditto. I'd just return true here.

I've removed both of these contextIsValids. In reading the spec more closely I
think my test was wrong; the constructor steps talk about the current global,
and not the active browsing context. And presumably V8 is changing the current
global correctly when the HTMLElement constructor starts running.

The next patch uses contextIsValid, but that is a callback situation. If I
understood your comments that is the correct thing to do in that situation.

[haraken, 2016-05-30 00:19:18]

On 2016/05/30 00:10:02, dominicc wrote:
> On 2016/05/28 at 08:29:48, haraken wrote:
> > LGTM
> > 
> > > > > Hence we should enter the
> > > > > context associated with the registry.
> > > > 
> > > > If that's the behavior you want, you should:
> > > > 
> > > > - cache the ScriptState that created the CustomElementRegistry object in
> > > CustomElementRegistry::m_scriptState (RefPtr<ScriptState>). You can pass
in
> the
> > > ScriptState by [ConstructorCallWith=ScriptState].
> > > 
> > > esprehn did not like that idea; see the review thread starting with
> > > https://codereview.chromium.org/2003593003#msg3
> > > 
> > > > - stop passing in a ScriptState to CustomeElementRegistry::define().
> > > Alternately, you should use the cached ScriptState on the
> > > CustomeElementRegistry.
> > > > 
> > > > Does it make sense?
> > > 
> > > I 100% understand the idea but I'm not sure if it fits with the direction
> > > esprehn wants.
> > > 
> > 
> > Note that this is not a problem of preference but a problem of correctness.
> 
> I grok what you are saying and am 100% with you; we need the ScriptState
before
> calling back into script for callbacks.
> 
> How I'm making this work is the 'define' entrypoint which takes a ScriptState
> passes it to the ScriptCustomElementDefinitionBuilder which passes it to the
> ScriptCustomElementDefinition.
> 
> We will add virtuals to CustomElementDefinition; when they reach
> ScriptCustomElementDefinition it has the saved ScriptState it needs.
> 
> Not ready for review yet (feedback always welcome) but you can see this here:
> https://codereview.chromium.org/1985623002 eg
> ScriptCustomElementDefinition::upgrade and trace the history of m_scriptState
> backwards from there to CustomElementsRegistry::define(ScriptState*, ...).
> 
> > For example, if define() can be called asynchronously by DOM (just like
event
> listeners), you need to store the ScriptState when the registry object is
> created and use the ScriptState in the define(). This is a well-established
> programming pattern in the binding layer.
> > 
> > If define() is guaranteed to be called by JS, the binding layer already
> guarantees that define() is called with the same ScriptState that created the
> registry object. That's why you don't need to store ScriptState anywhere (and
> you should just use the ScriptState passed into define() by the binding
layer).
> So the latest patchset looks correct.
> > 
> > How to remove ScriptState for modularization is a separate discussion. If we
> need to store the ScriptState for correctness reasons, we must do that. Then
if
> you need to remove the dependency on ScriptState, you'll need to introduce
some
> abstraction class that wraps ScriptState in a way that doesn't depend on V8.
> > 
> > (This is why I want to separate a discussion to correctly implement the
> feature from a discussion to modularize the feature.)
> > 
> >
>
https://codereview.chromium.org/2003033004/diff/140001/third_party/WebKit/Sou...
> > File
> third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2003033004/diff/140001/third_party/WebKit/Sou...
> >
>
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinition.cpp:40:
> if (!scriptState->contextIsValid())
> > 
> > Nit: I'd remove this check.
> > 
> > We normally don't check scriptState->contextIsValid() when the C++ method is
> being called by JS (which means that you have a valid scriptState). We only
> check it when the C++ method is invoked asynchronously by DOM (e.g., event
> listeners).
> > 
> > You're right in that even if the C++ method is called by JS (and thus it's
> guaranteed that you have a valid scriptState at the point when the C++ method
is
> invoked), the scriptState can go invalid when you call some V8 API (e.g.,
> Get()). However, we don't check it every time for the following (subtle)
> reasons:
> > 
> > - You need to check it every time you call V8 APIs on stack. This messes up
> the code base and slows down performance.
> > 
> > - The spec does not have a concept of scriptState->contextIsValid(). Per the
> spec, scriptState must be valid as long as you have any reference to the
> browsing context. So ideally, we should remove scriptState->contextIsValid()
> entirely.
> > 
> > - Lifetime management around scriptState->contextIsValid() is already broken
> in many ways in other browsers as well.
> > 
> >
>
https://codereview.chromium.org/2003033004/diff/140001/third_party/WebKit/Sou...
> > File
>
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2003033004/diff/140001/third_party/WebKit/Sou...
> >
>
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:82:
> return m_scriptState->contextIsValid();
> > 
> > Ditto. I'd just return true here.
> 
> I've removed both of these contextIsValids. In reading the spec more closely I
> think my test was wrong; the constructor steps talk about the current global,
> and not the active browsing context. And presumably V8 is changing the current
> global correctly when the HTMLElement constructor starts running.
> 
> The next patch uses contextIsValid, but that is a callback situation. If I
> understood your comments that is the correct thing to do in that situation.

Right, if you need to call back JS, you need to store the ScriptState on the
object and use it when calling back JS. At that point you need to check
scriptState->contextIsValid.

[dominicc (has gone to gerrit), 2016-05-30 00:22:51]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2016-05-30 00:22:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
esprehn@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2003033004/#ps160001
(title: "Address feedback.")

[commit-bot: I haz the power, 2016-05-30 00:23:02]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2003033004/160001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2003033004/160001

[commit-bot: I haz the power, 2016-05-30 04:20:10]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-05-30 04:21:33]

Description was changed from

==========
Split custom element script use and move it into bindings

In the future Web Modules may define custom elements from C++. This
adds some abstract classes in preparation for that.

In addition, adds some context death tests.

BUG=594918
==========

to

==========
Split custom element script use and move it into bindings

In the future Web Modules may define custom elements from C++. This
adds some abstract classes in preparation for that.

In addition, adds some context death tests.

BUG=594918

Committed: https://crrev.com/c4ccc6b681aba1870c8aed69b3a18201ee97ddd4
Cr-Commit-Position: refs/heads/master@{#396672}
==========

[commit-bot: I haz the power, 2016-05-30 04:21:35]

Patchset 9 (id:??) landed as
https://crrev.com/c4ccc6b681aba1870c8aed69b3a18201ee97ddd4
Cr-Commit-Position: refs/heads/master@{#396672}