GC can collect JS wrappers of nodes in the mutation records waiting to be delivered
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 3 Oct 2018 18:16:39 +0000 (18:16 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 3 Oct 2018 18:16:39 +0000 (18:16 +0000)
https://bugs.webkit.org/show_bug.cgi?id=190115

Reviewed by Geoffrey Garen.

Fixed the bug by retaining JS wrappers of elements in mutation records using GCReachableRef.

This patch deploys GCReachableRef in two places: MutationObserver where each mutation record's
target is kept alive and MutationObserverRegistration where each node which had been removed
from an observed tree is kept alive for a subtree observation.

No new test since the test which can reproduce this problem is too slow.

* dom/GCReachableRef.h:
(WebCore::GCReachableRef): Made it work with hash table.
(WebCore::GCReachableRef::operator T& const):
(WebCore::GCReachableRef::GCReachableRef):
(WebCore::GCReachableRef::isHashTableDeletedValue const):
(WebCore::GCReachableRef::isHashTableEmptyValue const):
(WebCore::GCReachableRef::ptrAllowingHashTableEmptyValue const):
(WebCore::GCReachableRef::ptrAllowingHashTableEmptyValue):
(WebCore::GCReachableRef::assignToHashTableEmptyValue):
(WTF::HashTraits<WebCore::GCReachableRef<P>>::emptyValue):
(WTF::HashTraits<WebCore::GCReachableRef<P>>::constructEmptyValue):
(WTF::HashTraits<WebCore::GCReachableRef<P>>::isEmptyValue):
(WTF::HashTraits<WebCore::GCReachableRef<P>>::assignToEmpty):
(WTF::HashTraits<WebCore::GCReachableRef<P>>::peek):
(WTF::HashTraits<WebCore::GCReachableRef<P>>::take):
* dom/MutationObserver.cpp:
(WebCore::MutationObserver::takeRecords): Don't clear m_pendingTargets because that would allow wrappers
to be collected before elements in mutation records are accessed. We delay until the end of the current
microtask at which point deliver() function is called.
(WebCore::MutationObserver::disconnect):
(WebCore::MutationObserver::enqueueMutationRecord): Add the target to the list of elements to keep alive.
This is needed for a newly inserted node, a node with attribute change, etc...
(WebCore::MutationObserver::deliver): Keep the set of transient registration targets alive until mutation
records are delivered to each observer. These are nodes which had been removed from a tree and whose
subtree had still been obsreved up until this point.
* dom/MutationObserver.h:
* dom/MutationObserverRegistration.cpp:
(WebCore::MutationObserverRegistration::observedSubtreeNodeWillDetach):
(WebCore::MutationObserverRegistration::takeTransientRegistrations): Return the hash set of elemenets
that need to be kept alive so that MutationObserver::deliver can keep them alive until the deliver
function had been called.
(WebCore::MutationObserverRegistration::addRegistrationNodesToSet const):
* dom/MutationObserverRegistration.h:

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

Source/WebCore/ChangeLog
Source/WebCore/dom/GCReachableRef.h
Source/WebCore/dom/MutationObserver.cpp
Source/WebCore/dom/MutationObserver.h
Source/WebCore/dom/MutationObserverRegistration.cpp
Source/WebCore/dom/MutationObserverRegistration.h

index 4322981..45b99b7 100644 (file)
@@ -1,3 +1,52 @@
+2018-10-03  Ryosuke Niwa  <rniwa@webkit.org>
+
+        GC can collect JS wrappers of nodes in the mutation records waiting to be delivered
+        https://bugs.webkit.org/show_bug.cgi?id=190115
+
+        Reviewed by Geoffrey Garen.
+
+        Fixed the bug by retaining JS wrappers of elements in mutation records using GCReachableRef.
+
+        This patch deploys GCReachableRef in two places: MutationObserver where each mutation record's
+        target is kept alive and MutationObserverRegistration where each node which had been removed
+        from an observed tree is kept alive for a subtree observation.
+
+        No new test since the test which can reproduce this problem is too slow.
+
+        * dom/GCReachableRef.h:
+        (WebCore::GCReachableRef): Made it work with hash table.
+        (WebCore::GCReachableRef::operator T& const):
+        (WebCore::GCReachableRef::GCReachableRef):
+        (WebCore::GCReachableRef::isHashTableDeletedValue const):
+        (WebCore::GCReachableRef::isHashTableEmptyValue const):
+        (WebCore::GCReachableRef::ptrAllowingHashTableEmptyValue const):
+        (WebCore::GCReachableRef::ptrAllowingHashTableEmptyValue):
+        (WebCore::GCReachableRef::assignToHashTableEmptyValue):
+        (WTF::HashTraits<WebCore::GCReachableRef<P>>::emptyValue):
+        (WTF::HashTraits<WebCore::GCReachableRef<P>>::constructEmptyValue):
+        (WTF::HashTraits<WebCore::GCReachableRef<P>>::isEmptyValue):
+        (WTF::HashTraits<WebCore::GCReachableRef<P>>::assignToEmpty):
+        (WTF::HashTraits<WebCore::GCReachableRef<P>>::peek):
+        (WTF::HashTraits<WebCore::GCReachableRef<P>>::take):
+        * dom/MutationObserver.cpp:
+        (WebCore::MutationObserver::takeRecords): Don't clear m_pendingTargets because that would allow wrappers
+        to be collected before elements in mutation records are accessed. We delay until the end of the current
+        microtask at which point deliver() function is called.
+        (WebCore::MutationObserver::disconnect):
+        (WebCore::MutationObserver::enqueueMutationRecord): Add the target to the list of elements to keep alive.
+        This is needed for a newly inserted node, a node with attribute change, etc...
+        (WebCore::MutationObserver::deliver): Keep the set of transient registration targets alive until mutation
+        records are delivered to each observer. These are nodes which had been removed from a tree and whose
+        subtree had still been obsreved up until this point.
+        * dom/MutationObserver.h:
+        * dom/MutationObserverRegistration.cpp:
+        (WebCore::MutationObserverRegistration::observedSubtreeNodeWillDetach):
+        (WebCore::MutationObserverRegistration::takeTransientRegistrations): Return the hash set of elemenets
+        that need to be kept alive so that MutationObserver::deliver can keep them alive until the deliver
+        function had been called.
+        (WebCore::MutationObserverRegistration::addRegistrationNodesToSet const):
+        * dom/MutationObserverRegistration.h:
+
 2018-10-03  Dean Jackson  <dino@apple.com>
 
         Make the Pointer Events feature description valid
 2018-10-03  Dean Jackson  <dino@apple.com>
 
         Make the Pointer Events feature description valid
index c5267d1..e66c975 100644 (file)
@@ -27,7 +27,7 @@
 
 #include <wtf/DumbPtrTraits.h>
 #include <wtf/HashCountedSet.h>
 
 #include <wtf/DumbPtrTraits.h>
 #include <wtf/HashCountedSet.h>
-#include <wtf/Ref.h>
+#include <wtf/RefPtr.h>
 
 namespace WebCore {
 
 
 namespace WebCore {
 
@@ -69,12 +69,83 @@ public:
     T* operator->() const { return &get(); }
     T* ptr() const RETURNS_NONNULL { return &get(); }
     T& get() const { ASSERT(m_ptr); return *m_ptr; }
     T* operator->() const { return &get(); }
     T* ptr() const RETURNS_NONNULL { return &get(); }
     T& get() const { ASSERT(m_ptr); return *m_ptr; }
-    operator T&() const { ASSERT(m_ptr); return *m_ptr; }
+    operator T&() const { return get(); }
     bool operator!() const { return !get(); }
 
     bool operator!() const { return !get(); }
 
-private:
+    // 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(); }
+
+    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;
 };
 
     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
+
index 1cbb09b..e33aad3 100644 (file)
@@ -114,11 +114,14 @@ Vector<Ref<MutationRecord>> MutationObserver::takeRecords()
 {
     Vector<Ref<MutationRecord>> records;
     records.swap(m_records);
 {
     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()
 {
     return records;
 }
 
 void MutationObserver::disconnect()
 {
+    m_pendingTargets.clear();
     m_records.clear();
     HashSet<MutationObserverRegistration*> registrations(m_registrations);
     for (auto* registration : registrations)
     m_records.clear();
     HashSet<MutationObserverRegistration*> registrations(m_registrations);
     for (auto* registration : registrations)
@@ -181,6 +184,8 @@ static void queueMutationObserverCompoundMicrotask()
 void MutationObserver::enqueueMutationRecord(Ref<MutationRecord>&& mutation)
 {
     ASSERT(isMainThread());
 void MutationObserver::enqueueMutationRecord(Ref<MutationRecord>&& mutation)
 {
     ASSERT(isMainThread());
+    ASSERT(mutation->target());
+    m_pendingTargets.add(*mutation->target());
     m_records.append(WTFMove(mutation));
     activeMutationObservers().add(this);
 
     m_records.append(WTFMove(mutation));
     activeMutationObservers().add(this);
 
@@ -221,15 +226,18 @@ void MutationObserver::deliver()
 {
     ASSERT(canDeliver());
 
 {
     ASSERT(canDeliver());
 
-    // Calling clearTransientRegistrations() can modify m_registrations, so it's necessary
+    // Calling takeTransientRegistrations() can modify m_registrations, so it's necessary
     // to make a copy of the transient registrations before operating on them.
     Vector<MutationObserverRegistration*, 1> transientRegistrations;
     // 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)
     for (auto* registration : m_registrations) {
         if (registration->hasTransientRegistrations())
             transientRegistrations.append(registration);
     }
     for (auto& registration : transientRegistrations)
-        registration->clearTransientRegistrations();
+        nodesToKeepAlive.append(registration->takeTransientRegistrations());
 
     if (m_records.isEmpty())
         return;
 
     if (m_records.isEmpty())
         return;
index e5d1cf0..2798d58 100644 (file)
@@ -32,6 +32,7 @@
 #pragma once
 
 #include "ExceptionOr.h"
 #pragma once
 
 #include "ExceptionOr.h"
+#include "GCReachableRef.h"
 #include <wtf/Forward.h>
 #include <wtf/HashSet.h>
 #include <wtf/IsoMalloc.h>
 #include <wtf/Forward.h>
 #include <wtf/HashSet.h>
 #include <wtf/IsoMalloc.h>
@@ -109,6 +110,7 @@ private:
 
     Ref<MutationCallback> m_callback;
     Vector<Ref<MutationRecord>> m_records;
 
     Ref<MutationCallback> m_callback;
     Vector<Ref<MutationRecord>> m_records;
+    HashSet<GCReachableRef<Node>> m_pendingTargets;
     HashSet<MutationObserverRegistration*> m_registrations;
     unsigned m_priority;
 };
     HashSet<MutationObserverRegistration*> m_registrations;
     unsigned m_priority;
 };
index 7705f64..442c30a 100644 (file)
@@ -48,13 +48,13 @@ MutationObserverRegistration::MutationObserverRegistration(MutationObserver& obs
 
 MutationObserverRegistration::~MutationObserverRegistration()
 {
 
 MutationObserverRegistration::~MutationObserverRegistration()
 {
-    clearTransientRegistrations();
+    takeTransientRegistrations();
     m_observer->observationEnded(*this);
 }
 
 void MutationObserverRegistration::resetObservation(MutationObserverOptions options, const HashSet<AtomicString>& attributeFilter)
 {
     m_observer->observationEnded(*this);
 }
 
 void MutationObserverRegistration::resetObservation(MutationObserverOptions options, const HashSet<AtomicString>& attributeFilter)
 {
-    clearTransientRegistrations();
+    takeTransientRegistrations();
     m_options = options;
     m_attributeFilter = attributeFilter;
 }
     m_options = options;
     m_attributeFilter = attributeFilter;
 }
@@ -68,28 +68,30 @@ void MutationObserverRegistration::observedSubtreeNodeWillDetach(Node& node)
     m_observer->setHasTransientRegistration();
 
     if (!m_transientRegistrationNodes) {
     m_observer->setHasTransientRegistration();
 
     if (!m_transientRegistrationNodes) {
-        m_transientRegistrationNodes = std::make_unique<HashSet<RefPtr<Node>>>();
+        m_transientRegistrationNodes = std::make_unique<HashSet<GCReachableRef<Node>>>();
 
         ASSERT(!m_nodeKeptAlive);
 
         ASSERT(!m_nodeKeptAlive);
-        m_nodeKeptAlive = &m_node; // Balanced in clearTransientRegistrations.
+        m_nodeKeptAlive = &m_node; // Balanced in takeTransientRegistrations.
     }
     }
-    m_transientRegistrationNodes->add(&node);
+    m_transientRegistrationNodes->add(node);
 }
 
 }
 
-void MutationObserverRegistration::clearTransientRegistrations()
+std::unique_ptr<HashSet<GCReachableRef<Node>>> MutationObserverRegistration::takeTransientRegistrations()
 {
     if (!m_transientRegistrationNodes) {
         ASSERT(!m_nodeKeptAlive);
 {
     if (!m_transientRegistrationNodes) {
         ASSERT(!m_nodeKeptAlive);
-        return;
+        return nullptr;
     }
 
     for (auto& node : *m_transientRegistrationNodes)
         node->unregisterTransientMutationObserver(*this);
 
     }
 
     for (auto& node : *m_transientRegistrationNodes)
         node->unregisterTransientMutationObserver(*this);
 
-    m_transientRegistrationNodes = nullptr;
+    auto returnValue = WTFMove(m_transientRegistrationNodes);
 
     ASSERT(m_nodeKeptAlive);
     m_nodeKeptAlive = nullptr; // Balanced in observeSubtreeNodeWillDetach.
 
     ASSERT(m_nodeKeptAlive);
     m_nodeKeptAlive = nullptr; // Balanced in observeSubtreeNodeWillDetach.
+
+    return returnValue;
 }
 
 bool MutationObserverRegistration::shouldReceiveMutationFrom(Node& node, MutationObserver::MutationType type, const QualifiedName* attributeName) const
 }
 
 bool MutationObserverRegistration::shouldReceiveMutationFrom(Node& node, MutationObserver::MutationType type, const QualifiedName* attributeName) const
@@ -116,7 +118,7 @@ void MutationObserverRegistration::addRegistrationNodesToSet(HashSet<Node*>& nod
     if (!m_transientRegistrationNodes)
         return;
     for (auto& node : *m_transientRegistrationNodes)
     if (!m_transientRegistrationNodes)
         return;
     for (auto& node : *m_transientRegistrationNodes)
-        nodes.add(node.get());
+        nodes.add(node.ptr());
 }
 
 } // namespace WebCore
 }
 
 } // namespace WebCore
index 7b6483f..59d43d2 100644 (file)
@@ -30,6 +30,7 @@
 
 #pragma once
 
 
 #pragma once
 
+#include "GCReachableRef.h"
 #include "MutationObserver.h"
 #include <wtf/HashSet.h>
 #include <wtf/text/AtomicString.h>
 #include "MutationObserver.h"
 #include <wtf/HashSet.h>
 #include <wtf/text/AtomicString.h>
@@ -47,7 +48,7 @@ public:
 
     void resetObservation(MutationObserverOptions, const HashSet<AtomicString>& attributeFilter);
     void observedSubtreeNodeWillDetach(Node&);
 
     void resetObservation(MutationObserverOptions, const HashSet<AtomicString>& attributeFilter);
     void observedSubtreeNodeWillDetach(Node&);
-    void clearTransientRegistrations();
+    std::unique_ptr<HashSet<GCReachableRef<Node>>> takeTransientRegistrations();
     bool hasTransientRegistrations() const { return m_transientRegistrationNodes && !m_transientRegistrationNodes->isEmpty(); }
 
     bool shouldReceiveMutationFrom(Node&, MutationObserver::MutationType, const QualifiedName* attributeName) const;
     bool hasTransientRegistrations() const { return m_transientRegistrationNodes && !m_transientRegistrationNodes->isEmpty(); }
 
     bool shouldReceiveMutationFrom(Node&, MutationObserver::MutationType, const QualifiedName* attributeName) const;
@@ -64,7 +65,7 @@ private:
     Ref<MutationObserver> m_observer;
     Node& m_node;
     RefPtr<Node> m_nodeKeptAlive;
     Ref<MutationObserver> m_observer;
     Node& m_node;
     RefPtr<Node> m_nodeKeptAlive;
-    std::unique_ptr<HashSet<RefPtr<Node>>> m_transientRegistrationNodes;
+    std::unique_ptr<HashSet<GCReachableRef<Node>>> m_transientRegistrationNodes;
     MutationObserverOptions m_options;
     HashSet<AtomicString> m_attributeFilter;
 };
     MutationObserverOptions m_options;
     HashSet<AtomicString> m_attributeFilter;
 };