Recursively defining custom elements should not lead to redefinition

Recursively defining custom elements should not lead to redefinition

See https://github.com/whatwg/html/issues/1329

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

[dominicc (has gone to gerrit), 2016-06-02 02:23:56]

dominicc@chromium.org changed reviewers:
+ blink-reviews-bindings@chromium.org, domenic@chromium.org

[dominicc (has gone to gerrit), 2016-06-02 02:24:10]

PTAL

[haraken, 2016-06-02 02:29:35]

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

[haraken, 2016-06-02 02:29:36]

https://codereview.chromium.org/2029923003/diff/1/third_party/WebKit/Source/b...
File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp
(right):

https://codereview.chromium.org/2029923003/diff/1/third_party/WebKit/Source/b...
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:77:
if (v8CallOrCrash(

Would you help me understand why it's not enough to check the
m_didStartDefiningConstructor flag to detect if you're in a recursion or not?
Why do we need to store a reference from constructors to m_constructor?

[dominicc (has gone to gerrit), 2016-06-02 05:46:09]

On 2016/06/02 at 02:29:36, haraken wrote:
>
https://codereview.chromium.org/2029923003/diff/1/third_party/WebKit/Source/b...
> File
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp
(right):
> 
>
https://codereview.chromium.org/2029923003/diff/1/third_party/WebKit/Source/b...
>
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:77:
if (v8CallOrCrash(
> 
> Would you help me understand why it's not enough to check the
m_didStartDefiningConstructor flag to detect if you're in a recursion or not?
Why do we need to store a reference from constructors to m_constructor?

If you look at the diff of the layout test it should clear up your confusion.
There is one ScriptCustomElementDefinitionBuilder per call to 'define'. 'define'
can be called recursively. The two (or N) active
ScriptCustomElementDefinitionBuilders have no relationship to each other; they
use the set of constructors to communicate.

I could also do a stack-walk of ScriptCustomElementDefinitionBuilders; do you
think that would be better?

[haraken, 2016-06-02 09:10:34]

On 2016/06/02 05:46:09, dominicc wrote:
> On 2016/06/02 at 02:29:36, haraken wrote:
> >
>
https://codereview.chromium.org/2029923003/diff/1/third_party/WebKit/Source/b...
> > File
>
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp
> (right):
> > 
> >
>
https://codereview.chromium.org/2029923003/diff/1/third_party/WebKit/Source/b...
> >
>
third_party/WebKit/Source/bindings/core/v8/ScriptCustomElementDefinitionBuilder.cpp:77:
> if (v8CallOrCrash(
> > 
> > Would you help me understand why it's not enough to check the
> m_didStartDefiningConstructor flag to detect if you're in a recursion or not?
> Why do we need to store a reference from constructors to m_constructor?
> 
> If you look at the diff of the layout test it should clear up your confusion.
> There is one ScriptCustomElementDefinitionBuilder per call to 'define'.
'define'
> can be called recursively. The two (or N) active
> ScriptCustomElementDefinitionBuilders have no relationship to each other; they
> use the set of constructors to communicate.
> 
> I could also do a stack-walk of ScriptCustomElementDefinitionBuilders; do you
> think that would be better?

Would it be an option to just use a static variable for the flag? If I'm
understanding correctly, what we need to know is whether there is any
ScriptCustomElementDefinitionBuilder or not.

[domenic, 2016-06-02 18:52:56]

lgtm.

haraken, no, you are allowed to define custom elements reentrantly, you are just
not allowed to define them with the same name/constructor, so we need to keep
track of all currently-being-defined names/constructors. The spec changes this
corresponds to are at https://github.com/whatwg/html/pull/1333

[domenic, 2016-06-02 18:54:06]

https://github.com/whatwg/html/pull/1333/files?w=1 is a reasonably-readable spec
diff

[haraken, 2016-06-03 05:09:20]

On 2016/06/02 18:52:56, domenic wrote:
> lgtm.
> 
> haraken, no, you are allowed to define custom elements reentrantly, you are
just
> not allowed to define them with the same name/constructor, so we need to keep
> track of all currently-being-defined names/constructors. The spec changes this
> corresponds to are at https://github.com/whatwg/html/pull/1333

Thanks, makes sense.

Using V8PrivateProperty looks a bit overkilling. Given that all of the
constructors are on the stack, can we just make CustomElementRegistry hold a set
of v8::Local<v8::Object>? You can push/pop the constructor object in
CustomElementsRegistry::NameIsBeingDefined.

[dominicc (has gone to gerrit), 2016-06-03 07:45:21]

On 2016/06/03 at 05:09:20, haraken wrote:
> On 2016/06/02 18:52:56, domenic wrote:
> > lgtm.
> > 
> > haraken, no, you are allowed to define custom elements reentrantly, you are
just
> > not allowed to define them with the same name/constructor, so we need to
keep
> > track of all currently-being-defined names/constructors. The spec changes
this
> > corresponds to are at https://github.com/whatwg/html/pull/1333
> 
> Thanks, makes sense.
> 
> Using V8PrivateProperty looks a bit overkilling. Given that all of the
constructors are on the stack, can we just make CustomElementRegistry hold a set
of v8::Local<v8::Object>? You can push/pop the constructor object in
CustomElementsRegistry::NameIsBeingDefined.

Yes, this does feel like overkill. Let me try something else.

We probably can't have CustomElementRegistry hold the constructors because that
would expose v8 into core. We could use ScriptValue; I wonder if esprehn would
be comfortable with that...

[dominicc (has gone to gerrit), 2016-06-03 08:37:51]

PTAL, this uses a different approach while keeping the script objects on the
bindings side.

[haraken, 2016-06-03 12:26:14]

Yeah, this looks much tidier. LGTM.

[dominicc (has gone to gerrit), 2016-06-05 23:57:14]

The CQ bit was checked by dominicc@chromium.org

[dominicc (has gone to gerrit), 2016-06-05 23:57:15]

The patchset sent to the CQ was uploaded after l-g-t-m from
domenic@chromium.org, haraken@chromium.org
Link to the patchset: https://codereview.chromium.org/2029923003/#ps40001
(title: "Rebase.")

[commit-bot: I haz the power, 2016-06-05 23:57:27]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2029923003/40001

[commit-bot: I haz the power, 2016-06-06 01:33:04]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-06-06 01:34:10]

Description was changed from

==========
Recursively defining custom elements should not lead to redefinition

See https://github.com/whatwg/html/issues/1329

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

to

==========
Recursively defining custom elements should not lead to redefinition

See https://github.com/whatwg/html/issues/1329

BUG=594918

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

[commit-bot: I haz the power, 2016-06-06 01:34:11]

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