Prevent infinite recursion when upgrading custom elements by domenic · Pull Request #5126 · whatwg/html

domenic · 2019-12-04T21:58:37Z

Fixes #5118.

At least two implementers are interested (and none opposed):
- Chromium
- Gecko
Tests are written and can be reviewed and commented upon at:
- Disallow recursive custom element constructions web-platform-tests/wpt#20465
Implementation bugs are filed:
- Chrome: https://bugs.chromium.org/p/chromium/issues/detail?id=966472
- Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=1610054
- Safari: https://bugs.webkit.org/show_bug.cgi?id=206605

(See WHATWG Working Mode: Changes for more details.)

@rniwa @smaug---- can you give a quick +1? This is just a bugfix in my opinion.

@mfreed7 can you write tests for this as part of the Chromium fix?

/custom-elements.html ( diff )

Fixes #5118.

mfreed7 · 2019-12-06T19:45:17Z

@domenic Yes I'll be happy to write a WPT for this with my patch. (Already done.)

With this CL, recursive custom element constructions are no longer allowed. I.e. this will now only run the constructor once: class extends HTMLElement { constructor() { super(); customElements.upgrade(this); } } Previously, the code and spec had a bug which caused the above code snippet to infinitely recurse. In [1] the spec has changed, to set the custom element state to kCustom earlier in the process, so that recursive calls can properly early-out at step #1 (of [2]). [1] whatwg/html#5126 [2] https://html.spec.whatwg.org/multipage/custom-elements.html#upgrades Bug: 966472 Change-Id: I76e88c0b70132eee2482c304ef9e727ae1fe8fc7

rniwa · 2019-12-09T22:14:17Z

source


+   <li><p>Set <var>element</var>'s <span>custom element state</span> to "<code
+   data-x="">custom</code>".</p></li>
+


This will make it so that custom element's state can move from "custom" to "failed". I don't think we currently allow that that would be strange and might pose a problem elsewhere. We may need to introduce a new state like "upgrading", and reject upgrading of an element in such a state instead.

I guess this would be observable if you do this.matches(":defined") inside the constructor? You want to ensure that keeps being false, I take it?

Right. We want to make sure once a custom element has reached :defined, then it never goes back into :undefined state. Since that would be strange & confusing; it could even cause some compatibility issues with existing content that relies on this invariant today.

OK, I've reworked this to hold, although I did it by just using the "failed" state instead of introducing a new state.

@mfreed7 can you be sure to capture this .matches(":defined") issue in your test case? And also your review would be appreciated since this deviates a bit from what we discussed in #5118.

@rniwa, does this LGTY?

@rniwa Thanks for the suggestion. I can confirm that things work much better with a new "upgrading" state, rather than immediately going to "defined" in step 3. No more test failures, and the two use cases that started the Chromium bug are fixed.

@domenic Sorry I missed your comment before posting mine above. I like the new change - saves a bit in node. And it appears to work correctly as you've written it. I added a WPT test for this.matches(":defined") within the constructor. Thanks!

@rniwa LGTY?

FYI, the chrome CL is ready, just waiting for this PR to land.

With this CL, recursive custom element constructions are no longer allowed. I.e. this will now only run the constructor once: class extends HTMLElement { constructor() { super(); customElements.upgrade(this); } } Previously, the code and spec had a bug which caused the above code snippet to infinitely recurse. In [1] the spec has changed, to set the custom element state to kCustom earlier in the process, so that recursive calls can properly early-out at step #1 (of [2]). [1] whatwg/html#5126 [2] https://html.spec.whatwg.org/multipage/custom-elements.html#upgrades Bug: 966472 Change-Id: I76e88c0b70132eee2482c304ef9e727ae1fe8fc7

With this CL, recursive custom element constructions are no longer allowed. I.e. this will now only run the constructor once: class extends HTMLElement { constructor() { super(); customElements.upgrade(this); } } Previously, the code and spec had a bug which caused the above code snippet to infinitely recurse. In [1] the spec has changed, to set the custom element state to a new state called "upgrading" before the constructor is called. With this change in place, recursive calls can properly early-out at step #1 (of [2]). [1] whatwg/html#5126 [2] https://html.spec.whatwg.org/multipage/custom-elements.html#upgrades Bug: 966472 Change-Id: I76e88c0b70132eee2482c304ef9e727ae1fe8fc7

With this CL, recursive custom element constructions are no longer allowed. I.e. this will now only run the constructor once: class extends HTMLElement { constructor() { super(); customElements.upgrade(this); } } Previously, the code and spec had a bug which caused the above code snippet to infinitely recurse. In [1] the spec has changed, to set the custom element state to "failed" before the constructor is called. With this change in place, recursive calls will early-out at step #2 (of [2]), and avoid the recursion. [1] whatwg/html#5126 [2] https://html.spec.whatwg.org/multipage/custom-elements.html#upgrades Bug: 966472 Change-Id: I76e88c0b70132eee2482c304ef9e727ae1fe8fc7

rniwa · 2019-12-19T23:08:04Z

Sorry, I know I have to review this PR. Hoping to do that in the next few days.

mfreed7 · 2019-12-30T17:46:05Z

Any objections to me landing this change in Chromium now, prior to this PR landing? It seems rather safe - I doubt anyone was depending on the prior infinite recursion behavior. And if we end up changing the details of exactly how the recursion is prevented, we can land that as a subsequent change in code.

domenic · 2019-12-30T17:56:08Z

No objections, please land! We need multi-implementer support to land in the spec, which is tricky over the holidays, but that shouldn't block implementations.

mfreed7 · 2019-12-31T01:25:50Z

Thanks!

With this CL, recursive custom element constructions are no longer allowed. I.e. this will now only run the constructor once: class extends HTMLElement { constructor() { super(); customElements.upgrade(this); } } Previously, the code and spec had a bug which caused the above code snippet to infinitely recurse. In [1] the spec has changed, to set the custom element state to "failed" before the constructor is called. With this change in place, recursive calls will early-out at step #2 (of [2]), and avoid the recursion. [1] whatwg/html#5126 [2] https://html.spec.whatwg.org/multipage/custom-elements.html#upgrades Bug: 966472 Change-Id: I76e88c0b70132eee2482c304ef9e727ae1fe8fc7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1931644 Reviewed-by: Kent Tamura <tkent@chromium.org> Commit-Queue: Mason Freed <masonfreed@chromium.org> Auto-Submit: Mason Freed <masonfreed@chromium.org> Cr-Commit-Position: refs/heads/master@{#727841}

Implement CustomElementRegistry.upgrade  An additional small change will be needed if whatwg/html#5126 lands, which will pass an infinite recursion test this is currently failing in custom-elements/upgrading.html (right now the test is asking for behavior that isn't in the spec). The remaining test in custom-elements/custom-element-registry/upgrade.html is because it needs shadow DOM. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #24989  - [X] There are tests for these changes

…uctions, a=testonly Automatic update from web-platform-tests Disallow recursive custom element constructions With this CL, recursive custom element constructions are no longer allowed. I.e. this will now only run the constructor once: class extends HTMLElement { constructor() { super(); customElements.upgrade(this); } } Previously, the code and spec had a bug which caused the above code snippet to infinitely recurse. In [1] the spec has changed, to set the custom element state to "failed" before the constructor is called. With this change in place, recursive calls will early-out at step #2 (of [2]), and avoid the recursion. [1] whatwg/html#5126 [2] https://html.spec.whatwg.org/multipage/custom-elements.html#upgrades Bug: 966472 Change-Id: I76e88c0b70132eee2482c304ef9e727ae1fe8fc7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1931644 Reviewed-by: Kent Tamura <tkent@chromium.org> Commit-Queue: Mason Freed <masonfreed@chromium.org> Auto-Submit: Mason Freed <masonfreed@chromium.org> Cr-Commit-Position: refs/heads/master@{#727841} -- wpt-commits: 4f24cabbe1f503f3daf8cf0834f4fb1a032f31bc wpt-pr: 20465

…uctions, a=testonly Automatic update from web-platform-tests Disallow recursive custom element constructions With this CL, recursive custom element constructions are no longer allowed. I.e. this will now only run the constructor once: class extends HTMLElement { constructor() { super(); customElements.upgrade(this); } } Previously, the code and spec had a bug which caused the above code snippet to infinitely recurse. In [1] the spec has changed, to set the custom element state to "failed" before the constructor is called. With this change in place, recursive calls will early-out at step mykmelez#2 (of [2]), and avoid the recursion. [1] whatwg/html#5126 [2] https://html.spec.whatwg.org/multipage/custom-elements.html#upgrades Bug: 966472 Change-Id: I76e88c0b70132eee2482c304ef9e727ae1fe8fc7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1931644 Reviewed-by: Kent Tamura <tkent@chromium.org> Commit-Queue: Mason Freed <masonfreed@chromium.org> Auto-Submit: Mason Freed <masonfreed@chromium.org> Cr-Commit-Position: refs/heads/master@{#727841} -- wpt-commits: 4f24cabbe1f503f3daf8cf0834f4fb1a032f31bc wpt-pr: 20465

EdgarChen · 2020-01-17T23:56:52Z

Gecko bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1610054

annevk

Editorially this looks good to me, but left some optional suggestions.

annevk · 2020-01-22T14:46:56Z

source

    </ol>
+
+    <p class="note">If the above steps threw an exception, then <var>element</var>'s <span>custom
+    element state</span> will remain "<code data-x="">failed</code>".</p>


You could make this an assert.

I like this more as a note, as it's more just reassuring you that nothing changed, instead of asserting something...

annevk · 2020-01-22T14:49:43Z

source

-    return.</p>
+   <li id="concept-upgrade-an-element-early-exit">
+    <p>If <var>element</var>'s <span>custom element state</span> is not "<code
+    data-x="">undefined</code>" or "<code data-x="">uncustomized</code>", then return.</p>


Curious, any reason not to go with the positive check here (failed or custom)?

I remember waffling on this a few times, especially when I was considering adding a new state. I'm still not sure what is best, but I guess the current PR feels a little better, because it's basically saying "If you're not in an upgradable state, then fail". The positive check is more like "If you are in an un-upgradable state, then fail", which communicates the intent slightly less well, in my opinion.

Prevent infinite recursion when upgrading custom elements  The spec and tests were out of sync when I implemented #25410 and I mentioned I'd have an extra detail to attend to if whatwg/html#5126 landed; it did, and I did, and a couple test cases pass. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix two WPT test cases, see test metadata in commit  - [X] There are tests for these changes

…ts; r=smaug See whatwg/html#5126. Differential Revision: https://phabricator.services.mozilla.com/D60848

…ts; r=smaug See whatwg/html#5126. Differential Revision: https://phabricator.services.mozilla.com/D60848 --HG-- extra : moz-landing-system : lando

…ts; r=smaug See whatwg/html#5126. Differential Revision: https://phabricator.services.mozilla.com/D60848 UltraBlame original commit: 687c61a5b2642d36bc7e6461bbc846fea5479e30

…changed during upgrading In https://dom.spec.whatwg.org/#handle-attribute-changes, the attributeChangedCallback reaction is enqueued only if the custom-element-state is customized. And the assumption of "custom-element-definition is only available on the element whose custom-element-state is customized" is no longer true after bug 1610054 along with the spec changings in whatwg/html#5126. Differential Revision: https://phabricator.services.mozilla.com/D71027 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1629761 gecko-commit: a8129abc4c2d5efca1c694803ea43550f39fd7a0 gecko-integration-branch: autoland gecko-reviewers: smaug

…ibute that is changed during upgrading; r=smaug In https://dom.spec.whatwg.org/#handle-attribute-changes, the attributeChangedCallback reaction is enqueued only if the custom-element-state is customized. And the assumption of "custom-element-definition is only available on the element whose custom-element-state is customized" is no longer true after bug 1610054 along with the spec changings in whatwg/html#5126. Differential Revision: https://phabricator.services.mozilla.com/D71027 --HG-- extra : moz-landing-system : lando

…ibute that is changed during upgrading; r=smaug In https://dom.spec.whatwg.org/#handle-attribute-changes, the attributeChangedCallback reaction is enqueued only if the custom-element-state is customized. And the assumption of "custom-element-definition is only available on the element whose custom-element-state is customized" is no longer true after bug 1610054 along with the spec changings in whatwg/html#5126. Differential Revision: https://phabricator.services.mozilla.com/D71027

…changed during upgrading In https://dom.spec.whatwg.org/#handle-attribute-changes, the attributeChangedCallback reaction is enqueued only if the custom-element-state is customized. And the assumption of "custom-element-definition is only available on the element whose custom-element-state is customized" is no longer true after bug 1610054 along with the spec changings in whatwg/html#5126. Differential Revision: https://phabricator.services.mozilla.com/D71027 bugzilla-url: https://bugzilla.mozilla.org/show_bug.cgi?id=1629761 gecko-commit: a8129abc4c2d5efca1c694803ea43550f39fd7a0 gecko-integration-branch: autoland gecko-reviewers: smaug

…ibute that is changed during upgrading; r=smaug a=RyanVM In https://dom.spec.whatwg.org/#handle-attribute-changes, the attributeChangedCallback reaction is enqueued only if the custom-element-state is customized. And the assumption of "custom-element-definition is only available on the element whose custom-element-state is customized" is no longer true after bug 1610054 along with the spec changings in whatwg/html#5126. Differential Revision: https://phabricator.services.mozilla.com/D71027

…uctions, a=testonly Automatic update from web-platform-tests Disallow recursive custom element constructions With this CL, recursive custom element constructions are no longer allowed. I.e. this will now only run the constructor once: class extends HTMLElement { constructor() { super(); customElements.upgrade(this); } } Previously, the code and spec had a bug which caused the above code snippet to infinitely recurse. In [1] the spec has changed, to set the custom element state to "failed" before the constructor is called. With this change in place, recursive calls will early-out at step #2 (of [2]), and avoid the recursion. [1] whatwg/html#5126 [2] https://html.spec.whatwg.org/multipage/custom-elements.html#upgrades Bug: 966472 Change-Id: I76e88c0b70132eee2482c304ef9e727ae1fe8fc7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1931644 Reviewed-by: Kent Tamura <tkent@chromium.org> Commit-Queue: Mason Freed <masonfreed@chromium.org> Auto-Submit: Mason Freed <masonfreed@chromium.org> Cr-Commit-Position: refs/heads/master@{#727841} -- wpt-commits: 4f24cabbe1f503f3daf8cf0834f4fb1a032f31bc wpt-pr: 20465

…ts; r=smaug See whatwg/html#5126. Differential Revision: https://phabricator.services.mozilla.com/D60848

…ibute that is changed during upgrading; r=smaug In https://dom.spec.whatwg.org/#handle-attribute-changes, the attributeChangedCallback reaction is enqueued only if the custom-element-state is customized. And the assumption of "custom-element-definition is only available on the element whose custom-element-state is customized" is no longer true after bug 1610054 along with the spec changings in whatwg/html#5126. Differential Revision: https://phabricator.services.mozilla.com/D71027

With this CL, recursive custom element constructions are no longer allowed. I.e. this will now only run the constructor once: class extends HTMLElement { constructor() { super(); customElements.upgrade(this); } } Previously, the code and spec had a bug which caused the above code snippet to infinitely recurse. In [1] the spec has changed, to set the custom element state to "failed" before the constructor is called. With this change in place, recursive calls will early-out at step #2 (of [2]), and avoid the recursion. [1] whatwg/html#5126 [2] https://html.spec.whatwg.org/multipage/custom-elements.html#upgrades Bug: 966472 Change-Id: I76e88c0b70132eee2482c304ef9e727ae1fe8fc7 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1931644 Reviewed-by: Kent Tamura <tkent@chromium.org> Commit-Queue: Mason Freed <masonfreed@chromium.org> Auto-Submit: Mason Freed <masonfreed@chromium.org> Cr-Commit-Position: refs/heads/master@{#727841}

Prevent infinite recursion when upgrading custom elements

bdbd92c

Fixes #5118.

domenic added the topic: custom elements Relates to custom elements (as defined in DOM and HTML) label Dec 4, 2019

chromium-wpt-export-bot mentioned this pull request Dec 6, 2019

Disallow recursive custom element constructions web-platform-tests/wpt#20465

Merged

rniwa reviewed Dec 9, 2019

View reviewed changes

domenic added 2 commits December 10, 2019 17:54

Do this a different way

99039ee

Do it a third way, no new state needed

982e8b4

domenic requested a review from rniwa December 19, 2019 19:11

This was referenced Jan 1, 2020

Implement CustomElementRegistry.upgrade servo/servo#24989

Closed

Implement CustomElementRegistry.upgrade servo/servo#25410

Merged

domenic requested a review from annevk January 21, 2020 20:49

annevk approved these changes Jan 22, 2020

View reviewed changes

domenic merged commit 2793ee4 into master Jan 22, 2020

domenic deleted the prevent-ce-upgrade-recursion branch January 22, 2020 19:39

pshaughn mentioned this pull request Jan 22, 2020

Prevent infinite recursion when upgrading custom elements servo/servo#25575

Merged

4 tasks

xeonchen pushed a commit to xeonchen/gecko that referenced this pull request Jan 28, 2020

Bug 1610054 - Prevent infinite recursion when upgrading custom elemen…

c599eba

…ts; r=smaug See whatwg/html#5126. Differential Revision: https://phabricator.services.mozilla.com/D60848

moz-wptsync-bot mentioned this pull request Apr 15, 2020

[Gecko Bug 1629761] Should not invoke attributeChangedCallback for the attribute that is changed during upgrading web-platform-tests/wpt#22972

Merged

bhearsum pushed a commit to mozilla-releng/staging-firefox that referenced this pull request May 1, 2025

Bug 1610054 - Prevent infinite recursion when upgrading custom elemen…

a29f747

…ts; r=smaug See whatwg/html#5126. Differential Revision: https://phabricator.services.mozilla.com/D60848


		<li><p>Set <var>element</var>'s <span>custom element state</span> to "<code
		data-x="">custom</code>".</p></li>

Conversation

domenic commented Dec 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mfreed7 commented Dec 6, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rniwa Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rniwa commented Dec 19, 2019

Uh oh!

mfreed7 commented Dec 30, 2019

Uh oh!

domenic commented Dec 30, 2019

Uh oh!

mfreed7 commented Dec 31, 2019

Uh oh!

EdgarChen commented Jan 17, 2020

Uh oh!

annevk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

5 participants

domenic commented Dec 4, 2019 •

edited

Loading

rniwa Dec 9, 2019 •

edited

Loading