Custom elements in a reaction queue can lose its JS wrapper and become HTMLUnknownElement
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 22 Sep 2018 00:17:17 +0000 (00:17 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 22 Sep 2018 00:17:17 +0000 (00:17 +0000)
https://bugs.webkit.org/show_bug.cgi?id=184307

Reviewed by Keith Miller.

Source/WebCore:

The bug was caused by the custom elements reaction queue not reporting its content to GC during marking.

When there is no JS reference to the JS wrappers of those custom element, and if those custom elements
are disconnected, GC would happily collect those the wrappers. Unfortunately, the same bug exists for
any asynchronous events and other WebCore code which keeps elements alive for a later use but doesn't
report them to GC (e.g. during visitChildren).

This patch, therefore, introduces a generic mechanism to keep these elements' wrappers alive. Namely,
we introduce GCReachableRef, a new smart pointer type for Node's subclasses, which keeps element as well
as its wrappers alive. GCReachableRef works by adding its Node to a global hash counted set when it's
created and making JSNodeOwner::isReachableFromOpaqueRoots return true when the node is in the set.

Test: fast/custom-elements/custom-elements-reaction-queue-retains-js-wrapper.html

* Sources.txt:
* WebCore.xcodeproj/project.pbxproj:
* bindings/js/JSNodeCustom.cpp:
(WebCore::isReachableFromDOM):
* dom/CustomElementReactionQueue.cpp:
(WebCore::CustomElementReactionQueue::ElementQueue::invokeAll): Don't swap the vector of elements in
in the queue. Assuming each DOM API has an appropriate CustomElementsReactionStack, we should never
append a new element to this queue while invoking custom element reactions.
* dom/CustomElementReactionQueue.h:
* dom/GCReachableRef.cpp: Added.
* dom/GCReachableRef.h: Added.
(WebCore::GCReachableRefMap::contains): Added.
(WebCore::GCReachableRefMap::add): Added.
(WebCore::GCReachableRefMap::remove): Added.
(WebCore::GCReachableRef::GCReachableRef): Added. We need isNull() check since WTFMove may have been
called on the source GCReachableRef.
(WebCore::GCReachableRef::~GCReachableRef): Ditto.
(WebCore::GCReachableRef::operator-> const): Added.
(WebCore::GCReachableRef::get const): Added.
(WebCore::GCReachableRef::operator T& const): Added.
(WebCore::GCReachableRef::operator! const): Added.
(WebCore::GCReachableRef::isNull const): Added. Returns true if WTFMove had been called on Ref.

LayoutTests:

Added a test for enqueuing a lot of custom elements into the reaction queue via innerHTML setter.
WebKit should retain the JS wrappers of all custom elements.

* fast/custom-elements/custom-elements-reaction-queue-retains-js-wrapper-expected.txt: Added.
* fast/custom-elements/custom-elements-reaction-queue-retains-js-wrapper.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/custom-elements/custom-elements-reaction-queue-retains-js-wrapper-expected.txt [new file with mode: 0644]
LayoutTests/fast/custom-elements/custom-elements-reaction-queue-retains-js-wrapper.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/Sources.txt
Source/WebCore/WebCore.xcodeproj/project.pbxproj
Source/WebCore/bindings/js/JSNodeCustom.cpp
Source/WebCore/dom/CustomElementReactionQueue.cpp
Source/WebCore/dom/CustomElementReactionQueue.h
Source/WebCore/dom/GCReachableRef.cpp [new file with mode: 0644]
Source/WebCore/dom/GCReachableRef.h [new file with mode: 0644]

index 0977486..9460b23 100644 (file)
@@ -1,3 +1,16 @@
+2018-09-21  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Custom elements in a reaction queue can lose its JS wrapper and become HTMLUnknownElement
+        https://bugs.webkit.org/show_bug.cgi?id=184307
+
+        Reviewed by Keith Miller.
+
+        Added a test for enqueuing a lot of custom elements into the reaction queue via innerHTML setter.
+        WebKit should retain the JS wrappers of all custom elements.
+
+        * fast/custom-elements/custom-elements-reaction-queue-retains-js-wrapper-expected.txt: Added.
+        * fast/custom-elements/custom-elements-reaction-queue-retains-js-wrapper.html: Added.
+
 2018-09-21  Chris Dumez  <cdumez@apple.com>
 
         Unreviewed, speed up storage/websql/transaction-database-expand-quota.html
diff --git a/LayoutTests/fast/custom-elements/custom-elements-reaction-queue-retains-js-wrapper-expected.txt b/LayoutTests/fast/custom-elements/custom-elements-reaction-queue-retains-js-wrapper-expected.txt
new file mode 100644 (file)
index 0000000..b137e40
--- /dev/null
@@ -0,0 +1,24 @@
+This tests enqueuing a lot of custom elements during innerHTML.
+WebKit should not lose JS wrappers of those custom elements.
+
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+PASS
+
diff --git a/LayoutTests/fast/custom-elements/custom-elements-reaction-queue-retains-js-wrapper.html b/LayoutTests/fast/custom-elements/custom-elements-reaction-queue-retains-js-wrapper.html
new file mode 100644 (file)
index 0000000..d78df7b
--- /dev/null
@@ -0,0 +1,40 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests enqueuing a lot of custom elements during innerHTML.<br>
+WebKit should not lose JS wrappers of those custom elements.</p>
+<pre id="log"></pre>
+<div id="container"><script>
+let failCount = 0;
+class MyComponent extends HTMLElement {
+    disconnectedCallback() {
+        if (!(this instanceof MyComponent))
+            failCount++;
+        const x = new Array(100);
+        for (let i = 0; i < 100; i++)
+            x[i] = new Array(10);
+    }
+}
+customElements.define("my-component", MyComponent);
+
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+let content = '';
+for (let i = 0; i < 50; i++)
+    content += '<my-component></my-component>';
+content += '<my-component id="last"></my-component>'
+
+let first = true;
+for (let test = 0; test < 20; test++) {
+    failCount = 0;
+    for (let i = 0; i < 10; i++)
+        document.getElementById('container').innerHTML = content;
+    document.getElementById('log').append(failCount ? `FAIL - ${failCount} elements lost wrappers\n` : 'PASS\n');
+}
+document.getElementById('container').textContent = '';
+
+</script></div>
+</body>
+</html>
+
index 8091975..05f4115 100644 (file)
@@ -1,3 +1,47 @@
+2018-09-21  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Custom elements in a reaction queue can lose its JS wrapper and become HTMLUnknownElement
+        https://bugs.webkit.org/show_bug.cgi?id=184307
+
+        Reviewed by Keith Miller.
+
+        The bug was caused by the custom elements reaction queue not reporting its content to GC during marking.
+
+        When there is no JS reference to the JS wrappers of those custom element, and if those custom elements
+        are disconnected, GC would happily collect those the wrappers. Unfortunately, the same bug exists for
+        any asynchronous events and other WebCore code which keeps elements alive for a later use but doesn't
+        report them to GC (e.g. during visitChildren).
+
+        This patch, therefore, introduces a generic mechanism to keep these elements' wrappers alive. Namely,
+        we introduce GCReachableRef, a new smart pointer type for Node's subclasses, which keeps element as well
+        as its wrappers alive. GCReachableRef works by adding its Node to a global hash counted set when it's
+        created and making JSNodeOwner::isReachableFromOpaqueRoots return true when the node is in the set.
+
+        Test: fast/custom-elements/custom-elements-reaction-queue-retains-js-wrapper.html
+
+        * Sources.txt:
+        * WebCore.xcodeproj/project.pbxproj:
+        * bindings/js/JSNodeCustom.cpp:
+        (WebCore::isReachableFromDOM):
+        * dom/CustomElementReactionQueue.cpp:
+        (WebCore::CustomElementReactionQueue::ElementQueue::invokeAll): Don't swap the vector of elements in
+        in the queue. Assuming each DOM API has an appropriate CustomElementsReactionStack, we should never
+        append a new element to this queue while invoking custom element reactions.
+        * dom/CustomElementReactionQueue.h:
+        * dom/GCReachableRef.cpp: Added.
+        * dom/GCReachableRef.h: Added.
+        (WebCore::GCReachableRefMap::contains): Added.
+        (WebCore::GCReachableRefMap::add): Added.
+        (WebCore::GCReachableRefMap::remove): Added.
+        (WebCore::GCReachableRef::GCReachableRef): Added. We need isNull() check since WTFMove may have been
+        called on the source GCReachableRef.
+        (WebCore::GCReachableRef::~GCReachableRef): Ditto.
+        (WebCore::GCReachableRef::operator-> const): Added.
+        (WebCore::GCReachableRef::get const): Added.
+        (WebCore::GCReachableRef::operator T& const): Added.
+        (WebCore::GCReachableRef::operator! const): Added.
+        (WebCore::GCReachableRef::isNull const): Added. Returns true if WTFMove had been called on Ref.
+
 2018-09-21  Alex Christensen  <achristensen@webkit.org>
 
         Use a Variant for FormDataElement
index e2bcdf1..2c00030 100644 (file)
@@ -815,6 +815,7 @@ dom/SpectreGadget.cpp
 dom/StaticNodeList.cpp
 dom/StaticRange.cpp
 dom/StringCallback.cpp
+dom/GCReachableRef.cpp
 dom/StyledElement.cpp
 dom/TagCollection.cpp
 dom/TemplateContentDocumentFragment.cpp
index 966c9ad..3a7fabd 100644 (file)
                9B6C41531344949000085B62 /* StringWithDirection.h in Headers */ = {isa = PBXBuildFile; fileRef = 9B6C41521344949000085B62 /* StringWithDirection.h */; settings = {ATTRIBUTES = (Private, ); }; };
                9B714E211C91166900AC0E92 /* EventPath.h in Headers */ = {isa = PBXBuildFile; fileRef = 9B714E1F1C91166900AC0E92 /* EventPath.h */; };
                9BA273F4172206BB0097CE47 /* LogicalSelectionOffsetCaches.h in Headers */ = {isa = PBXBuildFile; fileRef = 9BA273F3172206BB0097CE47 /* LogicalSelectionOffsetCaches.h */; };
+               9BAAC45C21520128003D4A98 /* GCReachableRef.h in Headers */ = {isa = PBXBuildFile; fileRef = 9BAAC4562151E39E003D4A98 /* GCReachableRef.h */; settings = {ATTRIBUTES = (Private, ); }; };
                9BAB6C6C12550631001626D4 /* EditingStyle.h in Headers */ = {isa = PBXBuildFile; fileRef = 9BAB6C6A12550631001626D4 /* EditingStyle.h */; settings = {ATTRIBUTES = (Private, ); }; };
                9BAF3B2412C1A39800014BF1 /* WritingDirection.h in Headers */ = {isa = PBXBuildFile; fileRef = 9BAF3B2312C1A39800014BF1 /* WritingDirection.h */; settings = {ATTRIBUTES = (Private, ); }; };
                9BBA2CAB1F679E0C00FD1C1E /* WebContentReader.h in Headers */ = {isa = PBXBuildFile; fileRef = 9BF433761F67619B00E1FD71 /* WebContentReader.h */; settings = {ATTRIBUTES = (Private, ); }; };
                9B9299B01F6796A4006723C2 /* WebContentReaderCocoa.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = WebContentReaderCocoa.mm; sourceTree = "<group>"; };
                9BA273F3172206BB0097CE47 /* LogicalSelectionOffsetCaches.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = LogicalSelectionOffsetCaches.h; sourceTree = "<group>"; };
                9BA827781F06156500F71E75 /* NavigationDisabler.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = NavigationDisabler.h; sourceTree = "<group>"; };
+               9BAAC4562151E39E003D4A98 /* GCReachableRef.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = GCReachableRef.h; sourceTree = "<group>"; };
+               9BAAC4582151E77A003D4A98 /* GCReachableRef.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = GCReachableRef.cpp; sourceTree = "<group>"; };
                9BAB6C6A12550631001626D4 /* EditingStyle.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = EditingStyle.h; sourceTree = "<group>"; };
                9BAB6C6B12550631001626D4 /* EditingStyle.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = EditingStyle.cpp; sourceTree = "<group>"; };
                9BAF3B2312C1A39800014BF1 /* WritingDirection.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = WritingDirection.h; sourceTree = "<group>"; };
                                8102C5871325BB1100DDE67A /* StringCallback.cpp */,
                                81AC6C35131C57D30009A7E0 /* StringCallback.h */,
                                81AC6C34131C57C20009A7E0 /* StringCallback.idl */,
+                               9BAAC4582151E77A003D4A98 /* GCReachableRef.cpp */,
+                               9BAAC4562151E39E003D4A98 /* GCReachableRef.h */,
                                A8C4A7EC09D563270003AC8D /* StyledElement.cpp */,
                                A8C4A7EB09D563270003AC8D /* StyledElement.h */,
                                C99058121E32B7340073BDDA /* SuccessOr.h */,
                                ECA680C71E67724500731D20 /* StringUtilities.h in Headers */,
                                9B6C41531344949000085B62 /* StringWithDirection.h in Headers */,
                                849F77760EFEC6200090849D /* StrokeStyleApplier.h in Headers */,
+                               9BAAC45C21520128003D4A98 /* GCReachableRef.h in Headers */,
                                414B82051D6DF0E50077EBE3 /* StructuredClone.h in Headers */,
                                BC5EB6A30E81DC4F00B25965 /* StyleBackgroundData.h in Headers */,
                                BC5EB67B0E81D3BE00B25965 /* StyleBoxData.h in Headers */,
index c485e30..894b346 100644 (file)
@@ -61,6 +61,7 @@
 #include "SVGElement.h"
 #include "ScriptState.h"
 #include "ShadowRoot.h"
+#include "GCReachableRef.h"
 #include "StyleSheet.h"
 #include "StyledElement.h"
 #include "Text.h"
@@ -107,6 +108,11 @@ static inline bool isReachableFromDOM(Node* node, SlotVisitor& visitor, const ch
                 *reason = "Node which is firing event listeners";
             return true;
         }
+        if (GCReachableRefMap::contains(*node)) {
+            if (UNLIKELY(reason))
+                *reason = "Node is scheduled to be used in an async script invocation)";
+            return true;
+        }
     }
 
     if (UNLIKELY(reason))
index c186ff5..243a806 100644 (file)
@@ -234,15 +234,14 @@ inline void CustomElementReactionQueue::ElementQueue::invokeAll()
 {
     RELEASE_ASSERT(!m_invoking);
     SetForScope<bool> invoking(m_invoking, true);
-    Vector<Ref<Element>> elements;
-    elements.swap(m_elements);
-    RELEASE_ASSERT(m_elements.isEmpty());
-    for (auto& element : elements) {
+    auto originalSize = m_elements.size();
+    for (auto& element : m_elements) {
         auto* queue = element->reactionQueue();
         ASSERT(queue);
         queue->invokeAll(element.get());
     }
-    RELEASE_ASSERT(m_elements.isEmpty());
+    RELEASE_ASSERT(m_elements.size() == originalSize);
+    m_elements.clear();
 }
 
 inline void CustomElementReactionQueue::ElementQueue::processQueue(JSC::ExecState* state)
index b4e38dc..a57ec5f 100644 (file)
@@ -25,6 +25,7 @@
 
 #pragma once
 
+#include "GCReachableRef.h"
 #include <wtf/Forward.h>
 #include <wtf/Noncopyable.h>
 #include <wtf/Vector.h>
@@ -71,7 +72,7 @@ public:
     private:
         void invokeAll();
 
-        Vector<Ref<Element>> m_elements;
+        Vector<GCReachableRef<Element>> m_elements;
         bool m_invoking { false };
     };
 
diff --git a/Source/WebCore/dom/GCReachableRef.cpp b/Source/WebCore/dom/GCReachableRef.cpp
new file mode 100644 (file)
index 0000000..1310100
--- /dev/null
@@ -0,0 +1,39 @@
+/*
+ * Copyright (C) 2018 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#include "config.h"
+#include "GCReachableRef.h"
+
+#include <wtf/NeverDestroyed.h>
+
+namespace WebCore {
+
+HashCountedSet<Node*>& GCReachableRefMap::map()
+{
+    static NeverDestroyed<HashCountedSet<Node*>> map;
+    return map;
+}
+
+} // namespace WebCore
diff --git a/Source/WebCore/dom/GCReachableRef.h b/Source/WebCore/dom/GCReachableRef.h
new file mode 100644 (file)
index 0000000..0ebb0ee
--- /dev/null
@@ -0,0 +1,91 @@
+/*
+ * Copyright (C) 2013-2018 Apple Inc. All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. AND ITS CONTRIBUTORS ``AS IS''
+ * AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL APPLE INC. OR ITS CONTRIBUTORS
+ * BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR
+ * CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF
+ * SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
+ * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN
+ * CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF
+ * THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+#pragma once
+
+#include <wtf/DumbPtrTraits.h>
+#include <wtf/HashCountedSet.h>
+#include <wtf/Ref.h>
+
+namespace WebCore {
+
+class Node;
+
+class GCReachableRefMap {
+public:
+    static inline bool contains(Node& node) { return map().contains(&node); }
+    static inline void add(Node& node) { map().add(&node); }
+    static inline void remove(Node& node) { map().remove(&node); }
+
+private:
+    static HashCountedSet<Node*>& map();
+};
+
+template <typename T, typename = std::enable_if_t<std::is_same<T, typename std::remove_const<T>::type>::value>>
+class GCReachableRef {
+    WTF_MAKE_NONCOPYABLE(GCReachableRef);
+public:
+
+    template<typename = std::enable_if_t<std::is_base_of<Node, T>::value>>
+    GCReachableRef(T& object)
+        : m_ref(object)
+    {
+        GCReachableRefMap::add(m_ref.get());
+    }
+
+    ~GCReachableRef()
+    {
+        if (!isNull())
+            GCReachableRefMap::remove(m_ref.get());
+    }
+
+    template<typename X, typename Y, typename = std::enable_if_t<std::is_base_of<Node, T>::value>>
+    GCReachableRef(Ref<X, Y>&& other)
+        : m_ref(WTFMove(other.m_ref))
+    {
+        if (!isNull())
+            GCReachableRefMap::add(m_ref.get());
+    }
+
+    GCReachableRef(GCReachableRef&& other)
+        : m_ref(WTFMove(other.m_ref))
+    {
+    }
+
+    template<typename X, typename Y> GCReachableRef(const GCReachableRef<X, Y>& other) = delete;
+
+    T* operator->() const { ASSERT(!isNull()); return m_ref.ptr(); }
+    T* ptr() const RETURNS_NONNULL { ASSERT(!isNull()); return m_ref.ptr(); }
+    T& get() const { ASSERT(!isNull()); return m_ref.get(); }
+    operator T&() const { ASSERT(!isNull()); return m_ref.get(); }
+    bool operator!() const { ASSERT(!isNull()); return !m_ref.get(); }
+
+private:
+    bool isNull() const { return m_ref.isHashTableEmptyValue(); }
+
+    Ref<T> m_ref;
+};
+
+}