MutationRecord doesn't keep JS wrappers of target, addedNodes, and removedNodes alive
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 4 Oct 2018 21:12:04 +0000 (21:12 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 4 Oct 2018 21:12:04 +0000 (21:12 +0000)
https://bugs.webkit.org/show_bug.cgi?id=190277

Reviewed by Antti Koivisto.

Source/WebCore:

The bug was caused by JSMutationRecord not visiting any of the nodes referenced by mutation records.

Fixed the bug by adding JSMutationRecord::visitAdditionalChildren, which adds the root nodes of
the root nodes of the target, addedNodes, and removedNodes in each mutation record.

Test: fast/dom/MutationObserver/mutation-record-keeps-js-wrappers-of-nodes-alive.html

* Sources.txt:
* WebCore.xcodeproj/project.pbxproj:
* bindings/js/JSMutationRecordCustom.cpp: Added.
(WebCore::JSMutationRecord::visitAdditionalChildren): Added.
* bindings/js/JSPerformanceObserverCustom.cpp: This file got dumped out of a unified build file
where using namespace JSC was defined. Use the fully qualified names to refer to JSC types.
(WebCore::JSPerformanceObserverOwner::isReachableFromOpaqueRoots):
* dom/MutationRecord.cpp:
(WebCore::ChildListRecord::visitNodesConcurrently): Added.
(WebCore::RecordWithEmptyNodeLists::visitNodesConcurrently): Added.
(WebCore::MutationRecordWithNullOldValue::visitNodesConcurrently): Added.
* dom/MutationRecord.h:
* dom/MutationRecord.idl:

LayoutTests:

Added two regression tests for making sure mutation observers and mutation records keep JS wrappers
of the enqueued nodes alive. Also see r236799 for a previous failed attempt to add a similar test.

* fast/dom/MutationObserver/mutation-observer-keeps-js-wrappers-of-nodes-alive-expected.txt: Added.
* fast/dom/MutationObserver/mutation-observer-keeps-js-wrappers-of-nodes-alive.html: Added.
* fast/dom/MutationObserver/mutation-record-keeps-js-wrappers-of-nodes-alive-expected.txt: Added.
* fast/dom/MutationObserver/mutation-record-keeps-js-wrappers-of-nodes-alive.html: Added.

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

13 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/dom/MutationObserver/mutation-observer-keeps-js-wrappers-of-nodes-alive-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/MutationObserver/mutation-observer-keeps-js-wrappers-of-nodes-alive.html [new file with mode: 0644]
LayoutTests/fast/dom/MutationObserver/mutation-record-keeps-js-wrappers-of-nodes-alive-expected.txt [new file with mode: 0644]
LayoutTests/fast/dom/MutationObserver/mutation-record-keeps-js-wrappers-of-nodes-alive.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/Sources.txt
Source/WebCore/WebCore.xcodeproj/project.pbxproj
Source/WebCore/bindings/js/JSMutationRecordCustom.cpp [new file with mode: 0644]
Source/WebCore/bindings/js/JSPerformanceObserverCustom.cpp
Source/WebCore/dom/MutationRecord.cpp
Source/WebCore/dom/MutationRecord.h
Source/WebCore/dom/MutationRecord.idl

index 99052d5..c7b9fb6 100644 (file)
@@ -1,3 +1,18 @@
+2018-10-03  Ryosuke Niwa  <rniwa@webkit.org>
+
+        MutationRecord doesn't keep JS wrappers of target, addedNodes, and removedNodes alive
+        https://bugs.webkit.org/show_bug.cgi?id=190277
+
+        Reviewed by Antti Koivisto.
+
+        Added two regression tests for making sure mutation observers and mutation records keep JS wrappers
+        of the enqueued nodes alive. Also see r236799 for a previous failed attempt to add a similar test.
+
+        * fast/dom/MutationObserver/mutation-observer-keeps-js-wrappers-of-nodes-alive-expected.txt: Added.
+        * fast/dom/MutationObserver/mutation-observer-keeps-js-wrappers-of-nodes-alive.html: Added.
+        * fast/dom/MutationObserver/mutation-record-keeps-js-wrappers-of-nodes-alive-expected.txt: Added.
+        * fast/dom/MutationObserver/mutation-record-keeps-js-wrappers-of-nodes-alive.html: Added.
+
 2018-10-04  Devin Rousso  <drousso@apple.com>
 
         Web Inspector: merge ProbeManager into DebuggerManager
diff --git a/LayoutTests/fast/dom/MutationObserver/mutation-observer-keeps-js-wrappers-of-nodes-alive-expected.txt b/LayoutTests/fast/dom/MutationObserver/mutation-observer-keeps-js-wrappers-of-nodes-alive-expected.txt
new file mode 100644 (file)
index 0000000..48b2682
--- /dev/null
@@ -0,0 +1,8 @@
+This tests that JS wrappers of nodes in a mutation record do not get collected.
+
+PASS
+PASS
+PASS
+PASS
+PASS
+
diff --git a/LayoutTests/fast/dom/MutationObserver/mutation-observer-keeps-js-wrappers-of-nodes-alive.html b/LayoutTests/fast/dom/MutationObserver/mutation-observer-keeps-js-wrappers-of-nodes-alive.html
new file mode 100644 (file)
index 0000000..1e8ebd8
--- /dev/null
@@ -0,0 +1,61 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests that JS wrappers of nodes in a mutation record do not get collected.</p>
+<pre id="log"></pre>
+<script src="../../../resources/gc.js"></script>
+<script>
+
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+const nodeCount = 5;
+const iterationCount = 5;
+
+async function runAll() {
+    if (window.testRunner)
+        testRunner.waitUntilDone();
+
+    for (let j = 0; j < iterationCount; ++j) {
+        runTest();
+        gc();
+        await Promise.resolve();
+    }
+
+    if (window.testRunner)
+        testRunner.notifyDone();
+}
+
+async function runTest() {
+    let container = document.createElement('div');
+    container.myState = 'alive';
+    observer.observe(container, {subtree: true, childList: true, attributes: true});
+
+    for (let i = 0; i < nodeCount; ++i) {
+        const child = document.createElement('span');
+        child.myState = 'alive';
+        container.appendChild(child);
+    }
+    container.textContent = '';
+}
+
+const observer = new MutationObserver((recordList) => {
+    let deadCount = 0;
+    for (const record of recordList) {
+        for (const node of record.addedNodes) {
+            if (node.myState != 'alive')
+                deadCount++;
+        }
+        for (const node of record.removedNodes) {
+            if (node.myState != 'alive')
+                deadCount++;
+        }
+    }
+    document.getElementById('log').textContent += (deadCount ? `FAIL - ${deadCount} nodes lost JS wrappers` : 'PASS') + '\n';
+});
+
+runAll();
+
+</script>
+</body>
+</html>
diff --git a/LayoutTests/fast/dom/MutationObserver/mutation-record-keeps-js-wrappers-of-nodes-alive-expected.txt b/LayoutTests/fast/dom/MutationObserver/mutation-record-keeps-js-wrappers-of-nodes-alive-expected.txt
new file mode 100644 (file)
index 0000000..c408c90
--- /dev/null
@@ -0,0 +1,8 @@
+This tests that JS wrappers of nodes enqueued to a mutation observer do not get collected.
+
+PASS
+PASS
+PASS
+PASS
+PASS
+
diff --git a/LayoutTests/fast/dom/MutationObserver/mutation-record-keeps-js-wrappers-of-nodes-alive.html b/LayoutTests/fast/dom/MutationObserver/mutation-record-keeps-js-wrappers-of-nodes-alive.html
new file mode 100644 (file)
index 0000000..6e4ede5
--- /dev/null
@@ -0,0 +1,72 @@
+<!DOCTYPE html>
+<html>
+<body>
+<p>This tests that JS wrappers of nodes enqueued to a mutation observer do not get collected.</p>
+<pre id="log"></pre>
+<script src="../../../resources/gc.js"></script>
+<script>
+
+if (window.testRunner)
+    testRunner.dumpAsText();
+
+const nodeCount = 5;
+const iterationCount = 5;
+
+let elementsMap;
+
+async function observe() {
+    let container = document.createElement('div');
+    container.myState = 'live';
+
+    const observer = new MutationObserver(() => { });
+    observer.observe(container, {subtree: true, childList: true, attributes: true});
+    for (let i = 0; i < nodeCount; ++i) {
+        const child = document.createElement('span');
+        child.myState = 'live';
+        container.appendChild(child);
+    }
+    container.setAttribute('title', 'foo');
+    container.setAttribute('title', 'bar');
+    container.textContent = '';
+
+    return observer.takeRecords();
+}
+
+function check(recordList) {
+    let deadCount = 0;
+    for (const record of recordList) {
+        if (record.target.myState != 'live')
+            deadCount++;
+        for (const node of record.addedNodes) {
+            if (node.myState != 'live')
+                deadCount++;
+        }
+        for (const node of record.removedNodes) {
+            if (node.myState != 'live')
+                deadCount++;
+        }
+    }
+    document.getElementById('log').textContent += (deadCount ? `FAIL - ${deadCount} nodes lost JS wrappers` : 'PASS') + '\n';
+}
+
+async function runAll() {
+    if (window.testRunner)
+        testRunner.waitUntilDone();
+
+    for (let j = 0; j < iterationCount; ++j) {
+        const records = await observe();
+        await new Promise((resolve) => setTimeout(resolve, 0));
+        gc();
+        check(records);
+    }
+
+
+    if (window.testRunner)
+        testRunner.notifyDone();
+}
+
+runAll();
+
+</script>
+</body>
+</html>
index 1fbc699..abc78dd 100644 (file)
@@ -1,3 +1,31 @@
+2018-10-03  Ryosuke Niwa  <rniwa@webkit.org>
+
+        MutationRecord doesn't keep JS wrappers of target, addedNodes, and removedNodes alive
+        https://bugs.webkit.org/show_bug.cgi?id=190277
+
+        Reviewed by Antti Koivisto.
+
+        The bug was caused by JSMutationRecord not visiting any of the nodes referenced by mutation records.
+
+        Fixed the bug by adding JSMutationRecord::visitAdditionalChildren, which adds the root nodes of
+        the root nodes of the target, addedNodes, and removedNodes in each mutation record.
+
+        Test: fast/dom/MutationObserver/mutation-record-keeps-js-wrappers-of-nodes-alive.html
+
+        * Sources.txt:
+        * WebCore.xcodeproj/project.pbxproj:
+        * bindings/js/JSMutationRecordCustom.cpp: Added.
+        (WebCore::JSMutationRecord::visitAdditionalChildren): Added.
+        * bindings/js/JSPerformanceObserverCustom.cpp: This file got dumped out of a unified build file
+        where using namespace JSC was defined. Use the fully qualified names to refer to JSC types.
+        (WebCore::JSPerformanceObserverOwner::isReachableFromOpaqueRoots):
+        * dom/MutationRecord.cpp:
+        (WebCore::ChildListRecord::visitNodesConcurrently): Added.
+        (WebCore::RecordWithEmptyNodeLists::visitNodesConcurrently): Added.
+        (WebCore::MutationRecordWithNullOldValue::visitNodesConcurrently): Added.
+        * dom/MutationRecord.h:
+        * dom/MutationRecord.idl:
+
 2018-10-04  Jiewen Tan  <jiewen_tan@apple.com>
 
         [WebAuthN] Move time out control from WebProcess to UIProcess
index 339fd0d..2159b5e 100644 (file)
@@ -423,6 +423,7 @@ bindings/js/JSMessageChannelCustom.cpp
 bindings/js/JSMessageEventCustom.cpp
 bindings/js/JSMessagePortCustom.cpp
 bindings/js/JSMutationObserverCustom.cpp
+bindings/js/JSMutationRecordCustom.cpp
 bindings/js/JSNavigatorCustom.cpp
 bindings/js/JSNodeCustom.cpp
 bindings/js/JSNodeIteratorCustom.cpp
index d2171cc..a5f326e 100644 (file)
                9B714E1F1C91166900AC0E92 /* EventPath.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = EventPath.h; sourceTree = "<group>"; };
                9B85530520E733B5009EEF4F /* EventTargetFactory.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = EventTargetFactory.cpp; sourceTree = "<group>"; };
                9B9299B01F6796A4006723C2 /* WebContentReaderCocoa.mm */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.objcpp; path = WebContentReaderCocoa.mm; sourceTree = "<group>"; };
+               9B9CADA72165DC7600E8D858 /* JSMutationRecordCustom.cpp */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.cpp.cpp; path = JSMutationRecordCustom.cpp; 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>"; };
                                E1A5F99A0E7EAA2500AF85EA /* JSMessageChannelCustom.cpp */,
                                E1ADED460E76B8DD004A1A5E /* JSMessagePortCustom.cpp */,
                                C6F0917E143A2BB900685849 /* JSMutationObserverCustom.cpp */,
+                               9B9CADA72165DC7600E8D858 /* JSMutationRecordCustom.cpp */,
                                83B5DA451FA9079300B59DF4 /* JSNavigatorCustom.cpp */,
                                BCD9C2600C17AA67005C90A2 /* JSNodeCustom.cpp */,
                                BC9439C2116CF4940048C750 /* JSNodeCustom.h */,
diff --git a/Source/WebCore/bindings/js/JSMutationRecordCustom.cpp b/Source/WebCore/bindings/js/JSMutationRecordCustom.cpp
new file mode 100644 (file)
index 0000000..f23dc29
--- /dev/null
@@ -0,0 +1,38 @@
+/*
+ * 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 "JSMutationRecord.h"
+
+#include "MutationRecord.h"
+
+namespace WebCore {
+
+void JSMutationRecord::visitAdditionalChildren(JSC::SlotVisitor& visitor)
+{
+    wrapped().visitNodesConcurrently(visitor);
+}
+    
+} // namespace WebCore
index 6efa3dc..5c1b6da 100644 (file)
@@ -35,12 +35,12 @@ void JSPerformanceObserver::visitAdditionalChildren(JSC::SlotVisitor& visitor)
     wrapped().callback().visitJSFunction(visitor);
 }
 
-bool JSPerformanceObserverOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, SlotVisitor&, const char** reason)
+bool JSPerformanceObserverOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, JSC::SlotVisitor&, const char** reason)
 {
     if (UNLIKELY(reason))
         *reason = "Registered PerformanceObserver callback";
 
-    return jsCast<JSPerformanceObserver*>(handle.slot()->asCell())->wrapped().isRegistered();
+    return JSC::jsCast<JSPerformanceObserver*>(handle.slot()->asCell())->wrapped().isRegistered();
 }
 
 }
index 1c7166d..257b5ea 100644 (file)
@@ -32,6 +32,7 @@
 #include "MutationRecord.h"
 
 #include "CharacterData.h"
+#include "JSNode.h"
 #include "StaticNodeList.h"
 #include <wtf/NeverDestroyed.h>
 #include <wtf/StdLibExtras.h>
@@ -40,7 +41,15 @@ namespace WebCore {
 
 namespace {
 
-class ChildListRecord : public MutationRecord {
+static void visitNodeList(JSC::SlotVisitor& visitor, NodeList& nodeList)
+{
+    ASSERT(!nodeList.isLiveNodeList());
+    unsigned length = nodeList.length();
+    for (unsigned i = 0; i < length; ++i)
+        visitor.addOpaqueRoot(root(nodeList.item(i)));
+}
+
+class ChildListRecord final : public MutationRecord {
 public:
     ChildListRecord(ContainerNode& target, Ref<NodeList>&& added, Ref<NodeList>&& removed, RefPtr<Node>&& previousSibling, RefPtr<Node>&& nextSibling)
         : m_target(target)
@@ -59,6 +68,15 @@ private:
     Node* previousSibling() override { return m_previousSibling.get(); }
     Node* nextSibling() override { return m_nextSibling.get(); }
 
+    void visitNodesConcurrently(JSC::SlotVisitor& visitor) const final
+    {
+        visitor.addOpaqueRoot(root(m_target.ptr()));
+        if (m_addedNodes)
+            visitNodeList(visitor, *m_addedNodes);
+        if (m_removedNodes)
+            visitNodeList(visitor, *m_removedNodes);
+    }
+    
     Ref<ContainerNode> m_target;
     RefPtr<NodeList> m_addedNodes;
     RefPtr<NodeList> m_removedNodes;
@@ -87,13 +105,18 @@ private:
         return nodeList.get();
     }
 
+    void visitNodesConcurrently(JSC::SlotVisitor& visitor) const final
+    {
+        visitor.addOpaqueRoot(root(m_target.ptr()));
+    }
+
     Ref<Node> m_target;
     String m_oldValue;
     RefPtr<NodeList> m_addedNodes;
     RefPtr<NodeList> m_removedNodes;
 };
 
-class AttributesRecord : public RecordWithEmptyNodeLists {
+class AttributesRecord final : public RecordWithEmptyNodeLists {
 public:
     AttributesRecord(Element& target, const QualifiedName& name, const AtomicString& oldValue)
         : RecordWithEmptyNodeLists(target, oldValue)
@@ -122,7 +145,7 @@ private:
     const AtomicString& type() override;
 };
 
-class MutationRecordWithNullOldValue : public MutationRecord {
+class MutationRecordWithNullOldValue final : public MutationRecord {
 public:
     MutationRecordWithNullOldValue(MutationRecord& record)
         : m_record(record)
@@ -141,6 +164,11 @@ private:
 
     String oldValue() override { return String(); }
 
+    void visitNodesConcurrently(JSC::SlotVisitor& visitor) const final
+    {
+        m_record->visitNodesConcurrently(visitor);
+    }
+
     Ref<MutationRecord> m_record;
 };
 
index ad848e7..8872b44 100644 (file)
 #include <wtf/RefPtr.h>
 #include <wtf/text/WTFString.h>
 
+namespace JSC {
+
+class SlotVisitor;
+
+}
+
 namespace WebCore {
 
 class CharacterData;
@@ -65,6 +71,8 @@ public:
     virtual const AtomicString& attributeNamespace() { return nullAtom(); }
 
     virtual String oldValue() { return String(); }
+
+    virtual void visitNodesConcurrently(JSC::SlotVisitor&) const = 0;
 };
 
 } // namespace WebCore
index 292964c..a08e661 100644 (file)
@@ -29,7 +29,8 @@
  */
 
 [    
-    SkipVTableValidation
+    SkipVTableValidation,
+    JSCustomMarkFunction
 ] interface MutationRecord {
     readonly attribute DOMString type;