1470242 - Automatically upgrade Custom Elements inside of XBL anon content before the XBL constructor runs

Status: RESOLVED WONTFIX

(Core :: XBL, task, P2)

Description

[Brian Grinstead [:bgrins]]

See https://bugzilla.mozilla.org/show_bug.cgi?id=1466833#c16 and https://bugzilla.mozilla.org/show_bug.cgi?id=1466833#c20. To fix Bug 1466833 we are going to upgrade the single element, but we'd like this to happen automatically in the future to avoid similar bugs.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Following up with https://bugzilla.mozilla.org/show_bug.cgi?id=1466833#c20

AutoCEReaction action does cause the upgrade to run. However that could cause re-entry to CustomElementRegistry::Upgrade when a dropmarker element is being upgraded, during browser start-up.

I would need to look into where it is used.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

I think I have something that works? Let me rebased to the latest central to verify.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1470242 - Automatically upgrade Custom Elements inside of XBL anon content before the XBL constructor runs

Review commit: https://reviewboard.mozilla.org/r/252190/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/252190/

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8986950 [details]
Bug 1470242 - Automatically upgrade Custom Elements inside of XBL anon content before the XBL constructor runs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252190/diff/1-2/

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8986950 [details]
Bug 1470242 - Automatically upgrade Custom Elements inside of XBL anon content before the XBL constructor runs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252190/diff/2-3/

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8986950 [details]
Bug 1470242 - Automatically upgrade Custom Elements inside of XBL anon content before the XBL constructor runs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252190/diff/3-4/

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

The patch can fix the problem described in bug 1466833. I didn't check CustomElementRegistry::IsCustomElementEnabled() in the function because I assume document can reach these (i.e. allow to run XBL) can surely use custom element.

I cannot figure out a reduced test case of bug 1466833 though. The custom element in the test runs when `nsAutoMicroTask` is in the function, however, I need the rest of the fix to fix the bug.

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8986950 [details]
Bug 1470242 - Automatically upgrade Custom Elements inside of XBL anon content before the XBL constructor runs

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/252190/diff/4-5/

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Hi :smaug,

Could you help me understand the approach with my patch? What I know is:

1) The test case tests the feature as described on in the summary of this bug: construct and connects CE in <content> before <constructor> runs.
2) The test case can be fixed by simply adding nsAutoMicroTask alone, not other pieces.
3) The other pieces (AutoCEReaction and call into CustomElements->Upgrade()) is needed in order to fix bug 1466833.
4) Patch causes perma-orange in Try [0] (WPT CSS test???). The same failure orange shows up even with just nsAutoMicroTask [1].

[0] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b7306db7272f00b377d771e4ef62dc673355e896
[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=4acebfd18134103c3b732f249621cdf5ddbc9777

What I can conclude from now is:

a) I don't think we have fully understand why bug 1466833 failed. Certainly simply ensure the added CE in <content> runs before XBL <constructor> won't help. However it might help in other cases.
b) Perhaps nsAutoMicroTask will change the timing of Promises and XBL binding constructions, and making this patch too risky?

Where do you recommend I should go from here? Perhaps you could comment on what these really triggers?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Adding nsAutoMicroTask to random places is risky. They can be web-visible, as the WPT CSS test seems to indicate. Now that I think, perhaps the existing nsAutoMicrotasks in XBL code are wrong.


I wonder, do we need synchronous version of Upgrade() method?

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Olli Pettay [:smaug] from comment #10)
> I wonder, do we need synchronous version of Upgrade() method?

I am not entire if the behavior specifically in bug 1466833 can or should be solved in the platform code at this point. But I do like to get this feature landed if it is not risky.

Interestingly, I don't see any failure if there are only nsAutoMicroTask and AutoCEReaction in the changes (without call into CustomElements->Upgrade()).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=305457377ac4b51ab816c1dd2a31161e8b42e45a

Is this something we want to land?

Comment 12

[:Paolo Amadini]

I'd say we should try to land this feature soon. In Chrome code we are already registering Custom Elements before any other script runs, so that every piece of code using one of our Custom Elements can assume that its properties are ready to use, whether the script uses createElement or uses a node already in the document.

Breaking this invariant may lead to subtle bugs, especially if nodes are passed around to helper functions and other components.

Bug 1470910 has other examples of special cases that are needed while we don't have this feature. The patch there has to call "upgrade" in two instances, but this also means all the other possible code paths where the element is created now require extra scrutiny to see if we missed and "upgrade" call, slowing down the review. Ideally, I'd prefer if we fixed this before landing bug 1470910.

Comment 13

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Could you verify that changesets as written in 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=305457377ac4b51ab816c1dd2a31161e8b42e45a

(Without calling into ::Upgrade()) helps the case you pointed out? If so I will ask for a review on that one.

Comment 14

[:Paolo Amadini]

Actually, I've rebased the patch in bug 1470910 on the latest mozilla-central and removed the two calls to "upgrade", and the element appears to be upgraded even without the patch in this bug. The test case in this bug still fails. Thoughts?

Comment 15

[Tim Guan-tin Chien [:timdream] (please needinfo)]

:-(

I have even less confidence in understanding that they do now. Let me ask around and get back to you.

Comment 16

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #14)
> Actually, I've rebased the patch in bug 1470910 on the latest
> mozilla-central and removed the two calls to "upgrade", and the element
> appears to be upgraded even without the patch in this bug. The test case in
> this bug still fails. Thoughts?

I checked before landing bug 1470910, and those two calls were required in automation (https://bugzilla.mozilla.org/show_bug.cgi?id=1470910#c36). One unexpected thing I noticed is that if I switched the registration to use setElementCreationCallback, then there were actually a couple of additional places where the manual call to upgrade() became required.

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1470242 - Automatically upgrade Custom Elements inside of XBL anon content before the XBL constructor runs

MozReview-Commit-ID: 7bgOVEik1gJ

Comment 18

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(nothing really changed, just move the WIP from Try before it disappears)

Comment 19

[Tim Guan-tin Chien [:timdream] (please needinfo)]

OK. I finally have some time to look into this again... this is my understanding so far and they might not be correct.

== AutoCEReaction ==

The proper way to invoke the CE constructor, like any other CE reactions, is to enqueue it into custom element reaction queue and run it in the specific order.

https://html.spec.whatwg.org/multipage/custom-elements.html#custom-element-reaction-queue

For JS APIs that translates to [CEReactions] extended attribute in WebIDL.

https://html.spec.whatwg.org/multipage/custom-elements.html#cereactions

In the implementation it is done by having the AutoCEReaction class called by the generated code, ensuring the to-be-upgraded elements upgrades before the JS methods/properties return to the JS caller.

When the anonymous content is cloned from the XBL document to the document, CE reactions is queued to the topmost CE reaction element queue. When the queue pops from the stack, the CE reactions executes.

In the XBL world, that means we must have the queue pop from the stack before the XBL constructor runs. Which translate to something like wrapping the method into another block like this for nsXBLService::LoadBindings().

       RefPtr<nsXBLBinding> binding;
-      xblService->LoadBindings(this, uri, principal, getter_AddRefs(binding),
-                               &dummy);
+      {
+        dom::AutoCEReaction autoCEReaction(
+          doc->GetDocGroup()->CustomElementReactionsStack(), nullptr);
+        xblService->LoadBindings(this, uri, principal, getter_AddRefs(binding),
+                                 &dummy);
+      }


The tricky thing being there is more than one place we call into nsXBLService::LoadBindings(), and many of them are not safe to run scripts.

The actual function that clones the content is actually nsXBLBinding::GenerateAnonymousContent() and it does only being called by nsXBLService::LoadBindings(), other than itself, but that doesn't fix the safe-to-run-script problem above.

There is one other place that calls AutoCEReaction than the generated code

https://searchfox.org/mozilla-central/rev/7c848ac7630df5baf1314b0c03e015683599efb9/parser/html/nsHtml5TreeOperation.cpp#418

but I guess it is safe to run scripts here anyway.

== Force it to run ==

The other approach I've tried is to ignore all the above and calls into the static CustomElementRegistry::Upgrade() method that does the real upgrade, in nsXBLBinding::ExecuteAttachedHandler()

https://searchfox.org/mozilla-central/rev/7c848ac7630df5baf1314b0c03e015683599efb9/dom/base/CustomElementRegistry.cpp#1208

This place is actually safe-to-run-script (since XBL constructor itself is JS), but it works against the CE reaction queue and thus triggered re-entry, and thus hits an assertion. I stop at there because I don't think the result will be sane, since the CE reactions will be triggered out-of-order just for the sake to have it run before the XBL constructor.

By the way, the tentative patch I did months ago is totally bogus, because the test case wasn't setup correctly -- as soon as I moved the binding to another document (as the binding XMLs usually do in production), it fails.

=====================

I still think it might be possible to find the right place to put AutoCEReaction class, but I would have to keep looking. Yet at this point, we could consider giving up retrofitting XBL and just call .upgrade() from the XBL constuctor, if that's not too much trouble.

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Ouch. It's been a long time since meant to write something down here.

There was an offline discussion about this and it was agreed that the risk outweighs the benefit here. The XBL bindings would have to do window.customElements.upgrade(<anon node just created>) in its constructor.

Thankfully this is somewhat manageable because anonymous content on a specific binding can be checked easily by looking at the <content> and can be easily searched on searchfox with <xul:tagname. There was a discussion about the possibility of adding a JS helper for this but I think the current state is good enough.

With that, I am going to set this to WONTFIX.