1299363 - Implement custom element v1 upgrade algorithm

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P2)

Description

[Edgar Chen [:edgar]]

The v0 upgrade is quite different from v1 upgrade [1], this bug is filed for moving current implementation for upgrade from v0 to v1.

[1] Please see https://html.spec.whatwg.org/multipage/scripting.html#enqueue-a-custom-element-upgrade-reaction and https://html.spec.whatwg.org/multipage/scripting.html#upgrades

Comment 1

[Andrew Overholt [:overholt]]

Are we aiming to get this done in the next few months, Edgar? I imagine so so I'm marking this as P2 but feel free to change that if I'm incorrect.

Comment 2

[Edgar Chen [:edgar]]

upgrade reaction [1] will be handled in bug 1315885, this bug will force on upgrade steps [2].

[1] https://html.spec.whatwg.org/multipage/scripting.html#enqueue-a-custom-element-upgrade-reaction
[2] https://html.spec.whatwg.org/multipage/scripting.html#upgrades

Comment 4

[Edgar Chen [:edgar]]

Attachment: WIP, Part 1: Allow switching prototype in html constructor, v1

Comment 5

[Edgar Chen [:edgar]]

Attachment: WIP, Part 2: Implement construction stack, v1

Comment 6

[Edgar Chen [:edgar]]

Attachment: WIP, Part 3-1: Implement upgrade steps, v1

Comment 7

[Edgar Chen [:edgar]]

Attachment: WIP, Part 3: Implement upgrade steps, v2

Comment 8

[Edgar Chen [:edgar]]

TODO:
- Rebase over bug 1315885.
- Implement custom element state [1].
- Maybe write some tests for Xray.
- Review web-platform-test coverage.

[1] https://dom.spec.whatwg.org/#concept-element-custom-element-state

Comment 9

[Edgar Chen [:edgar]]

Attachment: Part 1: Implement construction stack, v2

Comment 10

[Edgar Chen [:edgar]]

Attachment: Part 2: Implement custom element state, v1

Comment 11

[Edgar Chen [:edgar]]

Attachment: Part 1: Implement construction stack, v3

Rebase

Comment 12

[Edgar Chen [:edgar]]

Attachment: Part 2: Implement custom element state, v2

Rebase

Comment 13

[Edgar Chen [:edgar]]

Attachment: Part 3: Allow prototype swizzling in html constructor, v2

Comment 14

[Edgar Chen [:edgar]]

Attachment: Part 4: Remove unused argumenet in UpgradeCandidates, v1

Comment 15

[Edgar Chen [:edgar]]

Attachment: Part 5: Hold a pointer of ElementQueue in ReactionsStack instead, v1

Comment 16

[Edgar Chen [:edgar]]

Attachment: Part 6: Implement new upgrade steps, v2

Comment 17

[Edgar Chen [:edgar]]

Attachment: Part 7: Add wpt test for reporting an exception thrown by a custom element constructor when upgrading, v1

Comment 18

[Edgar Chen [:edgar]]

Attachment: Part 8: Add tests for Xray case, v1

Comment 19

[Edgar Chen [:edgar]]

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

Comment 20

[Edgar Chen [:edgar]]

Comment on attachment 8838497 [details] [diff] [review]
Part 2: Implement custom element state, v2

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

This patch implements custom element state in CustomElementData, and it requires a bit refactoring to create CustomElementData earlier (when SetupCustomElement is called).
And this refactoring will also benefit bug 1325279.

Comment 21

[Edgar Chen [:edgar]]

Comment on attachment 8838498 [details] [diff] [review]
Part 3: Allow prototype swizzling in html constructor, v2

The steps of upgrading a custom element do prototype swizzling in HTML constructor, see
- https://html.spec.whatwg.org/multipage/scripting.html#upgrades (step 7).
- https://html.spec.whatwg.org/multipage/dom.html#htmlconstructor (step 11).

But DoGetOrCreateDOMReflector doesn't allow changing prototype if DOM reflector already has one. So this patch extends CGPerSignatureCall to generate codes for prototype swizzling and it is only avaiable in HTML constructor.
Hi bz, could you help to review the codegen changes? Thank you.

Comment 22

[Edgar Chen [:edgar]]

Attachment: The resulting codegen of HTMLElement (HTMLElementBinding.cpp)

Comment 23

[Edgar Chen [:edgar]]

Comment on attachment 8838499 [details] [diff] [review]
Part 4: Remove unused argumenet in UpgradeCandidates, v1

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

Just remove unused argument.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Calling JS_SetPrototype will permanently deoptimize all property access on that element.  I guess that's unavoidable in the upgrade case if the element has previously had its reflector created.

But as this code is written, it could apply to non-upgrade cases too, no?  It might happen to not trigger because there's no reflector, but there's no clarity about when and how that might happen or not happen.

I would vastly prefer it if we followed the spec more closely here.  Specifically we want it to be _very_ clear that JS_SetPrototype does not happen if the "construction stack" is empty (the non-upgrade) case.  That's the equivalent of https://html.spec.whatwg.org/multipage/dom.html#htmlconstructor step 8.2, and we want to fuse that with step 8.1, basically.  That means the JS_SetPrototype needs to happen, if it happens at all, inside CreateHTMLElement, where we know whether the construction stack is empty.  That means you need to pass the desired proto to CreateHTMLElement.

In the case when the construction stack is nonempty, we do need to do the JS_SetPrototype.  The correct way to do that is to enter the reflector compartment, wrap the provided proto into that compartment, and then set it (so backwards from the way you have it in the attached patch).

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8838498 [details] [diff] [review]
Part 3: Allow prototype swizzling in html constructor, v2

Doing this in CreateHTMLElement would also have the minor benefit of only needing one copy of this code, of course.

And I just realized that your patch _does_ wrap the desired proto into the reflector compartment; I don't know how I misread that.  So that part is right already.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8838496 [details] [diff] [review]
Part 1: Implement construction stack, v3

>+  if (!constructorStack.Length()) {

  if (constructorStack.IsEmpty())

Also, why is this not called "constructionStack"?

Comment 27

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #26)
> Comment on attachment 8838496 [details] [diff] [review]
> Part 1: Implement construction stack, v3
> 
> >+  if (!constructorStack.Length()) {
> 
>   if (constructorStack.IsEmpty())

Done.

> 
> Also, why is this not called "constructionStack"?

Oop, it is a typo. Fixed.

Comment 28

[Edgar Chen [:edgar]]

Attachment: Part 1: Implement construction stack, v4

Addressed comment #26,
- if (constructorStack.IsEmpty() { ...
- s/constructorStack/constructionStack/

Comment 29

[Edgar Chen [:edgar]]

Attachment: Part 3: Allow prototype swizzling in html constructor, v3

Addressed review comment #24 and comment #25.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8839358 [details] [diff] [review]
Part 3: Allow prototype swizzling in html constructor, v3

>+  // DoGetOrCreateDOMReflector doesn't allow changing prototype if DOM reflector

DoGetOrCreateDOMReflector does what it says on the tin: it gets the reflector if we have one, else creates it with the given proto.

This comment should probably say something like this:

  // Do prototype swizzling for upgrading a custom element here, for cases when
  // we have a reflector already.  If we don't have one yet, our caller will 
  // create it with the right proto (by calling DoGetOrCreateDOMReflector with
  // that proto).

or something along those lines.

r=me with that.

Comment 31

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #30)
> This comment should probably say something like this:
> 
>   // Do prototype swizzling for upgrading a custom element here, for cases when
>   // we have a reflector already.  If we don't have one yet, our caller will 
>   // create it with the right proto (by calling DoGetOrCreateDOMReflector with
>   // that proto).
> 
> or something along those lines.

Done. Thanks for the review.

Comment 32

[Edgar Chen [:edgar]]

Attachment: Part 3: Allow prototype swizzling in html constructor, v4, r=bz

Addressed review comment #30.

Comment 33

[Edgar Chen [:edgar]]

Attachment: Part 6: Implement new upgrade steps, v3

I rewrote the CustomElementRegistry::Upgrade() a bit to make it having clearer flow and better error handling.

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8839421 [details] [diff] [review]
Part 6: Implement new upgrade steps, v3

In this patch we have:

   AutoJSAPI jsapi;
...
+  DoCustomElementUpgrade(cx, aElement, constructor, rv);

But DoCustomElementUpgrade calls "constructor", no?  And isn't that page-provided?

You need an AutoEntryScript here, not an AutoJSAPI; it's really rather depressing that we apparently don't have decent checks for this.  And if the spec doesn't have the right bits about pushing entry settings objects aroung this stuff, it needs to grow them.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Oh, and once the spec clarifies this stuff (or if it's already clear), you should double-check which exact object should be passed to the AutoEntryScript.  Please write some testcases testing the entry settings when "constructor" is not from mWindow.

Also, while we're here, I don't understand the point of the CheckedUnwrap() in DoCustomElementUpgrade.  UNWRAP_OBJECT will already CheckedUnwrap as needed.  And you don't need the manual IsDOMObject() check either; UNWRAP_OBJECT does _that_ too.

Comment 36

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #34)
> And if the spec doesn't have the right bits about pushing entry settings objects aroung
> this stuff, it needs to grow them.

Filed https://github.com/whatwg/html/issues/2381.

Comment 37

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) from comment #35)
> Also, while we're here, I don't understand the point of the CheckedUnwrap()
> in DoCustomElementUpgrade.  UNWRAP_OBJECT will already CheckedUnwrap as
> needed.  And you don't need the manual IsDOMObject() check either;
> UNWRAP_OBJECT does _that_ too.

You are totally right, CheckedUnwrap and IsDOMObject are redundant.
Will address them in next version of patch.

Comment 38

[Edgar Chen [:edgar]]

Comment on attachment 8838500 [details] [diff] [review]
Part 5: Hold a pointer of ElementQueue in ReactionsStack instead, v1

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

This part conflicts with bug 1347446. If bug 1347446 get landed first, this part needs a rebase.

Comment 39

[Edgar Chen [:edgar]]

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

Comment 40

[Edgar Chen [:edgar]]

Attachment: Part 5: Hold a pointer of ElementQueue in ReactionsStack instead, v2

Rebase over bug 1347446.

Comment 41

[Edgar Chen [:edgar]]

Attachment: Part 5: Hold a pointer of ElementQueue in ReactionsStack instead, v2

Wrong patch.

Comment 42

[Edgar Chen [:edgar]]

Attachment: Part 6: Put the reaction queue in CustomElementData structure instead of using a map, v1

Comment 43

[Edgar Chen [:edgar]]

Attachment: Part 7-1: Make the constructor created by document.registerElement() also works with construction stack, v1

Comment 44

[Edgar Chen [:edgar]]

Attachment: Part 7-2: Implement new upgrade steps, v4

Comment 45

[Edgar Chen [:edgar]]

Attachment: Part 7-2: Implement new upgrade steps, v5

Comment 46

[Edgar Chen [:edgar]]

Attachment: Part 8: Add wpt test for reporting an exception thrown by a custom element constructor when upgrading, v2

Comment 47

[Edgar Chen [:edgar]]

Attachment: Part 9: Add tests for Xray case, v2

Comment 48

[Edgar Chen [:edgar]]

Attachment: Part 10: Add tests for AutoEntryScript setup, v1

Comment 49

[Edgar Chen [:edgar]]

Comment on attachment 8850814 [details] [diff] [review]
Part 5: Hold a pointer of ElementQueue in ReactionsStack instead, v2

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

It is possible that invoking a reaction triggers pushing a new ElementQueue into ReactionStack
(e.g., calling define() in constructor which probably enqueue another upgrade reaction),
and the reference of ElementQueue passed to InvokeReactions becomes invalid due to the memmove
in nsTArray implementation. I hit such problem when running http://w3c-test.org/custom-elements/CustomElementRegistry.html test.

Comment 50

[Edgar Chen [:edgar]]

Comment on attachment 8850815 [details] [diff] [review]
Part 6: Put the reaction queue in CustomElementData structure instead of using a map, v1

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

Upgrade steps might interact with ReactionQueue (see "Empty element's custom element reaction queue" in step 7 of https://html.spec.whatwg.org/multipage/scripting.html#upgrades).
And bug 1347446 makes getting ElementReactionQueue becomes a bit non-trival, have to get it via DocGroup or pass the ElementReactionQueue to Upgrade().
Since part 2 already refactored the creation time of CustomElementData, it is safe to put ElementReactionQueue into CustomElementData. Then we can just get ReactionQueue from the Element.

Comment 51

[Edgar Chen [:edgar]]

Comment on attachment 8850824 [details] [diff] [review]
Part 7-1: Make the constructor created by document.registerElement() also works with construction stack, v1

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

This part makes the constructor created by document.registerElement() also works with construction stack (more specifically, get the candidate element and do prototype swizzling).
So that old api can also share the new upgrade steps (which will be implemented in part 7-2).

Comment 52

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #35)
> Oh, and once the spec clarifies this stuff (or if it's already clear), you
> should double-check which exact object should be passed to the
> AutoEntryScript.  Please write some testcases testing the entry settings
> when "constructor" is not from mWindow.

Hi bz, could you help to review construct/script_setting stuff and the relevant test

1. Part 7-2 Implement new upgrade steps, v5 (Attachment 8850875 [details] [diff])

   - Per https://github.com/heycam/webidl/pull/328, calling constructor will have the same script object setup as invoking 
     a callback function. The Function passed from binding code have already stored the current incumbent global and using 
     CallSetup will do the right thing. But I am not sure if it is a good idea to make all callback type support construct
     since it seems the only use case is custom element for now. So in this version, I introduce the CustomElementConstructor 
     which inherits CallbackFunction and supports construct, instead of modifying CodeGen.
   - Also addressed comment #37.

2. Part 10: Add tests for AutoEntryScript setup, v1 (Attachment 8850890 [details] [diff])

   - Test the entry script when definition is not from mWindow.
   - And it requires an new API to get current entry global.

Comment 53

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8838499 [details] [diff] [review]
Part 4: Remove unused argumenet in UpgradeCandidates, v1

"argument" in the commit message, please.

r=me fwiw.

Comment 54

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8850875 [details] [diff] [review]
Part 7-2: Implement new upgrade steps, v5

>+    JSContext* scx = s.GetContext();

"cx", please, for the variable name.

>+  if (!JS_WrapObject(aCx, aResult)) {

How much do we care about performance here?  JS_WrapObject is rather slow... but we have tons of other overhead here too.   Might be worth writing some same-compartment performance tests (for which the JS_WrapObject would be a no-op anyway) and timing with and without this JS_WrapObject call to check whether it's noticeable or not.

>+} // namespace anonymous

 } // anonymous namespace

>+  // For example, if |C.customsElement.define('x-foo', FooElement)| triggers

I expect "customsElement" is a typo.  Should be "customElements", right?

>+  // upgrading synchronously, the exception should be report to window |C|.

"should be reported".

>+  nsIGlobalObject* global = GetCurrentGlobal();

This seems slightly weird.  In particular, this gives you slightly weird action-at-a-distance when the element is created via some random DOM API like innerHTML set.

What behavior does the spec define here?  Does it define behavior at all?

>   AutoJSAPI jsapi;

So the only purpose of this AutoJSAPI is to report exceptions via error events?

Is there a reason we wouldn't just do that to the global of the constructor instead?  Again, what does the spec define?

>+  DoCustomElementUpgrade(cx, aElement, aDefinition->mConstructor, rv);

No, this doesn't make sense.  DoCustomElementUpgrade assumes "cx" is in a compartment (see the JS_WrapObject call at the end).  But if !global, then it will NOT be in a compartment here.  Was this !global codepath tested?

Having the CustomElementConstructor class here seems reasonable.

We're not preserving the async stack of the passed-in Function, but preserving that is a tad hard because bindings won't set it up until we return from the define() call.  It's probably OK to file a followup to think about how best to do this.

Comment 55

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8850824 [details] [diff] [review]
Part 7-1: Make the constructor created by document.registerElement() also works with construction stack, v1

It worries me that the logic around setting the prototype here differs from the logic in part 3.  Why does it differ?

Comment 56

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8850890 [details] [diff] [review]
Part 10: Add tests for AutoEntryScript setup, v1

I'd rather we didn't add a getEntryGlobal.  Not least because it may not match with what's web-observable.

Much better would be to use something web-observable here.  Specifically, either "which window's onerror gets called for exceptions?" (not specced to be entry global, but that's what it is in Gecko) or use of some API that actually uses the entry global.

Comment 57

[Boris Zbarsky [:bzbarsky]]

One other request.  When all the review bits are done, but before you land, I would really like to see a single roll-up patch (plus corresponding codegen) attached to this bug.  There have been a bunch of patches here, with many iterations across multiple reviewers, and I'm not very confident that I'd notice issues arising from interactions between them.  Reading over the final code all together would help with that.

Comment 58

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #54)
> Comment on attachment 8850875 [details] [diff] [review]
> Part 7-2: Implement new upgrade steps, v5
> 
> >+  if (!JS_WrapObject(aCx, aResult)) {
> 
> How much do we care about performance here?  JS_WrapObject is rather slow ...
> but we have tons of other overhead here too.   Might be worth writing some
> same-compartment performance tests (for which the JS_WrapObject would be a
> no-op anyway) and timing with and without this JS_WrapObject call to check
> whether it's noticeable or not.

Will test it and update the result.

> >+  // For example, if |C.customsElement.define('x-foo', FooElement)| triggers
> 
> I expect "customsElement" is a typo.  Should be "customElements", right?

Yes, it is typo.

> >+  nsIGlobalObject* global = GetCurrentGlobal();
> 
> This seems slightly weird.  In particular, this gives you slightly weird
> action-at-a-distance when the element is created via some random DOM API
> like innerHTML set.
> 
> What behavior does the spec define here?  Does it define behavior at all?
> 
> >   AutoJSAPI jsapi;
> 
> So the only purpose of this AutoJSAPI is to report exceptions via error
> events?

Yes, it is for reporting exceptions via error events.

> Is there a reason we wouldn't just do that to the global of the constructor
> instead?  Again, what does the spec define?

For the API that annotated with [CEReaction], custom element reactions will be invoked after executing the listed actions [1]. And the exception thrown by constructor or upgrade steps (e.g., 7.2 of [2]) will be caught and report the exception in [3]. And [4] says "using the global object specified by the script's settings object as the target". I thought the target will be the global object of the API who triggers the upgrade actions. Do I have that right?

[1] https://html.spec.whatwg.org/multipage/scripting.html#cereactions
[2] https://html.spec.whatwg.org/multipage/scripting.html#concept-upgrade-an-element
[3] https://html.spec.whatwg.org/multipage/scripting.html#invoke-custom-element-reactions
[4] https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception

> >+  DoCustomElementUpgrade(cx, aElement, aDefinition->mConstructor, rv);
> 
> No, this doesn't make sense.  DoCustomElementUpgrade assumes "cx" is in a
> compartment (see the JS_WrapObject call at the end).  But if !global, then
> it will NOT be in a compartment here.  Was this !global codepath tested?

No, the !global codepath wasn't tested since what we have implemented for now doesn't have a case that runs upgrade from backup reaction queue. But it definitely is an issue need to fix.

Thanks for the review!

Comment 59

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #55)
> Comment on attachment 8850824 [details] [diff] [review]
> Part 7-1: Make the constructor created by document.registerElement() also
> works with construction stack, v1
> 
> It worries me that the logic around setting the prototype here differs from
> the logic in part 3.  Why does it differ?

This is what old upgrade behaves right now to support defining a custom element in a FxOS addon (bug 1130028). And all the old implementation will be removed at some point (probably once we move all things to the new spec), so I just leave the old upgrade as what it behaves now.

Comment 60

[Edgar Chen [:edgar]]

Attachment: bug_1299363_performance.html

(In reply to Edgar Chen [:edgar] from comment #58)
> (In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no
> > Might be worth writing some same-compartment performance tests (for which the JS_WrapObject would be a
> > no-op anyway) and timing with and without this JS_WrapObject call to check
> > whether it's noticeable or not.
> 
> Will test it and update the result.

Here is the test result:
- OS: Ubuntu 16.10
- Machine: Desktop, Intel(R) Core(TM) i7-3770 @ 3.40GHz, Memory 16G
- Build: pgo
- Rev: based on f9362554866b
- Test script: in the attachment
- With JS_WrapObject call: 11090.7
- Without JS_WrapObject call: 10900

(custom elements needs classes, so there is no baseline and ion support.)

Comment 61

[Boris Zbarsky [:bzbarsky]]

> (custom elements needs classes, so there is no baseline and ion support.)

Sure there is.  Just put the code you're testing into a separate compilation unit (aka "function") from the class definition.  Most simply, use an IIFE like so:

  (function() {
    var count = 100000;
    ....
    document.writeln((end - start)/count * 1e6);
   })();

Do you mind remeasuring with that in place, so the JS execution overhead is closer to what it would be in real life?

I still need to get back to the rest of you responses; should be able to do that in a few hours.

Comment 62

[Edgar Chen [:edgar]]

Attachment: bug_1299363_performance.html

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #61)
>
> Do you mind remeasuring with that in place, so the JS execution overhead is
> closer to what it would be in real life?

Test result of using IIFE:
- OS: Ubuntu 16.10
- Machine: Desktop, Intel(R) Core(TM) i7-3770 @ 3.40GHz, Memory 16G
- Build: pgo
- Rev: based on f9362554866b
- Test script: in the attachment
- With JS_WrapObject call: 7418.92
- Without JS_WrapObject call: 6978.85

Comment 63

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #58)
> No, the !global codepath wasn't tested since what we have implemented for
> now doesn't have a case that runs upgrade from backup reaction queue. But it
> definitely is an issue need to fix.

After reviewing the all possible cases that may trigger upgrade steps in spec again (and also apply some local WIP patches to test),
- customElements.define: https://html.spec.whatwg.org/multipage/scripting.html#dom-customelementregistry-define
- document.createElement: https://dom.spec.whatwg.org/#dom-document-createelement
- document.createElementNS: https://dom.spec.whatwg.org/#dom-document-createelementns
- document.importNode: https://dom.spec.whatwg.org/#dom-document-importnode
- document.write: https://html.spec.whatwg.org/multipage/webappapis.html#dom-document-write
- document.writeln: https://html.spec.whatwg.org/multipage/webappapis.html#dom-document-writeln
- Element.innerHTML: https://w3c.github.io/DOM-Parsing/#dom-element-innerhtml
- Node.clone: https://dom.spec.whatwg.org/#concept-node-clone
- create element from parser: https://html.spec.whatwg.org/multipage/syntax.html#create-an-element-for-the-token

Most of API annotated with [CEReactions], excepts
- document.createElement[NS] runs upgrade steps synchronously, instead of interacting with reaction queue.
- parser bit has its own ReactionsStack push/pop.

So the comment isn't correct because there is no such case that runs upgrade steps in Backup Queue. But the current global still could possible be nullptr if the upgrade is from the parser bit and will crash in JS_WrapObject call ...

Comment 64

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #58)
> For the API that annotated with [CEReaction], custom element reactions will
> be invoked after executing the listed actions [1]. And the exception thrown
> by constructor or upgrade steps (e.g., 7.2 of [2]) will be caught and report
> the exception in [3]. And [4] says "using the global object specified by the
> script's settings object as the target". I thought the target will be the
> global object of the API who triggers the upgrade actions. Do I have that
> right?
> 
> [1] https://html.spec.whatwg.org/multipage/scripting.html#cereactions
> [2]
> https://html.spec.whatwg.org/multipage/scripting.html#concept-upgrade-an-
> element
> [3]
> https://html.spec.whatwg.org/multipage/scripting.html#invoke-custom-element-
> reactions
> [4]
> https://html.spec.whatwg.org/multipage/webappapis.html#report-the-exception

Ah, I just realized there are some discussions about "report an exception".
- w3c/webcomponents#547
- whatwg/html#958.

Comment 65

[Boris Zbarsky [:bzbarsky]]

Sorry for the lag here; I was at a work week most of this week.

> will be caught and report the exception in [3]
>[3] https://html.spec.whatwg.org/multipage/scripting.html#invoke-custom-element-reactions

This part of the spec is totally broken.  "report an expection" needs a "script" and assumes it's called with a "script" around, and this bit doesn't have a "script".

There's a longstanding spec issue about this.  See https://github.com/whatwg/html/issues/958 ... which I see you found.

Please file a spec issue and talk to domenic about making the spec make sense.  He had a plan in https://github.com/whatwg/html/issues/958#issuecomment-226309871 which involved using Web IDL callbacks for custom element reactions, iirc....

For our purposes, what this means is that you should probably eRethrowExceptions in your CallSetup when calling the constructor, because I believe that would most closely mirror what I understand of Domenic's proposed spec.  But again, you should check that with Domenic.

I'm not entirely sure what the means in terms of the NS_ERROR_DOM_INVALID_STATE_ERR that gets thrown if the constructor returns something that is not aElement.  Need to get that sorted out in terms of what global it uses.

Once we know what actual error-reporting behavior we want here, we can figure out what the code should look like...

> This is what old upgrade behaves right now to support defining a custom element in a FxOS addon

OK.  I think we should have a comment in the code explaining that, so someone doesn't come along and fix the two places to act the same.

That said, I think I'm still missing something.  I see nothing in the fix for bug 1130028 that does SubsumesConsideringDomain, for example.  And nothing being removed in the 7-1 patch that looks like the code being added.  What am I missing?

>Test result of using IIFE:
>- With JS_WrapObject call: 7418.92
>- Without JS_WrapObject call: 6978.85

This is ... literally 10x slower than I am seeing in my nightly (on Mac), if I just comment out the "customElements.define" call.  I assume my hardware is not 10x slower than mine.

What numbers do you get if you comment out the customElements.define() bit?

Comment 66

[William Chen [:wchen]]

Comment on attachment 8839340 [details] [diff] [review]
Part 1: Implement construction stack, v4

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

::: dom/bindings/BindingUtils.cpp
@@ +3530,5 @@
> +    return nullptr;
> +  }
> +
> +  // Step 11 is in the code output by CGClassConstructor.
> +  // Step 12 and Step 13.

For step 12 of the HTML element constructor we are supposed to replace the last entry with ALREADY_CONSTRUCTED_MARKER but where is that happening?

Comment 67

[William Chen [:wchen]]

Comment on attachment 8838497 [details] [diff] [review]
Part 2: Implement custom element state, v2

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

::: dom/base/CustomElementRegistry.h
@@ +74,5 @@
> +  // https://dom.spec.whatwg.org/#concept-element-custom-element-state
> +  // CustomElementData is only created on the element which is a custom element
> +  // or an upgrade candidate, so the state of an element without
> +  // CustomElementData is "uncustomized".
> +  enum State {

enum class

Comment 68

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #65)
> Please file a spec issue and talk to domenic about making the spec make
> sense.  
Will file a spec issue and post the link here.

> OK.  I think we should have a comment in the code explaining that, so
> someone doesn't come along and fix the two places to act the same.
Will do.

> That said, I think I'm still missing something.  I see nothing in the fix
> for bug 1130028 that does SubsumesConsideringDomain, for example.  And
> nothing being removed in the 7-1 patch that looks like the code being added.
> What am I missing?
The added code is from [1] which is removed and replaced by new upgrade steps in the 7-2 patch.  I will try to make patch clearer in next version of patch (maybe move the removement of [1] to the 7-1 patch).

There are two places doing prototype setup in old upgrade,
- If definition comes after JS reflector creation, CustomElementRegistry::Upgrade will do prototype swizzling. 
- If definition comes before JS reflector creation, Element::WrapObject will set up the prototype. 

The later one does SubsumesConsideringDomain, but the former doesn't not. I will file a separated bug to fix the inconsistency first (i.e. the former case should also do SubsumesConsideringDomain).

[1] http://searchfox.org/mozilla-central/rev/381a7b8f8a78b481336cfa384cb0a5536e217e4a/dom/base/CustomElementRegistry.cpp#882-895

> What numbers do you get if you comment out the customElements.define() bit?

The number is 593.3 with the same environment in comment #62.

Comment 69

[Edgar Chen [:edgar]]

(In reply to William Chen [:wchen] from comment #66)
> For step 12 of the HTML element constructor we are supposed to replace the
> last entry with ALREADY_CONSTRUCTED_MARKER but where is that happening?


"element.forget()" clears the last element of construction stack to nullptr.
I guess I should add more comments to make it clearer.

Comment 70

[Edgar Chen [:edgar]]

Comment on attachment 8850824 [details] [diff] [review]
Part 7-1: Make the constructor created by document.registerElement() also works with construction stack, v1

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

Cancel review request per comment #68 and bug 1353647.

Comment 71

[William Chen [:wchen]]

Comment on attachment 8839340 [details] [diff] [review]
Part 1: Implement construction stack, v4

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

(In reply to Edgar Chen [:edgar] from comment #69)
> (In reply to William Chen [:wchen] from comment #66)
> > For step 12 of the HTML element constructor we are supposed to replace the
> > last entry with ALREADY_CONSTRUCTED_MARKER but where is that happening?
> 
> "element.forget()" clears the last element of construction stack to nullptr.
> I guess I should add more comments to make it clearer.
Ah right. Yes, please add a comment.

Comment 72

[Boris Zbarsky [:bzbarsky]]

> The number is 593.3 with the same environment in comment #62.

OK.  So ~600 without the define() call, but ~7000 with it?  And the JS_WrapObject() overhead is comparable to the entire time without the define() call?

That's a pretty significant slowdown.  What does the time look like if you don't pass the { is: "x-foo" } arg, or pass { is: "x-bar" } instead?

Comment 73

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #72)
> OK.  So ~600 without the define() call, but ~7000 with it?  And the
> JS_WrapObject() overhead is comparable to the entire time without the
> define() call?

No, it is not comparable. Without define() call, createElement() won't run upgrade steps to upgrade the element.

> That's a pretty significant slowdown. What does the time look like if you
> don't pass the { is: "x-foo" } arg, or pass { is: "x-bar" } instead?

Actually the result of comment #68 is already removing the { is: "x-foo" } arg.
Passing { is: ... } arg to createElement() without relevant define() call will get NotFoundError (if custom element feature is pref-ed on). Hmm, I found the spec changes to NOT throw NotFoundError, see w3c/webcomponents#608 and filed bug 1354013. But I believe even bug 1354013 is fixed, the result of passing { is: "x-bar" } will still be ~600 since it doesn't run any upgrade steps.

Comment 74

[Boris Zbarsky [:bzbarsky]]

> No, it is not comparable

How is it not comparable?  The relevant numbers from the above comments are are:

Without define(): 593.3
With define(), without JS_WrapObject: 6978.85
With define(), with JS_WrapObject: 7418.92

So the JS_WrapObject takes about 420 of whatever time units are involved here, whereas an entire createElement call normally takes about 600.  That means that the JS_WrapObject overhead is really not all that acceptable.  But neither is any of the other stuff that makes thins take 10x slower.  Or at the very least it should be viewed with deep suspicion, profiled, and optimized.

> the result of passing { is: "x-bar" } will still be ~600 since it doesn't run any upgrade steps

Do you believe this based on measurement, or just based on a guess?

Ignoring the JS_WrapObject bit for a moment, it seems to me that we need to measure the following numbers:

1)  createElement without these changes.
2)  createElement with these changes, without any define() calls, with one arg.
    This number is 593.3, right?
3)  createElement with these changes, without any define() calls, with a second arg
    that does not include "is".
4)  createElement with these changes, without any define() calls, with a second arg
    that includes "is" (if this does not throw).
5)  createElement with these changes, with a define() call, with one arg.
5)  createElement with these changes, with a define() call, with a second arg
    that does not include "is".
6)  createElement with these changes, with a define() call, with a second arg
    that includes "is" that does not match the define() (if this does not throw).
7)  createElement with these changes, with a define() call, with a second arg
    that includes "is" that matches the define().  This number is around 7000,
    depending on what happens with JS_WrapObject, right?

That will give us an idea of how much slowdown we have in various use cases, and hence how worried we should be about turning this on affecting code that doesn't use custom elements at all, for example.  Based on that, we decide whether we can land this at all without more performance work.  If the slowness is restricted to cases when "is" is set and matches a registration, I'm ok with a followup to profile and optimize that we commit to (this bug is already getting pretty big and unwieldy).  The JS_WrapObject discussion could then be deferred to that optimization bug for now.  If, on the other hand, we're slowing down things badly across the board just by creating a definition even if no one uses it, then that's bad.  We need data.

Comment 75

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #74)
> Do you believe this based on measurement, or just based on a guess?
based on a guess, and following numbers show I was wrong...

> 1)  createElement without these changes.
486.765

> 2)  createElement with these changes, without any define() calls, with one
> arg.
>     This number is 593.3, right?
I test this again and get 617.355 this time.

> 3)  createElement with these changes, without any define() calls, with a
> second arg
>     that does not include "is".
815.355

> 4)  createElement with these changes, without any define() calls, with a
> second arg
>     that includes "is" (if this does not throw).
3205.095

> 5)  createElement with these changes, with a define() call, with one arg.
697.81

> 5)  createElement with these changes, with a define() call, with a second arg
>     that does not include "is".
817.365

> 6)  createElement with these changes, with a define() call, with a second arg
>     that includes "is" that does not match the define() (if this does not
> throw).
3172.795

> 7)  createElement with these changes, with a define() call, with a second arg
>     that includes "is" that matches the define().  This number is around
> 7000,
>     depending on what happens with JS_WrapObject, right?
7736.37 (with JS_WrapObject).

I also test the pref-off cases,
1). pref-off, createElement without these changes, without any define() calls, with one arg
400.28
2). pref-off, createElement without these changes, without any define() calls, with a second arg that does not include "is". 
594.075
3). pref-off, createElement without these changes, without any define() calls, with a second arg that includes "is".
782.065
4). pref-off, createElement with these changes, without any define() calls, with one arg
623.93
5). pref-off, createElement without these changes, without any define() calls, with a second arg that does not include "is". 
827.96
5). pref-off, createElement with these changes, without any define() calls, with a second arg that includes "is" (if this does not throw).
1029.9

Detailed result:
https://docs.google.com/a/mozilla.com/spreadsheets/d/1-hSfFsra4H3cSm18oKsl_kkLNkVUNqIcUb1yQVxJzhc/edit?usp=sharing

Comment 76

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #68)
 > Will file a spec issue and post the link here.

https://github.com/w3c/webcomponents/issues/635

Comment 77

[Boris Zbarsky [:bzbarsky]]

I'm not sure which pref "the pref-off cases" refers to, but here's my take on those numbers:

* The normal case (createElement, with these changes, no define() calls, one arg) is a noticeable slowdown over the "without these changes"  case.  That needs to be profiled, understood, and fixed.

* The nearly-normal case (createElement, with the changes, define() call, one arg), is also much slower than what we have now.  Needs to be profiled, understood, and fixed: the one-arg case should have no performance penalty, I would think.

* We should measure where the overhead comes from in the "have second arg with is=, no define() calls" case that's measuring ~3200 in the numbers above.  Seems a bit fishy that it would be that high.

* We should measure where the additional overhead comes from in the "have second arg with is= that matches a definition" case.  I can think of various sources, some of which already have bugs filed on them, but it would be good to see whether there's something non-obvious in there.

Comment 78

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (still a bit busy) (if a patch has no decent message, automatic r-) from comment #77)
> I'm not sure which pref "the pref-off cases" refers to, 

"dom.webcomponents.customelements.enabled"

Filed bug 1357002 for "pref-off, without these changes" case.

> * The normal case (createElement, with these changes, no define() calls, one
> arg) is a noticeable slowdown over the "without these changes"  case.  That
> needs to be profiled, understood, and fixed.

profile: https://perfht.ml/2pMnF8z

LookupCustomElementDefinition() returns nullptr immediately if mCustomDefinitions is empty might help. I will do more tests. And I would like to move the discussion to bug 1301024 since I applied the WIP patch of bug 1301024 for these tests, and the slowdown is from bug 1301024 (it doesn't run upgrade steps at all).

> * The nearly-normal case (createElement, with the changes, define() call,
> one arg), is also much slower than what we have now.  Needs to be profiled,
> understood, and fixed: the one-arg case should have no performance penalty,
> I would think.

profile: https://perfht.ml/2pslKGC

Profile shows some slowdown is from NS_Atomize(), switch to NS_AtomizeMainThread() might help (bug 1351303). Will do more tests.  Same as above, I would like to move the discussion to bug 1301024.

> * We should measure where the overhead comes from in the "have second arg
> with is=, no define() calls" case that's measuring ~3200 in the numbers
> above.  Seems a bit fishy that it would be that high.

profile: https://perfht.ml/2psMtDf

I saw something that we can improve on WIP patch of bug 1301024,
- Using cached the ""dom.webcomponents.customelements.enabled" preference might help.
- Switch to NS_AtomizeMainThread() might help.
- There are duplicate codes calls NS_Atomize() with the same string. We could refactor those functions to take nsIAtom as argument so that we don't need to call NS_Atomize() multiple times.

I will do more tests, but again, I would like to move the discussion to bug 1301024.

> * We should measure where the additional overhead comes from in the "have
> second arg with is= that matches a definition" case.  I can think of various
> sources, some of which already have bugs filed on them, but it would be good
> to see whether there's something non-obvious in there.

Profile: https://perfht.ml/2pu0X8B

At first glance mozilla::dom::CustomElementConstructor::Construct() is slow (446.0ms, 57.9%). And I would like to test again and do more analysis with the improved patch of bug 1301024.

Comment 79

[Boris Zbarsky [:bzbarsky]]

> "dom.webcomponents.customelements.enabled"

I see.  It will be rather hard to avoid slowdown in the two-arg case regardless of that pref value, because the pref doesn't affect dictionary processing.

Arguably that's a bug in this case, because if the pref is off presumably that dictionary should not be processed at all (for correctness, not performance), right?

> profile: https://perfht.ml/2pMnF8z

(For "normal case".)  I don't see how changing anything about LookupCustomElementDefinition() will help here.  If I read the profile right, that function is about 2% of the time under DocumentBinding::createElement.  The regression comment 75 describes is 20% or so, no?

> And I would like to move the discussion to bug 1301024

OK, as long as we don't land a 20% performance regression to the "normal case" in the meantime....

> Profile shows some slowdown is from NS_Atomize()

Is this patch adding NS_Atomize calls?

Basically, the current behavior, the "normal case" post-patch and the "nearly-normal case" post-patch should all have identical performance.  If they don't, we need to know why not and then decide what to do about it.

> profile: https://perfht.ml/2psMtDf

(the "have second arg, no define() calls" case).  This is showing SetAttr calls under CustomElementRegistry::SetupCustomElement.  Why is there any of that stuff happening if there were no define() calls?  Also time under RegisterUnresolvedElement, etc...

> Profile: https://perfht.ml/2pu0X8B

(the "match a definition" case).  This shows a bunch of interpreter time.  It's possible that we don't even JIT class constructors yet.  Please check with the spidermonkey folks.

Past that, the fact that we have to preserve the wrapper is hurting us.  I'm not sure I see a good way around that, though.  

The AutoJSAPI init in CustomElementRegistry::Upgrade is costing us some time and really shouldn't be needed...  

Actually, now that I read the CustomElementConstructor::Construct code more carefully, I don't think it's correct.  It's noting a JSContext exception on the ErrorResult, but that JSContext exception may not outlive the ErrorResult...  and in fact does not; teh CallSetup is told to rethrow it on the ErrorResult and does so.  And I don't think there's any point doing the JS_WrapObject in  CustomElementConstructor::Construct.  In fact, the only consumer wants access to the unwrapped object anyway.

So I think Construct should just do:

  CallSetup s(this, aRv, aExecutionReason, eRethrowExceptions);
  // Check for null GetContext() and such;
  JS::Rooted<JS::Value> constructor(scx, JS::ObjectValue(*mCallback));
  JS::Construct(cx, constructor, JS::HandleValueArray::empty(), aResult);

and that's it.  Neither it nor DoCustomElementUpgrade need a JSContext argument.

In fact, if you make Construct return a JSObject* you don't even need a Rooted in DoCustomElementUpgrade (though you do need it in Construct(), of course, once you have a JSContext there).

Comment 80

[Boris Zbarsky [:bzbarsky]]

Anyway, apart from the correctness issue noted in comment 79, I think we should have fixes for the "normal case" and "nearly-normal case" and can file followups for the other cases...  Those might block turning this stuff on by default, but shouldn't necessarily block this landing.

Comment 81

[Edgar Chen [:edgar]]

Comment on attachment 8850815 [details] [diff] [review]
Part 6: Put the reaction queue in CustomElementData structure instead of using a map, v1

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

This part and part 2 patch block other bugs, so I would like to move them to separated bugs to make other bugs can move forward.

Comment 82

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60dd330d4066286c0a13a176ccae156d5999bf70

Comment 83

[Edgar Chen [:edgar]]

I am going to,
- Use entry global for error reporting of reaction invocation.
- Move the performance discussion to bug 1301024, since the performance regression is from this bug + bug 1301024, landing only this bug won't make createElement slow (This bug doesn't touch the createElement path at all). Of course, I will test to ensure that.
- In bug 1301024, we should fix performance regression for "normal case" and "nearly-normal case". And file bugs for other cases.

Comment 84

[Edgar Chen [:edgar]]

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

Comment 85

[Edgar Chen [:edgar]]

Attachment: Part 1: Implement construction stack, v5, r=wchen

Rebase over bug 1359346.

Comment 86

[Edgar Chen [:edgar]]

Attachment: Part 2: Allow prototype swizzling in html constructor, v5, r=bz

Rebase.

Comment 87

[Edgar Chen [:edgar]]

Attachment: Part 3: Remove unused argumenet in UpgradeCandidates, v2, r=bz,wchen

Rebase

Comment 88

[Edgar Chen [:edgar]]

Attachment: Part 4: Hold a pointer of ElementQueue in ReactionsStack instead, v3

Rebase

Comment 89

[Edgar Chen [:edgar]]

Attachment: Part 3: Remove unused argument in UpgradeCandidates, v3, r=bz,wchen

Address review comment #53,
- s/argumenet/argument/ in commit message.

Comment 90

[Edgar Chen [:edgar]]

Attachment: Part 5-1: Make the constructor created by document.registerElement() also works with construction stack, v2

Address comment #68,
- add more comments.
- Move the removement of old upgrade to this part (the new upgrade steps will be added in part 5-2).

Comment 91

[Edgar Chen [:edgar]]

Attachment: Part 5-2: Implement new upgrade steps, v6

Address review comment #79,
- Use entry global for error reporting.
- Don't use an additional AutoJSAPI for error reporting.
- Correct the CustomElementConstructor::Construct.

Comment 92

[Edgar Chen [:edgar]]

Attachment: Part 6: Add tests for custom element upgrade, v3

Comment 93

[Edgar Chen [:edgar]]

Comment on attachment 8881567 [details] [diff] [review]
Part 4: Hold a pointer of ElementQueue in ReactionsStack instead, v3

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

We need this change because it is possible that invoking a reaction triggers pushing a new ElementQueue into ReactionStack (e.g., calling define() in constructor which probably enqueue another upgrade reaction), and the reference of ElementQueue passed to InvokeReactions becomes invalid due to the memmove in nsTArray implementation. I hit such problem when running http://w3c-test.org/custom-elements/CustomElementRegistry.html test. Hi William, could you help to review? Thank you.

Comment 94

[Edgar Chen [:edgar]]

Comment on attachment 8881591 [details] [diff] [review]
Part 5-2: Implement new upgrade steps, v6

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

In this version, I use the entry global for error reporting. Could you help to take a look again to see if I am doing correct things on error reporting? Thank you.

Comment 95

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8881591 [details] [diff] [review]
Part 5-2: Implement new upgrade steps, v6

>+  CallSetup s(this, aRv, aExecutionReason, eRethrowExceptions);

OK, so this says "set up a call, and if it throws an exception rethrow that exception on aRv when we come off the stack".

>+    aRv.NoteJSContextException(cx);

Which means this bit is not needed: It's telling aRv that there is an exception on the JSContext, but ~CallSetup will move the exception from the JSContext to aRv anyway.  Please take this line out.

The rest of the error reporting bits look good.

>+  return result.get();

Should just be able to "return result;"

Comment 96

[Edgar Chen [:edgar]]

Attachment: Part 5-2: Implement new upgrade steps, v7

Address comment #95.

Comment 97

[Edgar Chen [:edgar]]

Comment on attachment 8882097 [details] [diff] [review]
Part 5-2: Implement new upgrade steps, v7

r? to bz for error reporting.
r? to wchen for upgrade steps.

Thank you.

Comment 98

[Edgar Chen [:edgar]]

Comment on attachment 8881593 [details] [diff] [review]
Part 6: Add tests for custom element upgrade, v3

Add more tests for upgrade especially failure cases. And it is still not clear what error reporting will be looked like in custom element, see https://github.com/w3c/webcomponents/issues/635, so I add them as mochitest test instead of web-platform test.

Comment 99

[Edgar Chen [:edgar]]

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

Comment 100

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8882097 [details] [diff] [review]
Part 5-2: Implement new upgrade steps, v7

>+DoCustomElementUpgrade(Element* aElement,
>+  Element* element;
>+  if (NS_FAILED(UNWRAP_OBJECT(Element, constructResult, element)) ||

This is going to be a problem in the near future.  You have a few options:

1)  Use a RefPtr if you think that's not a performance problem here.
2)  Pass in the JSContext from the one caller of this function and make
    "constructResult" a Rooted<JSObject*>.
3)  File a bug on me to fix this up and mark it blocking bug 1371259
    if this bit is super performance-sensitive (which I don't think it is,
    based on the caller and whatnot).

Option 2 is probably best.  Sorry for the vagueness here.  :(

> CustomElementReactionsStack::InvokeReactions(ElementQueue* aElementQueue)
>+  nsIGlobalObject* global = GetEntryGlobal();

Why do we think there is one?  Like the comments say, there may not be one.  What if there isn't one?

More to the point, error reporting happens on the entry global once the stack has unwound to the entry point.  In this case, we're not unwinding the stack, so there's no reason the entry global would be the right thing at all.

>+  AutoJSContext cx;

Please don't add new uses of AutoJSContext with it's "yeah, guess at something" behavior.  We want to be very specific here, in terms of which compartments we're in, if any and whatnot.  AutoJSContext most emphatically does not do that for you.

If you plan to call into JS and report exceptions before returning from here, please use AutoEntryScript.  If you want to use the existing entry global (which is not clear to me per above), init the AutoEntryScript with it.  But please don't invent new exception-handling bits like nsContentUtils::ReportException.  AutoEntryScript will report exceptions for you as it comes off the stack.  It will correctly handle compartments in the process and so forth.  I didn't read the details of the nsContentUtils::ReportException code for the moment, because I don't think we need it.

It's also a bit weird that you start with a JSContext, then you call into Upgrade, where you forget the JSContext for the call into the Construct bit, etc.  Why not just do the normal thing we do: The CallSetup reports exceptions, does that on the entry global of the call it makes, etc.  You're having the CallSetup rethrow the exception but then catching and reporting it anyway...

So it's probably better to nix this GetEntryGlobal thing, nix the AutoJSContext, not pass a JSContext to Invoke() or Upgrade(), have Construct just report via the CallSetup as usual.  That makes it harder to do the Rooted suggestion for constructResult above, but I think the RefPtr thing should be fine anyway.

>+++ b/testing/web-platform/meta/custom-elements/upgrading/Node-cloneNode.html.ini
>+  expected: ERROR

Why?  Is this tracked by a bug?

I'm gone this next week, but I'll make reviewing updated patches here my top priority when I get back July 10.  Alternately, if William is happy with the patches addressing my comments above it's ok to land them with just his review and I can take a look when I get back and will file a followup if there is a remaining problem.

Comment 101

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] (vacation until 7/7) from comment #100)
> Comment on attachment 8882097 [details] [diff] [review]
> Part 5-2: Implement new upgrade steps, v7
> 
> > CustomElementReactionsStack::InvokeReactions(ElementQueue* aElementQueue)
> >+  nsIGlobalObject* global = GetEntryGlobal();
> 
> Why do we think there is one?  Like the comments say, there may not be one. 
> What if there isn't one?

If reaction invocation is from parser [1], I am going to use the node document's global object per https://github.com/w3c/webcomponents/issues/635#issuecomment-297516487.
And if the invocation is from backup-queue (in microtask checkpoint), I think we could just suppress the exception. But right now, above cases won't happen until bug 1378079 and bug 1315885, so I add the assertion for now and plan to fix it separately. Does it make sense to you?

[1] step 9.2 of https://html.spec.whatwg.org/multipage/parsing.html#create-an-element-for-the-token

> >+  AutoJSContext cx;
> 
> Please don't add new uses of AutoJSContext with it's "yeah, guess at
> something" behavior.
Will use AutoEntryScript instead.

> Why not just do the normal thing we do: The CallSetup reports
> exceptions, does that on the entry global of the call it makes, etc.  You're
> having the CallSetup rethrow the exception but then catching and reporting
> it anyway...

It is because spec says to rethrow the exception in step 7 of https://html.spec.whatwg.org/multipage/custom-elements.html#invoke-custom-element-reactions and report the exception in https://html.spec.whatwg.org/multipage/custom-elements.html#invoke-custom-element-reactions. Is it okay that we just let CallSetup handle the error reporting (which use the entry global of the constructor call, instead of the entry global of reaction invocation)?

And even if we let CallSetup handle the error reporting for Constructor call, we still need report other errors in reaction invocation.

> So it's probably better to nix this GetEntryGlobal thing, nix the
> AutoJSContext, not pass a JSContext to Invoke() or Upgrade(), have Construct
> just report via the CallSetup as usual.  That makes it harder to do the
> Rooted suggestion for constructResult above, but I think the RefPtr thing
> should be fine anyway.

Just want to make sure my understanding is correct, do you mean something like,
====
JSObject* constructResult = aConstructor->Construct("Custom Element Upgrade", aRv);
...

RefPtr<Element> element;
if (NS_FAILED(UNWRAP_OBJECT(Element, constructResult, element)) || ...) {
  ...
}
=====

> >+++ b/testing/web-platform/meta/custom-elements/upgrading/Node-cloneNode.html.ini
> >+  expected: ERROR
> 
> Why?  Is this tracked by a bug?

It is because we report the exception to the entry global and the test has a different expectation. But the error reporting is not clear in spec [2], file bug 1378079 to track.

[2] https://github.com/w3c/webcomponents/issues/635

Comment 102

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar] from comment #101)
> file bug 1378079 to track.
       ^^^^^^^^^^^
Er, it should be bug 1378265.

Comment 103

[William Chen [:wchen]]

Comment on attachment 8881584 [details] [diff] [review]
Part 5-1: Make the constructor created by document.registerElement() also works with construction stack, v2

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

::: dom/base/nsDocument.cpp
@@ +6038,5 @@
> +  if (constructionStack.Length()) {
> +    element = constructionStack.LastElement();
> +    NS_ENSURE_TRUE(element != ALEADY_CONSTRUCTED_MARKER, false);
> +
> +    // Do prototype swizzling if dom reflector is existed.

nit: is existed -> exists

@@ +6082,5 @@
> +                                                                : nullptr);
> +    NS_ENSURE_TRUE(element, false);
> +  }
> +
> +  // The prototype setup is happened in Element::WrapObject().

nit: is happened -> happens

Comment 104

[Edgar Chen [:edgar]]

Attachment: Part 5-1: Make the constructor created by document.registerElement() also works with construction stack, v3, r=wchen

Address review comment #103.

Comment 105

[William Chen [:wchen]]

Comment on attachment 8882097 [details] [diff] [review]
Part 5-2: Implement new upgrade steps, v7

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

The upgrade steps look good to me, r+ for those parts.

Comment 106

[William Chen [:wchen]]

Comment on attachment 8881593 [details] [diff] [review]
Part 6: Add tests for custom element upgrade, v3

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

Can we also add a test for a non-conformant constructor that doesn't call super()?

Comment 107

[Edgar Chen [:edgar]]

ni? for comment #101.

Comment 108

[Boris Zbarsky [:bzbarsky]]

Still catching up on email, but...

> I am going to use the node document's global object per https://github.com/w3c/webcomponents/issues/635#issuecomment-297516487

It's still not clear to me why the spec is not just using the "guarded" flag here.  Why is that, exactly?  Put another way, why does this not just behave like just about every other callback?  It seems to me like it really should, ideally, and if the spec doesn't do things that way, that's a bug in the spec.

> I think we could just suppress the exception

I would think silently suppressing exceptions is never ok.

> It is because spec says to rethrow the exception in step 7

Hmm.  Rethrow to where?  What things can end up invoking "upgrade an element"?

> Is it okay that we just let CallSetup handle the error reporting (which use the entry global of the
> constructor call, instead of the entry global of reaction invocation)?

Let's back up a second and look at the actual observable behavior.  Does the spec somewhere describe, or can you describe, the actual observable behaviors here?  Is there any actual rethrowing to script at any point, or is the only rethrowing to the "report" bit in https://html.spec.whatwg.org/multipage/custom-elements.html#invoke-custom-element-reactions ?

Because in the latter case (as in, all exceptions are non-escaping), it's not observably different to report at the point when we are told to "rethrow", as long as we propagate out the fact that it threw and skip any steps we need to skip.  Which is entirely unclear from the spec, actually.  Specifically, looking at https://html.spec.whatwg.org/multipage/custom-elements.html#invoke-custom-element-reactions if there are multiple reactions in the per-element queue and the first one throws, are the later ones invoked?  Are reactions for other elements in the element queue invoked?  If the answers to those questions are different, why?

Please make sure spec issues are filed to get all this clarified as needed.

> we still need report other errors in reaction invocation.

Is there something here that can throw other than the SameValue check failing in https://html.spec.whatwg.org/multipage/custom-elements.html#concept-upgrade-an-element step 7.2?

But yes, I agree that it's not quite obvious what global should be used for that error report...

>RefPtr<Element> element;
>if (NS_FAILED(UNWRAP_OBJECT(Element, constructResult, element)) || ...) {

Yes, exactly.

Note that at this point you really will need to do that; the code you had before (passing a JSObject* and unwrapping into an Element*) will not compile as of yesterday afternoon.

> and the test has a different expectation

I don't see how the test can have an expectation at all, since this is totally undefined in the spec.  Please file a bug on the test to stop testing things that aren't specced and cc Domenic on that.

Comment 109

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] from comment #108)
> Hmm.  Rethrow to where?  What things can end up invoking "upgrade an
> element"?

1), The webidl operation/attribute having CEReaction annotation [1],
    and if the operation/attribute will enqueue upgrade reaction, e.g. customElement.define(),
    upgrade reaction will be invoked after executing the listed actions.
    - step 15 of https://html.spec.whatwg.org/multipage/custom-elements.html#dom-customelementregistry-define
    - https://html.spec.whatwg.org/multipage/custom-elements.html#invoke-custom-element-reaction
2), Creating an element, e.g. document.createElement()
    - step 5 of https://dom.spec.whatwg.org/#dom-document-createelement
    - step 5.3 of https://dom.spec.whatwg.org/#concept-create-element

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

> Is there any actual rethrowing to script at any point, or
> is the only rethrowing to the "report" bit in
> https://html.spec.whatwg.org/multipage/custom-elements.html#invoke-custom-
> element-reactions ?

For 2) above, it rethrows to script since https://dom.spec.whatwg.org/#concept-create-element doesn't handle the exception (if my understanding is correct).

> Specifically, looking at
> https://html.spec.whatwg.org/multipage/custom-elements.html#invoke-custom-
> element-reactions if there are multiple reactions in the per-element queue
> and the first one throws, are the later ones invoked?  Are reactions for
> other elements in the element queue invoked?  If the answers to those
> questions are different, why?

I think both remaining reactions and other elements in the element queue will still be invoked. 

> Is there something here that can throw other than the SameValue check
> failing in
> https://html.spec.whatwg.org/multipage/custom-elements.html#concept-upgrade-
> an-element step 7.2?

No, only the SameValue check.
 
> Please file a bug on the test to stop
> testing things that aren't specced and cc Domenic on that.

Filed https://github.com/w3c/web-platform-tests/issues/6534.

Comment 110

[Boris Zbarsky [:bzbarsky]]

>    upgrade reaction will be invoked after executing the listed actions.

OK.  But in this case it will actually be reported by https://html.spec.whatwg.org/multipage/custom-elements.html#invoke-custom-element-reactions before it returns, right?  So we could just report it ourselves so somewhere in here, as long as we do it after all other possibly-observable things we do.

> 2), Creating an element, e.g. document.createElement()

OK.  This one looks like it might actually throw exceptions from the upgrade back out to the caller of createElement and not return the created element.  Sounds like your reading agrees with mine.  I think it's worth documenting this around where we use the rethrowing CallSetup.  Thank you for explaining!

Given that, are you still reasonably clear on what we want the code to look like here, or should I go back through it and see what the various codepaths look like?

> I think both remaining reactions and other elements in the element queue will still be invoked. 

OK.  That is entirely unclear to me from the spec; please file an issue to clarify...

Comment 111

[Boris Zbarsky [:bzbarsky]]

Oh, and I assume there are tests for createElement rethrowing the exact exception that was thrown from the upgrade?

Comment 112

[Edgar Chen [:edgar]]

(In reply to Boris Zbarsky [:bz] from comment #110)
> Given that, are you still reasonably clear on what we want the code to look
> like here, or should I go back through it and see what the various codepaths
> look like?

Yes, I think it is clear on what we want the code to look like. I will prepare another draft.
And will need your help to review again. Thank you!!

> 
> > I think both remaining reactions and other elements in the element queue will still be invoked. 
> 
> OK.  That is entirely unclear to me from the spec; please file an issue to
> clarify...

Filed https://github.com/whatwg/html/issues/2842

(In reply to Boris Zbarsky [:bz] from comment #111)
> Oh, and I assume there are tests for createElement rethrowing the exact
> exception that was thrown from the upgrade?

web-platform-test doesn't have such tests, but I will add tests to web-platform-tests in bug 1301024.

Comment 113

[Edgar Chen [:edgar]]

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

Comment 114

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=64fceaa7397300df2fb1613eef4e1cc62bd6a867

Comment 115

[Edgar Chen [:edgar]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=07b3fec4ecd84bff9be058342ac973d4040be2f4

Comment 116

[Edgar Chen [:edgar]]

Attachment: Part 1: Implement construction stack, v6, r=wchen

Rebase.

Comment 117

[Edgar Chen [:edgar]]

Attachment: Part 4: Hold a pointer of ElementQueue in ReactionsStack instead, v4, r=wchen

Rebase.

Comment 118

[Edgar Chen [:edgar]]

Attachment: Part 5-2: Implement new upgrade steps, v8, r=wchen

Rebase and address reviewed comment.

Comment 119

[Edgar Chen [:edgar]]

Comment on attachment 8892503 [details] [diff] [review]
Part 5-2: Implement new upgrade steps, v8, r=wchen

bz, may I have your review for error reporting again? Thank you.

Comment 120

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8892503 [details] [diff] [review]
Part 5-2: Implement new upgrade steps, v8, r=wchen

> +CustomElementConstructor::Construct(const char* aExecutionReason,
>+                                    ExceptionHandling aExceptionHandling,

Do we plan to have callers pass different values for aExceptionHandling?  So far we seem to have only one caller...

>+class MOZ_RAII AutoConstructionStack final

This is more of an AutoConstructionStackEntry, right?

>+    // It is still not clear what error reporting will be looked like in custom

"will look like"

The comment about "usually report the error to the entry global doesn't make much sense to me, still.  Normally for error reporting we report once the stack has unwound, at which point the entry global is the thing we have to report on.  We never report to some ambient entry global.

There should probabbly be a Mozilla bug tracking this spec issue.

>+  // This is used for error reporting.
>+  Maybe<AutoEntryScript> aes;

So if !aGlobal, what will happen with the error reporting?  Will we just leave it on the stack ErrorResult and lose it (with asserts in debug builds and possible leaks in opt ones)?  Can that happen?

>+        ErrorResult rv;
>+        reaction->Invoke(element, rv);

I'm not sure about this part either.  Some callees of Invoke() clearly do their own error reporting already (e.g. CustomElementCallbackReaction::Invoke).  Do we double-report those errors?  That seems a bit odd if we do.

It's really bothering me that there is no clear description in comments or something of what errors can come through here, from where, and how they should be handled. That makes it really hard to review whether (1) that description matches what we want to do and (2) whether the code matches the description.

>+        ErrorResult rv;
>+        mReaction->Invoke(mCustomElement, rv);

Can mReaction->Invoke fail?  If it can, why is this code ok?

Comment 121

[John Dai[:jdai]]

Since Edgar has been parental leaving, I would like to take this bug.

Comment 122

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #120)
> Comment on attachment 8892503 [details] [diff] [review]
> Part 5-2: Implement new upgrade steps, v8, r=wchen
> 
> > +CustomElementConstructor::Construct(const char* aExecutionReason,
> >+                                    ExceptionHandling aExceptionHandling,
> 
> Do we plan to have callers pass different values for aExceptionHandling?  So
> far we seem to have only one caller...
> 
Yes, there is only one caller, I'll address them in next patch.
> >+class MOZ_RAII AutoConstructionStack final
> 
> This is more of an AutoConstructionStackEntry, right?
> 
Right, will do.
> >+    // It is still not clear what error reporting will be looked like in custom
> 
> "will look like"
> 
Will do.
> The comment about "usually report the error to the entry global doesn't make
> much sense to me, still.  Normally for error reporting we report once the
> stack has unwound, at which point the entry global is the thing we have to
> report on.  We never report to some ambient entry global.
> 
> There should probabbly be a Mozilla bug tracking this spec issue.
> 
We have a Mozilla bug 1378265 tracking this spec issue.
> >+  // This is used for error reporting.
> >+  Maybe<AutoEntryScript> aes;
> 
> So if !aGlobal, what will happen with the error reporting?  Will we just
> leave it on the stack ErrorResult and lose it (with asserts in debug builds
> and possible leaks in opt ones)?  Can that happen?
> 
If I read code correctly, the !aGlobal case happens at CustomElementReactionsStack::InvokeBackupQueue(). Per comment #63, there is no such case that runs upgrade steps in Backup Queue.
> >+        ErrorResult rv;
> >+        reaction->Invoke(element, rv);
> 
> I'm not sure about this part either.  Some callees of Invoke() clearly do
> their own error reporting already (e.g.
> CustomElementCallbackReaction::Invoke).  Do we double-report those errors? 
> That seems a bit odd if we do.
I guess it won't happen double-report those errors, because we'll handle errors at CustomElementCallback::Call() and clear those errors. Hence, CustomElementCallbackReaction::Invoke won't get any pending exception.
The reason we need to report error in CustomElementReactionsStack::InvokeReactions() is because we need to follow spec[1].

[1] https://html.spec.whatwg.org/multipage/custom-elements.html#invoke-custom-element-reactions
> 
> It's really bothering me that there is no clear description in comments or
> something of what errors can come through here, from where, and how they
> should be handled. That makes it really hard to review whether (1) that
> description matches what we want to do and (2) whether the code matches the
> description.
> 
> >+        ErrorResult rv;
> >+        mReaction->Invoke(mCustomElement, rv);
> 
> Can mReaction->Invoke fail?  If it can, why is this code ok?
If mReaction->Invoke in SyncInvokeReaction, it must happens at CustomElementCallback::Call(), then the exceptions will be handled and reported at Lifecycle*Callback::Call[2] function.

[2] https://searchfox.org/mozilla-central/rev/c329d562fb6c6218bdb79290faaf015467ef89e2/dom/base/CustomElementRegistry.cpp#59-61

Comment 123

[Boris Zbarsky [:bzbarsky]]

I don't have this fully paged back in, but...

> because we'll handle errors at CustomElementCallback::Call() and clear those errors.

In that case, why does CustomElementCallback::Call take an ErrorResult argument at all, instead of an on-stack IgnoredErrorResult?  This seems like it would make the logic flow clearer...

> then the exceptions will be handled and reported at Lifecycle*Callback::Call[2] function.

My point is that we have a function taking an ErrorResult argument.  The fundamental assumption is that such a function can leave an error on the ErrorResult and hence the caller must handle that error somehow.

If the function _can't_ leave an error on the ErrorResult, it should not have an ErrorResult argument.  If it can, the caller needs to handle errors on it (which might be by suppressing them, but that needs to be a deliberate decision with documentation as to why).

Comment 124

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #123)
> I don't have this fully paged back in, but...
> 
> > because we'll handle errors at CustomElementCallback::Call() and clear those errors.
> 
> In that case, why does CustomElementCallback::Call take an ErrorResult
> argument at all, instead of an on-stack IgnoredErrorResult?  This seems like
> it would make the logic flow clearer...
> 

The reason is CustomElementUpgradeReaction and CustomElementCallbackReaction are inherited by CustomElementReaction which has a virtual function named Invoke(). Hence, CustomElementCallbackReaction::Invoke() need to bring ErrorResult as a argument.

> > then the exceptions will be handled and reported at Lifecycle*Callback::Call[2] function.
> 
> My point is that we have a function taking an ErrorResult argument.  The
> fundamental assumption is that such a function can leave an error on the
> ErrorResult and hence the caller must handle that error somehow.
> 
> If the function _can't_ leave an error on the ErrorResult, it should not
> have an ErrorResult argument.  If it can, the caller needs to handle errors
> on it (which might be by suppressing them, but that needs to be a deliberate
> decision with documentation as to why).

SyncInvokeReactionRunnable doesn't need ErrorResult, the reason we keep it is to have same Invoke() parameter list. However, SyncInvokeReactionRunnable will be deprecated when we remove custom element v0 code. I prefer keep same parameter list and document it.

Comment 125

[John Dai[:jdai]]

Attachment: Part 5-2: Implement new upgrade steps, v9, r=wchen

- Address comment #120. 
- Revised wpt .ini files in order to pass try.

BZ, may I have your review again? Thank you.


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

Comment 126

[John Dai[:jdai]]

Attachment: Part 6: Add tests for custom element upgrade, v4

Disabled slotchange-customelements.html, because attachShadow is a shadow-dom v1 api which is not supported now.

Comment 127

[John Dai[:jdai]]

Attachment: Roll up patch.

Comment 128

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8896336 [details] [diff] [review]
Part 5-2: Implement new upgrade steps, v9, r=wchen

>+++ b/dom/base/CustomElementRegistry.cpp
>+CustomElementCallback::Call(ErrorResult& aRv)

OK.  Per IRC discussion, this should not take an ErrorResult arg, should use an on-stack IgnoredErrorResult to do the Call() invocations it does.

>+JSObject*
>+CustomElementConstructor::Construct(const char* aExecutionReason,
>+                                    ErrorResult& aRv)

It just occurred to me that this is going to fail the rooting hazard analysis, because ~CallSetup can gc and the JSObject* return value is an unrooted temporary by the time ~CallSetup runs.

This could have a signature like so:

  void Construct(const char* aExecutionReason,
                 ErrorResult& aRv,
		 JS::MutableHandle<JSObject*> aRetval);

and where you currently "return result" you'd do:

  aRetval.set(result);

instead.  That would require you to have a Rooted in the caller, which is kind of annoying.  Another option is to not return the constructed thing at all.  There is only one caller, and that caller just wants to compare it to the given Element*, so we could push that check down into Construct().  That is, have Construct() return a boolean for "constructed thing matches given element" and then you don't need to worry about this issue.  In fact, then you could avoid the extra refcount on the element, like so:

  Element* element;
  if (NS_FAILED(UNWRAP_OBJECT(Element, &result, element)) ||
      element != aElement) {
    aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
    return;
  }

in CustomElementConstructor::Construct.  Which should maybe be renamed DoUpgrade.  At that point it would take just an Element* and ErrorResult& aRv and handle the rest.

>@@ -764,17 +814,17 @@ CustomElementRegistry::Get(JSContext* aCx, const nsAString& aName,
>+  aRetVal.setObject(*data->mConstructor->CallableOrNull());

This should really be:

  aRetVal.setObjectOrNull(data->mConstructor->CallableOrNull());

because it might be null...

> CustomElementReactionsStack::InvokeBackupQueue()
>+    // If the reactions are invoked from backup queue (in microtask check point)
>+    // , we don't need to pass global object for error reporting.

',' should move to previous line.

>+CustomElementReactionsStack::InvokeReactions(ElementQueue* aElementQueue,
>+        ErrorResult rv;
>+        reaction->Invoke(element, rv);
>+        if (aes) {
>+          JSContext* cx = aes->cx();
>+          if (rv.MaybeSetPendingException(cx)) {

I think this could use a comment about how we reuse the error reporting machinery in AutoEntryScript to do the right thing.

About the !aes case... is that case one in which we know Invoke() won't throw?  If so, we should just assert after the |if (aes)| block that !rv.Failed().

If Invoke() _can_ throw in this case, then we should at the very least explicitly suppress if !aes and file a followup to come up with a way to report it.

>+            aes->ReportException();
>+          }
>+          MOZ_ASSERT(!JS_IsExceptionPending(cx));
>+        }
>       }
>     }
>     reactions.Clear();
>   }
>   aElementQueue->Clear();
> }
> 
> //-----------------------------------------------------
> // CustomElementDefinition
> 
> CustomElementDefinition::CustomElementDefinition(nsIAtom* aType,
>                                                  nsIAtom* aLocalName,
>-                                                 JSObject* aConstructor,
>+                                                 Function* aConstructor,
>                                                  JSObject* aPrototype,
>                                                  LifecycleCallbacks* aCallbacks,
>                                                  uint32_t aDocOrder)
>   : mType(aType),
>     mLocalName(aLocalName),
>-    mConstructor(aConstructor),
>+    mConstructor(new CustomElementConstructor(aConstructor)),
>     mPrototype(aPrototype),
>     mCallbacks(aCallbacks),
>     mDocOrder(aDocOrder)
> {
> }
> 
> 
> //-----------------------------------------------------
> // CustomElementUpgradeReaction
> 
> /* virtual */ void
>-CustomElementUpgradeReaction::Invoke(Element* aElement)
>+CustomElementUpgradeReaction::Invoke(Element* aElement, ErrorResult& aRv)
> {
>-  mRegistry->Upgrade(aElement, mDefinition);
>+  mRegistry->Upgrade(aElement, mDefinition, aRv);
> }
> 
> //-----------------------------------------------------
> // CustomElementCallbackReaction
> 
> /* virtual */ void
>-CustomElementCallbackReaction::Invoke(Element* aElement)

>+CustomElementCallbackReaction::Invoke(Element* aElement, ErrorResult& aRv)
>+  // It'll never throw exceptions, because all the exceptions are handled
>+  // by Lifecycle*Callback::Call function.

This comment can go away, since we're not going to pass aRv anywhere here.

>+++ b/dom/base/CustomElementRegistry.h

>+++ b/testing/web-platform/meta/custom-elements/upgrading/Node-cloneNode.html.ini
>   [Upgrading a custom element must throw InvalidStateError when the custom element's constructor returns another element]
>+    expected: FAIL

Why is that still a fail?  There's code in here to handle this case...

>\ No newline at end of file

Please add one.

r=me with the above fixes.

Comment 129

[John Dai[:jdai]]

(In reply to Boris Zbarsky [:bz] (vacation Aug 14-27) from comment #128)
> Comment on attachment 8896336 [details] [diff] [review]
> Part 5-2: Implement new upgrade steps, v9, r=wchen
> 
> >+++ b/dom/base/CustomElementRegistry.cpp
> >+CustomElementCallback::Call(ErrorResult& aRv)
> 
> OK.  Per IRC discussion, this should not take an ErrorResult arg, should use
> an on-stack IgnoredErrorResult to do the Call() invocations it does.
> 
Will do.
> >+JSObject*
> >+CustomElementConstructor::Construct(const char* aExecutionReason,
> >+                                    ErrorResult& aRv)
> 
> It just occurred to me that this is going to fail the rooting hazard
> analysis, because ~CallSetup can gc and the JSObject* return value is an
> unrooted temporary by the time ~CallSetup runs.
> 
> This could have a signature like so:
> 
>   void Construct(const char* aExecutionReason,
>                  ErrorResult& aRv,
> 		 JS::MutableHandle<JSObject*> aRetval);
> 
> and where you currently "return result" you'd do:
> 
>   aRetval.set(result);
> 
> instead.  That would require you to have a Rooted in the caller, which is
> kind of annoying.  Another option is to not return the constructed thing at
> all.  There is only one caller, and that caller just wants to compare it to
> the given Element*, so we could push that check down into Construct().  
You gave me another option is to not return the constructed thing at all. I checked the spec, we need constructed result for create an element[1](see setp 6.1.1). 

[1] https://dom.spec.whatwg.org/#concept-create-element

Per IRC discussion, we can do the simplest thing might be to have two separate entry points DoUpgrade and Construct which both create a CallSetup and then call some shared code to do the call into JS and get the JSObject which they root using the CallSetup. DoUpgrade takes an Element* argument and does the check that we returned the right thing Construct takes an Element** argument and pulls the Element out of the constructed thing, assuming it returns an Element. In both cases, the JSAPI bits are all completely contained inside the CustomElementConstructor.

> That is, have Construct() return a boolean for "constructed thing matches given
> element" and then you don't need to worry about this issue.  In fact, then
> you could avoid the extra refcount on the element, like so:
> 
>   Element* element;
>   if (NS_FAILED(UNWRAP_OBJECT(Element, &result, element)) ||
>       element != aElement) {
>     aRv.Throw(NS_ERROR_DOM_INVALID_STATE_ERR);
>     return;
>   }
> 
> in CustomElementConstructor::Construct.  Which should maybe be renamed
> DoUpgrade.  At that point it would take just an Element* and ErrorResult&
> aRv and handle the rest.
> 
> >@@ -764,17 +814,17 @@ CustomElementRegistry::Get(JSContext* aCx, const nsAString& aName,
> >+  aRetVal.setObject(*data->mConstructor->CallableOrNull());
> 
> This should really be:
> 
>   aRetVal.setObjectOrNull(data->mConstructor->CallableOrNull());
> 
> because it might be null...
> 
Will do.
> > CustomElementReactionsStack::InvokeBackupQueue()
> >+    // If the reactions are invoked from backup queue (in microtask check point)
> >+    // , we don't need to pass global object for error reporting.
> 
> ',' should move to previous line.
> 
Will do.
> >+CustomElementReactionsStack::InvokeReactions(ElementQueue* aElementQueue,
> >+        ErrorResult rv;
> >+        reaction->Invoke(element, rv);
> >+        if (aes) {
> >+          JSContext* cx = aes->cx();
> >+          if (rv.MaybeSetPendingException(cx)) {
> 
> I think this could use a comment about how we reuse the error reporting
> machinery in AutoEntryScript to do the right thing.
> 
> About the !aes case... is that case one in which we know Invoke() won't
> throw?  If so, we should just assert after the |if (aes)| block that
> !rv.Failed().
> 
For !aes case, we know Invoke() won't throw. We can add assert after the |if (aes)| block that !rv.Failed().

> If Invoke() _can_ throw in this case, then we should at the very least
> explicitly suppress if !aes and file a followup to come up with a way to
> report it.
> 
> >+            aes->ReportException();
> >+          }
> >+          MOZ_ASSERT(!JS_IsExceptionPending(cx));
> >+        }
> >       }
> >     }
> >     reactions.Clear();
> >   }
> >   aElementQueue->Clear();
> > }
> > 
> > //-----------------------------------------------------
> > // CustomElementDefinition
> > 
> > CustomElementDefinition::CustomElementDefinition(nsIAtom* aType,
> >                                                  nsIAtom* aLocalName,
> >-                                                 JSObject* aConstructor,
> >+                                                 Function* aConstructor,
> >                                                  JSObject* aPrototype,
> >                                                  LifecycleCallbacks* aCallbacks,
> >                                                  uint32_t aDocOrder)
> >   : mType(aType),
> >     mLocalName(aLocalName),
> >-    mConstructor(aConstructor),
> >+    mConstructor(new CustomElementConstructor(aConstructor)),
> >     mPrototype(aPrototype),
> >     mCallbacks(aCallbacks),
> >     mDocOrder(aDocOrder)
> > {
> > }
> > 
> > 
> > //-----------------------------------------------------
> > // CustomElementUpgradeReaction
> > 
> > /* virtual */ void
> >-CustomElementUpgradeReaction::Invoke(Element* aElement)
> >+CustomElementUpgradeReaction::Invoke(Element* aElement, ErrorResult& aRv)
> > {
> >-  mRegistry->Upgrade(aElement, mDefinition);
> >+  mRegistry->Upgrade(aElement, mDefinition, aRv);
> > }
> > 
> > //-----------------------------------------------------
> > // CustomElementCallbackReaction
> > 
> > /* virtual */ void
> >-CustomElementCallbackReaction::Invoke(Element* aElement)
> 
> >+CustomElementCallbackReaction::Invoke(Element* aElement, ErrorResult& aRv)
> >+  // It'll never throw exceptions, because all the exceptions are handled
> >+  // by Lifecycle*Callback::Call function.
> 
> This comment can go away, since we're not going to pass aRv anywhere here.
> 
Will do.
> >+++ b/dom/base/CustomElementRegistry.h
> 
> >+++ b/testing/web-platform/meta/custom-elements/upgrading/Node-cloneNode.html.ini
> >   [Upgrading a custom element must throw InvalidStateError when the custom element's constructor returns another element]
> >+    expected: FAIL
> 
> Why is that still a fail?  There's code in here to handle this case...
> 
Will do.
> >\ No newline at end of file
> 
> Please add one.
> 
Will do.
> r=me with the above fixes.

Comment 130

[Boris Zbarsky [:bzbarsky]]

> For !aes case, we know Invoke() won't throw. We can add assert after the |if (aes)| block that !rv.Failed().

Sounds good.

Comment 131

[John Dai[:jdai]]

Attachment: Part 5-2: Implement new upgrade steps, v10, r=wchen, bz

Address comment #129.

> >+++ b/dom/base/CustomElementRegistry.h
> 
> >+++ b/testing/web-platform/meta/custom-elements/upgrading/Node-cloneNode.html.ini
> >   [Upgrading a custom element must throw InvalidStateError when the custom element's constructor returns another element]
> >+    expected: FAIL
> 
> Why is that still a fail?  There's code in here to handle this case...
> 
The test use cloneNode which doesn't run upgrade steps. It'll fix at https://bugzilla.mozilla.org/show_bug.cgi?id=1319342

Comment 132

[John Dai[:jdai]]

Attachment: Part 5-2: Implement new upgrade steps, v11, r=wchen, bz

- Add error result when CustomElementConstructor::Construct() has error.
- Use constructResult.get() instead of static_cast.

Comment 133

[John Dai[:jdai]]

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

Comment 134

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5cd0326e135
Part 1: Implement construction stack. r=wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/0198da9fb1ba
Part 2: Allow prototype swizzling in html constructor. r=bz
https://hg.mozilla.org/integration/mozilla-inbound/rev/acdd9706f32d
Part 3: Remove unused argument in UpgradeCandidates. r=bz, r=wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/b5469b16410b
Part 4: Hold a pointer of ElementQueue in ReactionsStack instead. r=wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/6836dfc28fda
Part 5-1: Make the constructor created by document.registerElement() also works with construction stack. r=wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/476b08d860bf
Part 5-2: Implement new upgrade steps. r=bz, r=wchen
https://hg.mozilla.org/integration/mozilla-inbound/rev/73ab5f2b2965
Part 6: Add tests for custom element upgrade. r=wchen

Comment 135

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/f5cd0326e135
https://hg.mozilla.org/mozilla-central/rev/0198da9fb1ba
https://hg.mozilla.org/mozilla-central/rev/acdd9706f32d
https://hg.mozilla.org/mozilla-central/rev/b5469b16410b
https://hg.mozilla.org/mozilla-central/rev/6836dfc28fda
https://hg.mozilla.org/mozilla-central/rev/476b08d860bf
https://hg.mozilla.org/mozilla-central/rev/73ab5f2b2965

Comment 136

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Thanks for your work, John, Edgar, and thanks for your review, Boris and William. :)