Bug 188327 – innerHTML should not synchronously create a custom element

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

The fragment parsing algorithm is supposed to simply enqueue an element to upgrade, not synchronously construct it.

[Radar WebKit Bug Importer]

<rdar://problem/42923114>

[Ryosuke Niwa]

Created attachment 346578 [details]
Fixes the bug

[Daniel Bates]

Comment on attachment 346578 [details]
Fixes the bug

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

Looks sane to me.

> Source/WebCore/ChangeLog:13
> +        Namely, when the HTML parser creates an element for a token, *will execute script* will be set to false in
> +        the fragment parsing algorithm, and creates an element with synchronous custom elements flag *not* set:

This sentence does not read well. In particular, the "and creates an element" does not read well.

> Source/WebCore/ChangeLog:21
> +        enqueued to upgrade is enqueued to upgarde for the second time. In this case, we need to put the element to the

upgarde => upgrade

"to the" => "into the"

> Source/WebCore/ChangeLog:24
> +        While the specification simply enqueues another upgarde reaction and bails out immediately in the first step of

upgarde => upgrade

> Source/WebCore/ChangeLog:46
> +        was synchrnously constructing a custom element even for the fragment parsing algorithm.

synchrnously => synchronously

> Source/WebCore/dom/CustomElementReactionQueue.cpp:126
> +    if (alreadyScheduledToUpgrade) {
> +        ASSERT(queue.m_items.size() == 1);
> +        ASSERT(queue.m_items[0].type() == CustomElementReactionQueueItem::Type::ElementUpgrade);
> +    } else
> +        queue.m_items.append({CustomElementReactionQueueItem::Type::ElementUpgrade});

I wish there was a better way we could design this code such that we wouldn't need to have alreadyScheduledToUpgrade. Putting this aside lets look at this code. Although the compiler is smart enough to optimize out the branch on alreadyScheduledToUpgrade in a release build the existence of alreadyScheduledToUpgrade is only meaningful in a debug build (for the assertion), its unnecessary to write out such a branch in source code. (Disregarding code style, I suspect another reason you wrote it this way was to avoid an unused argument compiler warning when building release). For your consideration, I suggest that we make use of the convenience macro ASSERT_UNUSED() and then simplify this code to read:

ASSERT_UNUSED(alreadyScheduledToUpgrade, !alreadyScheduledToUpgrade || queue.m_items.size() == 1);
ASSERT_UNUSED(alreadyScheduledToUpgrade, !alreadyScheduledToUpgrade || queue.m_items[0].type() == CustomElementReactionQueueItem::Type::ElementUpgrade);
queue.m_items.append({ CustomElementReactionQueueItem::Type::ElementUpgrade });

(Notice that I also took stylistic liberty to put space characters after the '{' and before the '}' in the last line as it's an unwritten convention to put such spaces).

On another note, is queue.m_items a public field? If so, it is an unwritten convention to omit the "m_" prefix from the name of public member fields. We do not need to fix this up in this patch.

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:681
> +            element = HTMLElement::create(QualifiedName(nullAtom(), localName, xhtmlNamespaceURI), ownerDocument);

Stylistic nit (code is fine as-is): There is a growing movement of using uniform initialization syntax for new code.

> Source/WebCore/html/parser/HTMLConstructionSite.cpp:685
> +            QualifiedName qualifiedName(nullAtom(), localName, xhtmlNamespaceURI);

Stylistic nit (code is fine as-is): There is a growing movement of using uniform initialization syntax for new code.

> LayoutTests/imported/w3c/web-platform-tests/custom-elements/connected-callbacks-html-fragment-parsing-expected.txt:9
> +PASS Inserting a custom element into the document using HTML fragment parsing must enqueue a custom element upgrade reaction, not synchronously invoke its constructor 
> +PASS Inserting a custom element into the document of the template elements using HTML fragment parsing must enqueue a custom element upgrade reaction, not synchronously invoke its constructor 
> +PASS Inserting a custom element into a new document using HTML fragment parsing must enqueue a custom element upgrade reaction, not synchronously invoke its constructor 
> +PASS Inserting a custom element into a cloned document using HTML fragment parsing must enqueue a custom element upgrade reaction, not synchronously invoke its constructor 
> +PASS Inserting a custom element into a document created by createHTMLDocument using HTML fragment parsing must enqueue a custom element upgrade reaction, not synchronously invoke its constructor 
> +PASS Inserting a custom element into an HTML document created by createDocument using HTML fragment parsing must enqueue a custom element upgrade reaction, not synchronously invoke its constructor 
> +PASS Inserting a custom element into the document of an iframe using HTML fragment parsing must enqueue a custom element upgrade reaction, not synchronously invoke its constructor 
> +PASS Inserting a custom element into an HTML document fetched by XHR using HTML fragment parsing must enqueue a custom element upgrade reaction, not synchronously invoke its constructor 

Yay!

[Ryosuke Niwa]

Thanks for the review.

(In reply to Daniel Bates from comment #3)
> Comment on attachment 346578 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346578&action=review
> 
> Looks sane to me.
> 
> > Source/WebCore/ChangeLog:13
> > +        Namely, when the HTML parser creates an element for a token, *will execute script* will be set to false in
> > +        the fragment parsing algorithm, and creates an element with synchronous custom elements flag *not* set:
> 
> This sentence does not read well. In particular, the "and creates an
> element" does not read well.

Okay, revised to say:

The fragment parsing algorithm creates an element for a token with *will execute script* flag set to false:
https://html.spec.whatwg.org/multipage/parsing.html#create-an-element-for-the-token
which results in creating an element with synchronous custom elements flag *not* set:
https://dom.spec.whatwg.org/#concept-create-element

> 
> > Source/WebCore/ChangeLog:21
> > +        enqueued to upgrade is enqueued to upgarde for the second time. In this case, we need to put the element to the
> 
> upgarde => upgrade
> 
> "to the" => "into the"

Fixed.

> > Source/WebCore/ChangeLog:24
> > +        While the specification simply enqueues another upgarde reaction and bails out immediately in the first step of
> 
> upgarde => upgrade

Fixed.

> > Source/WebCore/ChangeLog:46
> > +        was synchrnously constructing a custom element even for the fragment parsing algorithm.
> 
> synchrnously => synchronously

Fixed.

> > Source/WebCore/dom/CustomElementReactionQueue.cpp:126
> > +    if (alreadyScheduledToUpgrade) {
> > +        ASSERT(queue.m_items.size() == 1);
> > +        ASSERT(queue.m_items[0].type() == CustomElementReactionQueueItem::Type::ElementUpgrade);
> > +    } else
> > +        queue.m_items.append({CustomElementReactionQueueItem::Type::ElementUpgrade});
> 
> I wish there was a better way we could design this code such that we
> wouldn't need to have alreadyScheduledToUpgrade. Putting this aside lets
> look at this code. Although the compiler is smart enough to optimize out the
> branch on alreadyScheduledToUpgrade in a release build the existence of
> alreadyScheduledToUpgrade is only meaningful in a debug build (for the
> assertion), its unnecessary to write out such a branch in source code.

No. The boolean flag is useful in release build as well because we need to avoid inserting a new reaction to queue.m_items.append but we still have to call ensureCurrentQueue.

> (Disregarding code style, I suspect another reason you wrote it this way was
> to avoid an unused argument compiler warning when building release). For
> your consideration, I suggest that we make use of the convenience macro
> ASSERT_UNUSED() and then simplify this code to read:
> 
> ASSERT_UNUSED(alreadyScheduledToUpgrade, !alreadyScheduledToUpgrade ||
> queue.m_items.size() == 1);
> ASSERT_UNUSED(alreadyScheduledToUpgrade, !alreadyScheduledToUpgrade ||
> queue.m_items[0].type() ==
> CustomElementReactionQueueItem::Type::ElementUpgrade);
> queue.m_items.append({ CustomElementReactionQueueItem::Type::ElementUpgrade
> });
> 
> (Notice that I also took stylistic liberty to put space characters after the
> '{' and before the '}' in the last line as it's an unwritten convention to
> put such spaces).

Again, this code is wrong because it inserts a new item to queue.m_items. If we did that, then we'd have to modify JSCustomElementInterface::upgradeElement to exit early as the spec does, which incurs necessary heap allocation and a function call.

> On another note, is queue.m_items a public field? If so, it is an unwritten
> convention to omit the "m_" prefix from the name of public member fields. We
> do not need to fix this up in this patch.

It's not. m_items is a private member of CustomElementReactionQueue.

> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:681
> > +            element = HTMLElement::create(QualifiedName(nullAtom(), localName, xhtmlNamespaceURI), ownerDocument);
> 
> Stylistic nit (code is fine as-is): There is a growing movement of using
> uniform initialization syntax for new code.
> 
> > Source/WebCore/html/parser/HTMLConstructionSite.cpp:685
> > +            QualifiedName qualifiedName(nullAtom(), localName, xhtmlNamespaceURI);
> 
> Stylistic nit (code is fine as-is): There is a growing movement of using
> uniform initialization syntax for new code.

Fixed.

[Ryosuke Niwa]

Committed r234577: <https://trac.webkit.org/changeset/234577>

[Daniel Bates]

Comment on attachment 346578 [details]
Fixes the bug

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

>>> Source/WebCore/dom/CustomElementReactionQueue.cpp:126
>>> +        queue.m_items.append({CustomElementReactionQueueItem::Type::ElementUpgrade});
>> 
>> I wish there was a better way we could design this code such that we wouldn't need to have alreadyScheduledToUpgrade. Putting this aside lets look at this code. Although the compiler is smart enough to optimize out the branch on alreadyScheduledToUpgrade in a release build the existence of alreadyScheduledToUpgrade is only meaningful in a debug build (for the assertion), its unnecessary to write out such a branch in source code. (Disregarding code style, I suspect another reason you wrote it this way was to avoid an unused argument compiler warning when building release). For your consideration, I suggest that we make use of the convenience macro ASSERT_UNUSED() and then simplify this code to read:
>> 
>> ASSERT_UNUSED(alreadyScheduledToUpgrade, !alreadyScheduledToUpgrade || queue.m_items.size() == 1);
>> ASSERT_UNUSED(alreadyScheduledToUpgrade, !alreadyScheduledToUpgrade || queue.m_items[0].type() == CustomElementReactionQueueItem::Type::ElementUpgrade);
>> queue.m_items.append({ CustomElementReactionQueueItem::Type::ElementUpgrade });
>> 
>> (Notice that I also took stylistic liberty to put space characters after the '{' and before the '}' in the last line as it's an unwritten convention to put such spaces).
>> 
>> On another note, is queue.m_items a public field? If so, it is an unwritten convention to omit the "m_" prefix from the name of public member fields. We do not need to fix this up in this patch.
> 
> No. The boolean flag is useful in release build as well because we need to avoid inserting a new reaction to queue.m_items.append but we still have to call ensureCurrentQueue.

You’re right! I’m blind.