innerHTML should not synchronously create a custom element
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 4 Aug 2018 06:35:37 +0000 (06:35 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 4 Aug 2018 06:35:37 +0000 (06:35 +0000)
https://bugs.webkit.org/show_bug.cgi?id=188327
<rdar://problem/42923114>

Reviewed by Daniel Bates.

LayoutTests/imported/w3c:

Rebaselined the test now that all test cases are passing.

* web-platform-tests/custom-elements/connected-callbacks-html-fragment-parsing-expected.txt:

Source/WebCore:

Fixed the bug that the fragment parsing algorithm was synchronously constructing a custom element instead of
enqueuing an element to upgrade.

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

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 upgrade for the second time. In this case, we need to put the element into the
current element queue (https://html.spec.whatwg.org/multipage/custom-elements.html#current-element-queue) again.

While the specification simply enqueues another upgrade 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 synchronously constructing a custom element even for the fragment parsing algorithm.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@234577 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/custom-elements/connected-callbacks-html-fragment-parsing-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/dom/CustomElementReactionQueue.cpp
Source/WebCore/dom/CustomElementReactionQueue.h
Source/WebCore/dom/Element.cpp
Source/WebCore/html/parser/HTMLConstructionSite.cpp

index 2409531..1bc0aa3 100644 (file)
@@ -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 Daniel Bates.
+
+        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 266d683..3d43b60 100644 (file)
@@ -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 
 
index 4477e6c..bc49dea 100644 (file)
@@ -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 Daniel Bates.
+
+        Fixed the bug that the fragment parsing algorithm was synchronously constructing a custom element instead of
+        enqueuing an element to upgrade.
+
+        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
+
+        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 upgrade for the second time. In this case, we need to put the element into the
+        current element queue (https://html.spec.whatwg.org/multipage/custom-elements.html#current-element-queue) again.
+
+        While the specification simply enqueues another upgrade 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 synchronously constructing a custom element even for the fragment parsing algorithm.
+
 2018-08-03  Ben Richards  <benton_richards@apple.com>
 
         We should cache the compiled sandbox profile in a data vault
index 1c75f3f..4e83501 100644 (file)
@@ -70,6 +70,8 @@ public:
         , m_newValue(newValue)
     { }
 
+    Type type() const { return m_type; }
+
     void invoke(Element& element, JSCustomElementInterface& elementInterface)
     {
         switch (m_type) {
@@ -115,10 +117,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 c6768e3..ac83b09 100644 (file)
@@ -49,7 +49,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 4549e87..79cecb5 100644 (file)
@@ -2009,10 +2009,10 @@ void Element::enqueueToUpgrade(JSCustomElementInterface& elementInterface)
     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 023c8fd..8c03f27 100644 (file)
@@ -649,6 +649,19 @@ inline Document& HTMLConstructionSite::ownerDocumentForCurrentNode()
     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::createHTMLElementOrFindCustomElementInterf
     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);