Bug 206605 - Prevent infinite recursion when upgrading custom elements
Summary: Prevent infinite recursion when upgrading custom elements
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari Technology Preview
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 154907
  Show dependency treegraph
 
Reported: 2020-01-22 11:43 PST by Domenic Denicola
Modified: 2020-08-27 19:56 PDT (History)
9 users (show)

See Also:


Attachments
Implements the new behavior (19.00 KB, patch)
2020-08-26 23:51 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Domenic Denicola 2020-01-22 11:43:11 PST
We recently discovered a bug where certain strange patterns of author code could cause infinite recursion in the spec algorithms and in all browsers. Blink has fixed this and Gecko has a bug on file, so here's one for WebKit.

Spec fix: https://github.com/whatwg/html/pull/5126

Tests: https://github.com/web-platform-tests/wpt/pull/20465

Live tests: https://wpt.fyi/results/custom-elements/upgrading.html?label=master&label=experimental&aligned
Comment 1 Ryosuke Niwa 2020-08-26 23:51:08 PDT
Created attachment 407380 [details]
Implements the new behavior
Comment 2 Antti Koivisto 2020-08-27 09:59:49 PDT
Comment on attachment 407380 [details]
Implements the new behavior

View in context: https://bugs.webkit.org/attachment.cgi?id=407380&action=review

> Source/WebCore/bindings/js/JSCustomElementInterface.cpp:203
> +    // Unlike spec, set element's custom element state to "failed" after enqueueing post-upgrade reactions
> +    // to avoid hitting debug assertions in enqueuePostUpgradeReactions.
> +    element.setIsFailedCustomElementWithoutClearingReactionQueue();

Isn't this confusing? Wouldn't it be better to change the asserts?

> Source/WebCore/dom/Element.cpp:2468
> +#if ASSERT_ENABLED
> +    if (isFailedCustomElement()) {
> +        auto* queue = elementRareData()->customElementReactionQueue();
> +        ASSERT(queue);
> +        ASSERT(queue->isEmpty() || queue->hasJustUpgradeReaction());
> +    } else
> +        ASSERT(isDefinedCustomElement() || isCustomElementUpgradeCandidate());
> +#endif

Alternatively you could factor the validity tests into a boolean returning function and ASSERT that.
Comment 3 Ryosuke Niwa 2020-08-27 19:53:18 PDT
Thanks for the review.

(In reply to Antti Koivisto from comment #2)
> Comment on attachment 407380 [details]
> Implements the new behavior
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407380&action=review
> 
> > Source/WebCore/bindings/js/JSCustomElementInterface.cpp:203
> > +    // Unlike spec, set element's custom element state to "failed" after enqueueing post-upgrade reactions
> > +    // to avoid hitting debug assertions in enqueuePostUpgradeReactions.
> > +    element.setIsFailedCustomElementWithoutClearingReactionQueue();
> 
> Isn't this confusing? Wouldn't it be better to change the asserts?

I don't think so. enqueuePostUpgradeReactions asserts that the element is isCustomElementUpgradeCandidate. It would be weird to assert that it's a "failed" custom element since that could occur when the custom element had actually failed to upgrade.

> > Source/WebCore/dom/Element.cpp:2468
> > +#if ASSERT_ENABLED
> > +    if (isFailedCustomElement()) {
> > +        auto* queue = elementRareData()->customElementReactionQueue();
> > +        ASSERT(queue);
> > +        ASSERT(queue->isEmpty() || queue->hasJustUpgradeReaction());
> > +    } else
> > +        ASSERT(isDefinedCustomElement() || isCustomElementUpgradeCandidate());
> > +#endif
> 
> Alternatively you could factor the validity tests into a boolean returning
> function and ASSERT that.

Hm... that would make it less obvious which condition had failed when one of these debug assertions hit so I'd keep it as is.
Comment 4 Ryosuke Niwa 2020-08-27 19:55:22 PDT
Comment on attachment 407380 [details]
Implements the new behavior

Clearing flags on attachment: 407380

Committed r266269: <https://trac.webkit.org/changeset/266269>
Comment 5 Ryosuke Niwa 2020-08-27 19:55:24 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Radar WebKit Bug Importer 2020-08-27 19:56:14 PDT
<rdar://problem/67914221>