Title: [236799] trunk
Revision
236799
Author
jlew...@apple.com
Date
2018-10-03 11:02:45 -0700 (Wed, 03 Oct 2018)

Log Message

Unreviewed, rolling out r236781.

The test added with this commit is timing out consistently.

Reverted changeset:

"GC can collect JS wrappers of nodes in the mutation records
waiting to be delivered"
https://bugs.webkit.org/show_bug.cgi?id=190115
https://trac.webkit.org/changeset/236781

Modified Paths

Removed Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (236798 => 236799)


--- trunk/LayoutTests/ChangeLog	2018-10-03 17:56:29 UTC (rev 236798)
+++ trunk/LayoutTests/ChangeLog	2018-10-03 18:02:45 UTC (rev 236799)
@@ -1,3 +1,16 @@
+2018-10-03  Matt Lewis  <jlew...@apple.com>
+
+        Unreviewed, rolling out r236781.
+
+        The test added with this commit is timing out consistently.
+
+        Reverted changeset:
+
+        "GC can collect JS wrappers of nodes in the mutation records
+        waiting to be delivered"
+        https://bugs.webkit.org/show_bug.cgi?id=190115
+        https://trac.webkit.org/changeset/236781
+
 2018-10-03  Youenn Fablet  <you...@apple.com>
 
         Enable H264 simulcast

Deleted: trunk/LayoutTests/fast/dom/MutationObserver/mutation-observer-retains-js-wrappers-of-targets-alive-expected.txt (236798 => 236799)


--- trunk/LayoutTests/fast/dom/MutationObserver/mutation-observer-retains-js-wrappers-of-targets-alive-expected.txt	2018-10-03 17:56:29 UTC (rev 236798)
+++ trunk/LayoutTests/fast/dom/MutationObserver/mutation-observer-retains-js-wrappers-of-targets-alive-expected.txt	2018-10-03 18:02:45 UTC (rev 236799)
@@ -1,23 +0,0 @@
-This tests that JS wrappers of nodes in a tree mutation record do not get collected.
-
-PASS
-PASS
-PASS
-PASS
-PASS
-PASS
-PASS
-PASS
-PASS
-PASS
-PASS
-PASS
-PASS
-PASS
-PASS
-PASS
-PASS
-PASS
-PASS
-PASS
-

Deleted: trunk/LayoutTests/fast/dom/MutationObserver/mutation-observer-retains-js-wrappers-of-targets-alive.html (236798 => 236799)


--- trunk/LayoutTests/fast/dom/MutationObserver/mutation-observer-retains-js-wrappers-of-targets-alive.html	2018-10-03 17:56:29 UTC (rev 236798)
+++ trunk/LayoutTests/fast/dom/MutationObserver/mutation-observer-retains-js-wrappers-of-targets-alive.html	2018-10-03 18:02:45 UTC (rev 236799)
@@ -1,83 +0,0 @@
-<!DOCTYPE html>
-<html>
-<body>
-<p>This tests that JS wrappers of nodes in a tree mutation record do not get collected.</p>
-<pre id="log"></pre>
-<script>
-
-if (window.testRunner)
-    testRunner.dumpAsText();
-
-const nodeCount = 20;
-const iterationCount = window.GCController ? 20 : 50;
-const observer = new MutationObserver((recordList) => {
-    let deadCount = 0;
-    for (const record of recordList) {
-        for (const node of record.addedNodes) {
-            if (node.myState != 'alive')
-                deadCount++;
-        }
-        triggerGC();
-        for (const node of record.removedNodes) {
-            if (node.myState != 'alive')
-                deadCount++;
-        }
-    }
-    document.getElementById('log').textContent += (deadCount ? `FAIL - ${deadCount} nodes lost JS wrappers` : 'PASS') + '\n';
-});
-
-function runTest() {
-    let container = document.createElement('div');
-    container.innerHTML = '<div></div>';
-    container.firstChild.myState = 'alive';
-    observer.observe(container, {subtree: true, childList: true});
-
-    // Test transient registrations.
-    for (let i = 0; i < nodeCount; ++i) {
-        const child = document.createElement('span');
-        child.myState = 'alive';
-        child.appendChild(document.createElement('span')).myState = 'alive';
-        container.firstChild.appendChild(child);
-        child.textContent = '';
-    }
-    container.textContent = '';
-
-    // Test pending targets.
-    for (let i = 0; i < nodeCount; ++i)
-        container.appendChild(document.createElement('span')).myState = 'alive';
-}
-
-function triggerGC() {
-    if (window.GCController) {
-        GCController.collect();
-        return;
-    } 
-
-    const elements = new Array(nodeCount)
-    for (let i = 0; i < nodeCount; ++i)
-        elements[i] = document.createElement('span');
-
-    const x = new Array(1000);
-    for (let i = 0; i < 1000; ++i)
-        x[i] = new Array(100);
-}
-
-async function runAll() {
-    if (window.testRunner)
-        testRunner.waitUntilDone();
-
-    for (let j = 0; j < iterationCount; ++j) {
-        runTest();
-        triggerGC();
-        await Promise.resolve();
-    }
-
-    if (window.testRunner)
-        testRunner.notifyDone();
-}
-
-runAll();
-
-</script>
-</body>
-</html>

Modified: trunk/Source/WebCore/ChangeLog (236798 => 236799)


--- trunk/Source/WebCore/ChangeLog	2018-10-03 17:56:29 UTC (rev 236798)
+++ trunk/Source/WebCore/ChangeLog	2018-10-03 18:02:45 UTC (rev 236799)
@@ -1,3 +1,16 @@
+2018-10-03  Matt Lewis  <jlew...@apple.com>
+
+        Unreviewed, rolling out r236781.
+
+        The test added with this commit is timing out consistently.
+
+        Reverted changeset:
+
+        "GC can collect JS wrappers of nodes in the mutation records
+        waiting to be delivered"
+        https://bugs.webkit.org/show_bug.cgi?id=190115
+        https://trac.webkit.org/changeset/236781
+
 2018-10-03  Dean Jackson  <d...@apple.com>
 
         [macOS] Switching to discrete GPU should be done in the UI process

Modified: trunk/Source/WebCore/dom/GCReachableRef.h (236798 => 236799)


--- trunk/Source/WebCore/dom/GCReachableRef.h	2018-10-03 17:56:29 UTC (rev 236798)
+++ trunk/Source/WebCore/dom/GCReachableRef.h	2018-10-03 18:02:45 UTC (rev 236799)
@@ -27,7 +27,7 @@
 
 #include <wtf/DumbPtrTraits.h>
 #include <wtf/HashCountedSet.h>
-#include <wtf/RefPtr.h>
+#include <wtf/Ref.h>
 
 namespace WebCore {
 
@@ -69,83 +69,12 @@
     T* operator->() const { return &get(); }
     T* ptr() const RETURNS_NONNULL { return &get(); }
     T& get() const { ASSERT(m_ptr); return *m_ptr; }
-    operator T&() const { return get(); }
+    operator T&() const { ASSERT(m_ptr); return *m_ptr; }
     bool operator!() const { return !get(); }
 
-    // Hash table deleted values, which are only constructed and never copied or destroyed.
-    GCReachableRef(WTF::HashTableDeletedValueType)
-        : m_ptr(RefPtr<T>::hashTableDeletedValue())
-    { }
-    bool isHashTableDeletedValue() const { return m_ptr.isHashTableDeletedValue(); }
+private:
 
-    GCReachableRef(WTF::HashTableEmptyValueType)
-        : m_ptr(nullptr)
-    { }
-    bool isHashTableEmptyValue() const { return !m_ptr; }
-
-    const T* ptrAllowingHashTableEmptyValue() const { ASSERT(m_ptr || isHashTableEmptyValue()); return m_ptr.get(); }
-    T* ptrAllowingHashTableEmptyValue() { ASSERT(m_ptr || isHashTableEmptyValue()); return m_ptr.get(); }
-
-    void assignToHashTableEmptyValue(GCReachableRef&& reference)
-    {
-        ASSERT(!m_ptr);
-        m_ptr = WTFMove(reference.m_ptr);
-        ASSERT(m_ptr);
-    }
-
-private:
     RefPtr<T> m_ptr;
 };
 
-} // namespace WebCore
-
-namespace WTF {
-
-template<typename P> struct HashTraits<WebCore::GCReachableRef<P>> : SimpleClassHashTraits<WebCore::GCReachableRef<P>> {
-    static const bool emptyValueIsZero = true;
-    static WebCore::GCReachableRef<P> emptyValue() { return HashTableEmptyValue; }
-
-    template <typename>
-    static void constructEmptyValue(WebCore::GCReachableRef<P>& slot)
-    {
-        new (NotNull, std::addressof(slot)) WebCore::GCReachableRef<P>(HashTableEmptyValue);
-    }
-
-    static const bool hasIsEmptyValueFunction = true;
-    static bool isEmptyValue(const WebCore::GCReachableRef<P>& value) { return value.isHashTableEmptyValue(); }
-
-    static void assignToEmpty(WebCore::GCReachableRef<P>& emptyValue, WebCore::GCReachableRef<P>&& newValue)
-    {
-        ASSERT(isEmptyValue(emptyValue));
-        emptyValue.assignToHashTableEmptyValue(WTFMove(newValue));
-    }
-
-    typedef P* PeekType;
-    static PeekType peek(const Ref<P>& value) { return const_cast<PeekType>(value.ptrAllowingHashTableEmptyValue()); }
-    static PeekType peek(P* value) { return value; }
-
-    typedef std::optional<Ref<P>> TakeType;
-    static TakeType take(Ref<P>&& value) { return isEmptyValue(value) ? std::nullopt : std::optional<Ref<P>>(WTFMove(value)); }
-};
-
-template <typename T, typename U>
-struct GetPtrHelper<WebCore::GCReachableRef<T, U>> {
-    typedef T* PtrType;
-    static T* getPtr(const WebCore::GCReachableRef<T, U>& reference) { return const_cast<T*>(reference.ptr()); }
-};
-
-template <typename T, typename U>
-struct IsSmartPtr<WebCore::GCReachableRef<T, U>> {
-    static const bool value = true;
-};
-
-template<typename P> struct PtrHash<WebCore::GCReachableRef<P>> : PtrHashBase<WebCore::GCReachableRef<P>, IsSmartPtr<WebCore::GCReachableRef<P>>::value> {
-    static const bool safeToCompareToEmptyOrDeleted = false;
-};
-
-template<typename P> struct DefaultHash<WebCore::GCReachableRef<P>> {
-    typedef PtrHash<WebCore::GCReachableRef<P>> Hash;
-};
-
-} // namespace WTF
-
+}

Modified: trunk/Source/WebCore/dom/MutationObserver.cpp (236798 => 236799)


--- trunk/Source/WebCore/dom/MutationObserver.cpp	2018-10-03 17:56:29 UTC (rev 236798)
+++ trunk/Source/WebCore/dom/MutationObserver.cpp	2018-10-03 18:02:45 UTC (rev 236799)
@@ -114,14 +114,11 @@
 {
     Vector<Ref<MutationRecord>> records;
     records.swap(m_records);
-    // Don't clear m_pendingTargets here because we can collect JS wrappers
-    // between the time takeRecords is called and nodes in records are accesssed.
     return records;
 }
 
 void MutationObserver::disconnect()
 {
-    m_pendingTargets.clear();
     m_records.clear();
     HashSet<MutationObserverRegistration*> registrations(m_registrations);
     for (auto* registration : registrations)
@@ -184,8 +181,6 @@
 void MutationObserver::enqueueMutationRecord(Ref<MutationRecord>&& mutation)
 {
     ASSERT(isMainThread());
-    ASSERT(mutation->target());
-    m_pendingTargets.add(*mutation->target());
     m_records.append(WTFMove(mutation));
     activeMutationObservers().add(this);
 
@@ -226,18 +221,15 @@
 {
     ASSERT(canDeliver());
 
-    // Calling takeTransientRegistrations() can modify m_registrations, so it's necessary
+    // Calling clearTransientRegistrations() can modify m_registrations, so it's necessary
     // to make a copy of the transient registrations before operating on them.
     Vector<MutationObserverRegistration*, 1> transientRegistrations;
-    Vector<std::unique_ptr<HashSet<GCReachableRef<Node>>>, 1> nodesToKeepAlive;
-    HashSet<GCReachableRef<Node>> pendingTargets;
-    pendingTargets.swap(m_pendingTargets);
     for (auto* registration : m_registrations) {
         if (registration->hasTransientRegistrations())
             transientRegistrations.append(registration);
     }
     for (auto& registration : transientRegistrations)
-        nodesToKeepAlive.append(registration->takeTransientRegistrations());
+        registration->clearTransientRegistrations();
 
     if (m_records.isEmpty())
         return;

Modified: trunk/Source/WebCore/dom/MutationObserver.h (236798 => 236799)


--- trunk/Source/WebCore/dom/MutationObserver.h	2018-10-03 17:56:29 UTC (rev 236798)
+++ trunk/Source/WebCore/dom/MutationObserver.h	2018-10-03 18:02:45 UTC (rev 236799)
@@ -32,7 +32,6 @@
 #pragma once
 
 #include "ExceptionOr.h"
-#include "GCReachableRef.h"
 #include <wtf/Forward.h>
 #include <wtf/HashSet.h>
 #include <wtf/IsoMalloc.h>
@@ -110,7 +109,6 @@
 
     Ref<MutationCallback> m_callback;
     Vector<Ref<MutationRecord>> m_records;
-    HashSet<GCReachableRef<Node>> m_pendingTargets;
     HashSet<MutationObserverRegistration*> m_registrations;
     unsigned m_priority;
 };

Modified: trunk/Source/WebCore/dom/MutationObserverRegistration.cpp (236798 => 236799)


--- trunk/Source/WebCore/dom/MutationObserverRegistration.cpp	2018-10-03 17:56:29 UTC (rev 236798)
+++ trunk/Source/WebCore/dom/MutationObserverRegistration.cpp	2018-10-03 18:02:45 UTC (rev 236799)
@@ -48,13 +48,13 @@
 
 MutationObserverRegistration::~MutationObserverRegistration()
 {
-    takeTransientRegistrations();
+    clearTransientRegistrations();
     m_observer->observationEnded(*this);
 }
 
 void MutationObserverRegistration::resetObservation(MutationObserverOptions options, const HashSet<AtomicString>& attributeFilter)
 {
-    takeTransientRegistrations();
+    clearTransientRegistrations();
     m_options = options;
     m_attributeFilter = attributeFilter;
 }
@@ -68,30 +68,28 @@
     m_observer->setHasTransientRegistration();
 
     if (!m_transientRegistrationNodes) {
-        m_transientRegistrationNodes = std::make_unique<HashSet<GCReachableRef<Node>>>();
+        m_transientRegistrationNodes = std::make_unique<HashSet<RefPtr<Node>>>();
 
         ASSERT(!m_nodeKeptAlive);
-        m_nodeKeptAlive = &m_node; // Balanced in takeTransientRegistrations.
+        m_nodeKeptAlive = &m_node; // Balanced in clearTransientRegistrations.
     }
-    m_transientRegistrationNodes->add(node);
+    m_transientRegistrationNodes->add(&node);
 }
 
-std::unique_ptr<HashSet<GCReachableRef<Node>>> MutationObserverRegistration::takeTransientRegistrations()
+void MutationObserverRegistration::clearTransientRegistrations()
 {
     if (!m_transientRegistrationNodes) {
         ASSERT(!m_nodeKeptAlive);
-        return nullptr;
+        return;
     }
 
     for (auto& node : *m_transientRegistrationNodes)
         node->unregisterTransientMutationObserver(*this);
 
-    auto returnValue = WTFMove(m_transientRegistrationNodes);
+    m_transientRegistrationNodes = nullptr;
 
     ASSERT(m_nodeKeptAlive);
     m_nodeKeptAlive = nullptr; // Balanced in observeSubtreeNodeWillDetach.
-
-    return returnValue;
 }
 
 bool MutationObserverRegistration::shouldReceiveMutationFrom(Node& node, MutationObserver::MutationType type, const QualifiedName* attributeName) const
@@ -118,7 +116,7 @@
     if (!m_transientRegistrationNodes)
         return;
     for (auto& node : *m_transientRegistrationNodes)
-        nodes.add(node.ptr());
+        nodes.add(node.get());
 }
 
 } // namespace WebCore

Modified: trunk/Source/WebCore/dom/MutationObserverRegistration.h (236798 => 236799)


--- trunk/Source/WebCore/dom/MutationObserverRegistration.h	2018-10-03 17:56:29 UTC (rev 236798)
+++ trunk/Source/WebCore/dom/MutationObserverRegistration.h	2018-10-03 18:02:45 UTC (rev 236799)
@@ -30,7 +30,6 @@
 
 #pragma once
 
-#include "GCReachableRef.h"
 #include "MutationObserver.h"
 #include <wtf/HashSet.h>
 #include <wtf/text/AtomicString.h>
@@ -48,7 +47,7 @@
 
     void resetObservation(MutationObserverOptions, const HashSet<AtomicString>& attributeFilter);
     void observedSubtreeNodeWillDetach(Node&);
-    std::unique_ptr<HashSet<GCReachableRef<Node>>> takeTransientRegistrations();
+    void clearTransientRegistrations();
     bool hasTransientRegistrations() const { return m_transientRegistrationNodes && !m_transientRegistrationNodes->isEmpty(); }
 
     bool shouldReceiveMutationFrom(Node&, MutationObserver::MutationType, const QualifiedName* attributeName) const;
@@ -65,7 +64,7 @@
     Ref<MutationObserver> m_observer;
     Node& m_node;
     RefPtr<Node> m_nodeKeptAlive;
-    std::unique_ptr<HashSet<GCReachableRef<Node>>> m_transientRegistrationNodes;
+    std::unique_ptr<HashSet<RefPtr<Node>>> m_transientRegistrationNodes;
     MutationObserverOptions m_options;
     HashSet<AtomicString> m_attributeFilter;
 };
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to