Bug 188327

<rdar://problem/42923114>

Created attachment 346578 [details] Fixes the bug

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!

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.

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

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.