Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Should infinite recursion of custom element constructors be possible? #5118

Closed
mfreed7 opened this issue Dec 2, 2019 · 7 comments · Fixed by #5126
Closed

Should infinite recursion of custom element constructors be possible? #5118

mfreed7 opened this issue Dec 2, 2019 · 7 comments · Fixed by #5126
Assignees
Labels
topic: custom elements Relates to custom elements (as defined in DOM and HTML)

Comments

@mfreed7
Copy link
Contributor

mfreed7 commented Dec 2, 2019

The custom element upgrade spec, section 4.13.5, step 1, says "If element is custom, then return". And there is a large example there showing a custom element constructor that attempts to call itself recursively. What should that example do?

A simplified example is:

customElements.define("x-foo", class extends HTMLElement {
  constructor() {
    super();
    customElements.upgrade(this);
  }
});

See this Chromium bug for a more detailed discussion of this situation. There is a debate about whether the constructor and/or CE reactions should happen in step 2 or step 4 of the CEReactions spec. If the constructor happens at step 2, and the (recursively triggered) upgrade reactions happen at step 4, then the current spec is correct, the Chromium implementation is wrong, and there should be no infinite recursion here. If, on the other hand, both the constructor and the (recursively triggered) upgrades happen within a single step (2 or 4) then the spec is wrong, Chromium is correct, and the example above will generate a JS stack overflow.

@annevk annevk added the topic: custom elements Relates to custom elements (as defined in DOM and HTML) label Dec 2, 2019
@domenic
Copy link
Member

domenic commented Dec 2, 2019

Can you clarify what

There is a debate about whether the constructor and/or CE reactions should happen in step 2 or step 4 of the CEReactions spec.

means? I.e. what algorithms are "the constructor" and what algorithms are "CE reactions"?

@mfreed7
Copy link
Contributor Author

mfreed7 commented Dec 2, 2019

For this issue, by “the constructor” I mean the call to the JS function called constructor() in the example above, and by “CE Reactions” I mean the Upgrade reaction that should be run or queued by the call to customElements.upgrade(this) in the example above.

@mfreed7
Copy link
Contributor Author

mfreed7 commented Dec 2, 2019

I got lazy with this issue and incorporated-by-reference the rest of the discussion from crbug.com/966472. I am happy to summarize that discussion more completely here, if needed. Just let me know.

@domenic
Copy link
Member

domenic commented Dec 2, 2019

Here is my reading, assuming there is some preceding instance of x-foo in the document being upgraded.

  • Via various queuing machinery, eventually we reach upgrade. At this time the element's CE reaction queue is empty, because https://html.spec.whatwg.org/#invoke-custom-element-reactions 1.2.1 removes the reaction before performing the upgrade.
    • 8.2 performs constructing on the Web IDL Function object.
      • 10 calls in (via the JS spec's Construct() operation) to the author code
        • super() calls the [HTMLConstructor] algorithm. This mostly just does [[SetPrototypeOf]].
        • customElements.upgrade(this) happens
          • Step 1 of CEReactions steps happens, pushing a new element queue (call it Q) onto the agent's CE reactions stack
          • Step 2 of CEReactions steps happens, executing the algorithm for customElements.upgrade(), which mainly does try to upgrade
          • Step 3 of CEReactions steps happens, which pops Q off the agent's CE reactions stack
          • Step 4 of CEReactions steps happens, which invokes CE reactions in Q, i.e. invokes the upgrade reaction from (*), and thus recursively ends up at the upgrade algorithm.

So it looks like an infinite recursion to me.

I don't quite understand the step 2 or step 4 thing still, but my diagnosis is that probably we should move step 10 of "upgrade an element" up to around step 3.

@mfreed7
Copy link
Contributor Author

mfreed7 commented Dec 3, 2019

Thank you for the analysis - I agree with you. And I believe moving step 10 up to step 3 should solve the infinite recursion, and should make step 1 and its example make sense.

@tkent-google brought up the step 2/4 issue, so I want to give him the chance to weigh in here.

@tkent-google
Copy link
Contributor

I just explained that the recursion was an expected behavior according to the current specification and Chrome followed the specification correctly.

I also think moving the step 10 is the simplest solution, and it should not have bad side-effect.

@mfreed7
Copy link
Contributor Author

mfreed7 commented Dec 6, 2019

@tkent-google Ok good, sorry I misunderstood you then. Sounds like this is resolved, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: custom elements Relates to custom elements (as defined in DOM and HTML)
Development

Successfully merging a pull request may close this issue.

4 participants