Attachment 346578 Details for Bug 188327 – Fixes the bug

[patch] Fixes the bug

bug-188327-20180803194031.patch (text/plain), 13.76 KB, created by Ryosuke Niwa on 2018-08-03 19:40:31 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Ryosuke Niwa

Created: 2018-08-03 19:40:31 PDT

Size: 13.76 KB

patch

obsolete

>Index: Source/WebCore/ChangeLog
>===================================================================
>--- Source/WebCore/ChangeLog	(revision 234538)
>+++ Source/WebCore/ChangeLog	(working copy)
>@@ -1,3 +1,50 @@
>+2018-08-03  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        innerHTML should not synchronously create a custom element
>+        https://bugs.webkit.org/show_bug.cgi?id=188327
>+        <rdar://problem/42923114>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Fixed the bug that the fragment parsing algorithm was synchronously constructing a custom element instead of
>+        enqueuing an element to upgrade.
>+
>+        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:
>+        https://html.spec.whatwg.org/multipage/parsing.html#create-an-element-for-the-token
>+        https://dom.spec.whatwg.org/#concept-create-element
>+
>+        When synchronous custom elements flag is false, we're supposed to create an element and enqueue a custom element
>+        upgrade reaction. createHTMLElementOrFindCustomElementInterface was missing this last logic. 
>+
>+        Also fixed a bug that Element::enqueueToUpgrade would hit a debug assertion when a custom element which has been
>+        enqueued to upgrade is enqueued to upgarde for the second time. In this case, we need to put the element to the
>+        current element queue (https://html.spec.whatwg.org/multipage/custom-elements.html#current-element-queue) again.
>+
>+        While the specification simply enqueues another upgarde reaction and bails out immediately in the first step of
>+        the upgrade, WebKit's implementation simply avoids this redundancy in the first place:
>+        https://html.spec.whatwg.org/multipage/custom-elements.html#concept-upgrade-an-element
>+
>+        Existing tests such as imported/w3c/web-platform-tests/custom-elements/reactions/Document.html exercises this
>+        code path after the fragment parsing algorithm fix.
>+
>+        Tests: imported/w3c/web-platform-tests/custom-elements/connected-callbacks-html-fragment-parsing.html
>+
>+        * dom/CustomElementReactionQueue.cpp:
>+        (WebCore::CustomElementReactionQueueItem::type const): Added for an assertion.
>+        (WebCore::CustomElementReactionQueue::enqueueElementUpgrade): Enqueue this element to the current element queue
>+        by calling ensureCurrentQueue and avoid inserting a redundant upgrade reaction.
>+        * dom/CustomElementReactionQueue.h:
>+        * dom/Element.cpp:
>+        (WebCore::Element::enqueueToUpgrade): Handle the case when a custom element is enqueued to upgrade for the second
>+        time while it had been waiting in some element queue. In this case, the reaction queue for this element has
>+        already been created and we simply need to put this element back into the current element queue (i.e. this element
>+        now belongs to both element queues).
>+        * html/parser/HTMLConstructionSite.cpp:
>+        (WebCore::findCustomElementInterface): Extracted out of createHTMLElementOrFindCustomElementInterface.
>+        (WebCore::HTMLConstructionSite::createHTMLElementOrFindCustomElementInterface): Fixed the bug that the HTML parser
>+        was synchrnously constructing a custom element even for the fragment parsing algorithm.
>+
> 2018-08-02  Zalan Bujtas  <zalan@apple.com>
> 
>         [LFC][BFC] Apply the "10.6.6 Complicated cases" when computing height and margin for the document renderer
>Index: Source/WebCore/dom/CustomElementReactionQueue.cpp
>===================================================================
>--- Source/WebCore/dom/CustomElementReactionQueue.cpp	(revision 234538)
>+++ Source/WebCore/dom/CustomElementReactionQueue.cpp	(working copy)
>@@ -69,6 +69,8 @@ public:
>         , m_newValue(newValue)
>     { }
> 
>+    Type type() const { return m_type; }
>+
>     void invoke(Element& element, JSCustomElementInterface& elementInterface)
>     {
>         switch (m_type) {
>@@ -114,10 +116,14 @@ void CustomElementReactionQueue::clear()
>     m_items.clear();
> }
> 
>-void CustomElementReactionQueue::enqueueElementUpgrade(Element& element)
>+void CustomElementReactionQueue::enqueueElementUpgrade(Element& element, bool alreadyScheduledToUpgrade)
> {
>     auto& queue = ensureCurrentQueue(element);
>-    queue.m_items.append({CustomElementReactionQueueItem::Type::ElementUpgrade});
>+    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});
> }
> 
> void CustomElementReactionQueue::enqueueElementUpgradeIfDefined(Element& element)
>Index: Source/WebCore/dom/CustomElementReactionQueue.h
>===================================================================
>--- Source/WebCore/dom/CustomElementReactionQueue.h	(revision 234538)
>+++ Source/WebCore/dom/CustomElementReactionQueue.h	(working copy)
>@@ -43,7 +43,7 @@ public:
>     CustomElementReactionQueue(JSCustomElementInterface&);
>     ~CustomElementReactionQueue();
> 
>-    static void enqueueElementUpgrade(Element&);
>+    static void enqueueElementUpgrade(Element&, bool alreadyScheduledToUpgrade);
>     static void enqueueElementUpgradeIfDefined(Element&);
>     static void enqueueConnectedCallbackIfNeeded(Element&);
>     static void enqueueDisconnectedCallbackIfNeeded(Element&);
>Index: Source/WebCore/dom/Element.cpp
>===================================================================
>--- Source/WebCore/dom/Element.cpp	(revision 234538)
>+++ Source/WebCore/dom/Element.cpp	(working copy)
>@@ -2009,10 +2009,10 @@ void Element::enqueueToUpgrade(JSCustomE
>     InspectorInstrumentation::didChangeCustomElementState(*this);
> 
>     auto& data = ensureElementRareData();
>-    ASSERT(!data.customElementReactionQueue());
>-
>-    data.setCustomElementReactionQueue(std::make_unique<CustomElementReactionQueue>(elementInterface));
>-    data.customElementReactionQueue()->enqueueElementUpgrade(*this);
>+    bool alreadyScheduledToUpgrade = data.customElementReactionQueue();
>+    if (!alreadyScheduledToUpgrade)
>+        data.setCustomElementReactionQueue(std::make_unique<CustomElementReactionQueue>(elementInterface));
>+    data.customElementReactionQueue()->enqueueElementUpgrade(*this, alreadyScheduledToUpgrade);
> }
> 
> CustomElementReactionQueue* Element::reactionQueue() const
>Index: Source/WebCore/html/parser/HTMLConstructionSite.cpp
>===================================================================
>--- Source/WebCore/html/parser/HTMLConstructionSite.cpp	(revision 234538)
>+++ Source/WebCore/html/parser/HTMLConstructionSite.cpp	(working copy)
>@@ -649,6 +649,19 @@ inline Document& HTMLConstructionSite::o
>     return currentNode().document();
> }
> 
>+static inline JSCustomElementInterface* findCustomElementInterface(Document& ownerDocument, const AtomicString& localName)
>+{
>+    auto* window = ownerDocument.domWindow();
>+    if (!window)
>+        return nullptr;
>+
>+    auto* registry = window->customElementRegistry();
>+    if (LIKELY(!registry))
>+        return nullptr;
>+
>+    return registry->findInterface(localName);
>+}
>+
> RefPtr<Element> HTMLConstructionSite::createHTMLElementOrFindCustomElementInterface(AtomicHTMLToken& token, JSCustomElementInterface** customElementInterface)
> {
>     auto& localName = token.name();
>@@ -660,23 +673,22 @@ RefPtr<Element> HTMLConstructionSite::cr
>     bool insideTemplateElement = !ownerDocument.frame();
>     RefPtr<Element> element = HTMLElementFactory::createKnownElement(localName, ownerDocument, insideTemplateElement ? nullptr : form(), true);
>     if (UNLIKELY(!element)) {
>-        auto window = makeRefPtr(ownerDocument.domWindow());
>-        if (customElementInterface && window) {
>-            auto registry = makeRefPtr(window->customElementRegistry());
>-            if (UNLIKELY(registry)) {
>-                if (auto elementInterface = makeRefPtr(registry->findInterface(localName))) {
>-                    *customElementInterface = elementInterface.get();
>-                    return nullptr;
>-                }
>+        if (auto* elementInterface = findCustomElementInterface(ownerDocument, localName)) {
>+            if (!m_isParsingFragment) {
>+                *customElementInterface = elementInterface;
>+                return nullptr;
>             }
>-        }
>-
>-        QualifiedName qualifiedName(nullAtom(), localName, xhtmlNamespaceURI);
>-        if (Document::validateCustomElementName(localName) == CustomElementNameValidationStatus::Valid) {
>-            element = HTMLElement::create(qualifiedName, ownerDocument);
>+            element = HTMLElement::create(QualifiedName(nullAtom(), localName, xhtmlNamespaceURI), ownerDocument);
>             element->setIsCustomElementUpgradeCandidate();
>-        } else
>-            element = HTMLUnknownElement::create(qualifiedName, ownerDocument);
>+            element->enqueueToUpgrade(*elementInterface);
>+        } else {
>+            QualifiedName qualifiedName(nullAtom(), localName, xhtmlNamespaceURI);
>+            if (Document::validateCustomElementName(localName) == CustomElementNameValidationStatus::Valid) {
>+                element = HTMLElement::create(qualifiedName, ownerDocument);
>+                element->setIsCustomElementUpgradeCandidate();
>+            } else
>+                element = HTMLUnknownElement::create(qualifiedName, ownerDocument);
>+        }
>     }
>     ASSERT(element);
> 
>Index: LayoutTests/imported/w3c/ChangeLog
>===================================================================
>--- LayoutTests/imported/w3c/ChangeLog	(revision 234575)
>+++ LayoutTests/imported/w3c/ChangeLog	(working copy)
>@@ -1,3 +1,15 @@
>+2018-08-03  Ryosuke Niwa  <rniwa@webkit.org>
>+
>+        innerHTML should not synchronously create a custom element
>+        https://bugs.webkit.org/show_bug.cgi?id=188327
>+        <rdar://problem/42923114>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Rebaselined the test now that all test cases are passing.
>+
>+        * web-platform-tests/custom-elements/connected-callbacks-html-fragment-parsing-expected.txt:
>+
> 2018-08-02  Ryosuke Niwa  <rniwa@webkit.org>
> 
>         Release assert when throwing exceptions in custom element reactions
>Index: LayoutTests/imported/w3c/web-platform-tests/custom-elements/connected-callbacks-html-fragment-parsing-expected.txt
>===================================================================
>--- LayoutTests/imported/w3c/web-platform-tests/custom-elements/connected-callbacks-html-fragment-parsing-expected.txt	(revision 234538)
>+++ LayoutTests/imported/w3c/web-platform-tests/custom-elements/connected-callbacks-html-fragment-parsing-expected.txt	(working copy)
>@@ -1,10 +1,10 @@
> 
>-FAIL Inserting a custom element into the document using HTML fragment parsing must enqueue a custom element upgrade reaction, not synchronously invoke its constructor assert_equals: expected 1 but got 2
>-FAIL 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 assert_equals: expected 1 but got 2
>-FAIL Inserting a custom element into a new document using HTML fragment parsing must enqueue a custom element upgrade reaction, not synchronously invoke its constructor assert_equals: expected 1 but got 2
>-FAIL Inserting a custom element into a cloned document using HTML fragment parsing must enqueue a custom element upgrade reaction, not synchronously invoke its constructor assert_equals: expected 1 but got 2
>-FAIL 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 assert_equals: expected 1 but got 2
>-FAIL 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 assert_equals: expected 1 but got 2
>-FAIL 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 assert_equals: expected 1 but got 2
>-FAIL 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 assert_equals: expected 1 but got 2
>+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 
>

Flags:

dbates: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 188327: 346578