1408828 - Fix missing CustomElementData setup in CustomElementRegistry::SetupCustomElement

Status: RESOLVED INVALID

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[Edgar Chen [:edgar]]

This is a follow-up for bug 1392970.

- Since we keep a reference of the CustomElementDefinition in the custom element (which is in "custom" state) itself, we could avoid unnecessary hash table lookup in http://searchfox.org/mozilla-central/rev/40b456626e2d0409b7034768b4d9526fc7235ea4/dom/base/Element.cpp#523 though it might be removed when we totally deprecate v0 spec.

- And we only set CustomElementDefinition on a custom element which is "custom", we could also add more assertion to ensure that.

Comment 1

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=67b66a21ec489236697b197f6d31c55bed25f70d&group_state=expanded&filter-tier=1

Comment 2

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #1)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=67b66a21ec489236697b197f6d31c55bed25f70d&group_state=e
> xpanded&filter-tier=1

Ah, I just realized that there are probably some implementations for old spec missing updating correct state and definition to CustomElementData, like
- http://searchfox.org/mozilla-central/rev/40b456626e2d0409b7034768b4d9526fc7235ea4/dom/base/nsDocument.cpp#6369-6407
  which is the upgrading for old spec (the definition is registered via document.registerElement()).  
- http://searchfox.org/mozilla-central/source/dom/base/CustomElementRegistry.cpp#318-323
  which is used for the custom element created by the parser right now, but the parser will no longer use this after
  bug 1378079.

Although those codes will probably be removed/replaced when we switch all things to latest spec, I'd like to fix them still to have consistent behaviour in current code.

Comment 3

[Edgar Chen [:edgar]]

> - Since we keep a reference of the CustomElementDefinition in the custom
> element (which is in "custom" state) itself, we could avoid unnecessary hash
> table lookup in
> http://searchfox.org/mozilla-central/rev/
> 40b456626e2d0409b7034768b4d9526fc7235ea4/dom/base/Element.cpp#523 though it
> might be removed when we totally deprecate v0 spec.

Okay, after more investigating, there is one case that needs the hash table lookup, http://searchfox.org/mozilla-central/rev/40b456626e2d0409b7034768b4d9526fc7235ea4/dom/base/nsDocument.cpp#6434-6435, at this point, element have no correct CustomElementDefinition setup (setup happens later in CustomElementRegistry::Upgrade). So it looks like we could not just rely on the CustomElementDefinition in CustomElementData. :(
Since this is for old spec and will be removed eventually, I decide not to spend time fixing this.

> Ah, I just realized that there are probably some implementations for old
> spec missing updating correct state and definition to CustomElementData, like
> -
> http://searchfox.org/mozilla-central/rev/
> 40b456626e2d0409b7034768b4d9526fc7235ea4/dom/base/nsDocument.cpp#6369-6407
>   which is the upgrading for old spec (the definition is registered via
> document.registerElement()).

Actually, the previous investigation is not correct. In this case, the CustomElementData will be updated correctly later in CustomElementRegistry::Upgrade. We don't need to handle that here.

> -
> http://searchfox.org/mozilla-central/source/dom/base/CustomElementRegistry.
> cpp#318-323
>   which is used for the custom element created by the parser right now, but
> the parser will no longer use this after
>   bug 1378079.

This is why we could not find CustomElementDefinition in CustomElementData when createdcallback is invoked.

Comment 4

[Edgar Chen [:edgar]]

Attachment: Part 1: Add more assertion in CustomElementData::SetCustomElementDefinition and GetCustomElementDefinition, v1

Comment 5

[Edgar Chen [:edgar]]

Attachment: Part 2: Fix missing CustomElementData setup in CustomElementRegistry::SetupCustomElement, v1

Comment 6

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c3deeb1bb9c8c048d49ea65ab197b1f86d116b3&filter-tier=1&group_state=expanded&selectedJob=137189073

Comment 7

[Edgar Chen [:edgar]]

Comment on attachment 8918800 [details] [diff] [review]
Part 2: Fix missing CustomElementData setup in CustomElementRegistry::SetupCustomElement, v1

Review of attachment 8918800 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Edgar Chen [:edgar] from comment #6)
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=2c3deeb1bb9c8c048d49ea65ab197b1f86d116b3&filter-
> tier=1&group_state=expanded&selectedJob=137189073

It seems applying this part will hit "Assertion failure: js::IsObjectInContextCompartment(aGivenProto, aCx)" in some test cases, cancel the feedback request first...

Comment 8

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #7)
> Comment on attachment 8918800 [details] [diff] [review]
> Part 2: Fix missing CustomElementData setup in
> CustomElementRegistry::SetupCustomElement, v1
> 
> Review of attachment 8918800 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Edgar Chen [:edgar] from comment #6)
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=2c3deeb1bb9c8c048d49ea65ab197b1f86d116b3&filter-
> > tier=1&group_state=expanded&selectedJob=137189073
> 
> It seems applying this part will hit "Assertion failure:
> js::IsObjectInContextCompartment(aGivenProto, aCx)" in some test cases,
> cancel the feedback request first...

The assertion happens when we invoke custom element callbacks, similar to bug 1348220.
.innerHTML case is not yet switched to new spec steps (will be handled in bug 1378079), so it still do prototype swizzling in Element::WrapObject [1], and the prototype stored in CustomElementDefinition could be in different compartmenet with element's DOM reflector ....

We don't hit this assertion currently is because the connected callback reaction in [2] doesn't be enqueued at all: we pass CustomElementData->mType to LookupCustomElementDefinition() as aIs argument, but the mType of <x-foo> will be 'x-foo', the LookupCustomElementDefinition() here always returns no definition.

[1] http://searchfox.org/mozilla-central/rev/40b456626e2d0409b7034768b4d9526fc7235ea4/dom/base/Element.cpp#513
[2] http://searchfox.org/mozilla-central/rev/40b456626e2d0409b7034768b4d9526fc7235ea4/dom/base/CustomElementRegistry.cpp#39-52

Comment 9

[Edgar Chen [:edgar]]

Attachment: Part 3: Fix the crash when doing old prototype swizzling for custom element, v1

Comment 10

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc0e16d11fb1f01c51c70e0800c86490f92a2e6c&group_state=expanded&filter-tier=1

Comment 11

[Jessica Jong [:jessica] (inactive)]

Comment on attachment 8918800 [details] [diff] [review]
Part 2: Fix missing CustomElementData setup in CustomElementRegistry::SetupCustomElement, v1

Hmm, with this code, we will be setting custom element state to 'custom' before being upgraded. But I think it's ok, since parser code doesn't work correctly right now (doesn't even get upgraded).

Another question is, maybe we don't need `SetupCustomElement` anymore _after_ bug 1378079, right? Since the case we call SetupCustomElement will be when there is no definition, which is in: http://searchfox.org/mozilla-central/rev/dca019c94bf3a840ed7ff50261483410cfece24f/dom/html/nsHTMLContentSink.cpp#359

Maybe we can do all of this after bug 1378079, but it's still ok if you don't want to wait.

Comment 12

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #8)
> We don't hit this assertion currently is because the connected callback
> reaction in [2] doesn't be enqueued at all: we pass CustomElementData->mType
> to LookupCustomElementDefinition() as aIs argument, but the mType of <x-foo>
> will be 'x-foo', the LookupCustomElementDefinition() here always returns no
> definition.

Err. We _do_ enqueue connected callback when invoking createdCallback currently.

The assertion happens after this patch is actually because we will enqueue an additional connectedCallback in BindToTree() which will get fired before createdCallback, it is not correct against old spec: any callback should not fired before createdCallback, and also incorrectly invoke connectedCallback twice if createdCallback is defined .... 

(In reply to Jessica Jong [:jessica] from comment #11)
> Comment on attachment 8918800 [details] [diff] [review]
> Part 2: Fix missing CustomElementData setup in
> CustomElementRegistry::SetupCustomElement, v1
> 
> Hmm, with this code, we will be setting custom element state to 'custom'
> before being upgraded. But I think it's ok, since parser code doesn't work
> correctly right now (doesn't even get upgraded).
> 
> Another question is, maybe we don't need `SetupCustomElement` anymore
> _after_ bug 1378079, right?

Yeah, we don't need the SetupCustomElement after bug 1378079.
I am doing this changes is mainly because I want to add assertion to ensure that we fire callback _only_ on an element which is "custom", but we don't set CustomElementData correctly in parser case right now. But parser case will correct run upgrade steps and also set CustomElementData correctly after bug 1378079.

> Since the case we call SetupCustomElement will
> be when there is no definition, which is in:
> http://searchfox.org/mozilla-central/rev/
> dca019c94bf3a840ed7ff50261483410cfece24f/dom/html/nsHTMLContentSink.cpp#359
> 
> Maybe we can do all of this after bug 1378079, but it's still ok if you
> don't want to wait.

Given this patch will cause unexpected behaviour mention above and we also plan to remove createdCallback in bug 1396620.
Yes, we can do "add assertion to ensure that we fire callback _only_ on an element which is custom" things after bug 1378079.

So I am going to,
- File a separated bug for adding more assertion in CustomElementData::SetCustomElementDefinition and GetCustomElementDefinition (for part 1 patch)
- Probably do "add assertion to ensure that we fire callback _only_ on an element which is custom" in bug 1396620 or file a separated bug for that (after bug 1378079).
- Close this bug since we don't need to call SetupCustomElement for parser case after bug 1378079.

Comment 13

[Edgar Chen [:edgar]]

Close per comment 12.