- Revision
- 236801
- Author
- rn...@webkit.org
- Date
- 2018-10-03 11:16:39 -0700 (Wed, 03 Oct 2018)
Log Message
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:
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (236800 => 236801)
--- trunk/Source/WebCore/ChangeLog 2018-10-03 18:14:29 UTC (rev 236800)
+++ trunk/Source/WebCore/ChangeLog 2018-10-03 18:16:39 UTC (rev 236801)
@@ -1,3 +1,52 @@
+2018-10-03 Ryosuke Niwa <rn...@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 <d...@apple.com>
Make the Pointer Events feature description valid
Modified: trunk/Source/WebCore/dom/GCReachableRef.h (236800 => 236801)
--- trunk/Source/WebCore/dom/GCReachableRef.h 2018-10-03 18:14:29 UTC (rev 236800)
+++ trunk/Source/WebCore/dom/GCReachableRef.h 2018-10-03 18:16:39 UTC (rev 236801)
@@ -27,7 +27,7 @@
#include <wtf/DumbPtrTraits.h>
#include <wtf/HashCountedSet.h>
-#include <wtf/Ref.h>
+#include <wtf/RefPtr.h>
namespace WebCore {
@@ -69,12 +69,83 @@
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(); }
+ // 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;
};
-}
+} // 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 (236800 => 236801)
--- trunk/Source/WebCore/dom/MutationObserver.cpp 2018-10-03 18:14:29 UTC (rev 236800)
+++ trunk/Source/WebCore/dom/MutationObserver.cpp 2018-10-03 18:16:39 UTC (rev 236801)
@@ -114,11 +114,14 @@
{
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)
@@ -181,6 +184,8 @@
void MutationObserver::enqueueMutationRecord(Ref<MutationRecord>&& mutation)
{
ASSERT(isMainThread());
+ ASSERT(mutation->target());
+ m_pendingTargets.add(*mutation->target());
m_records.append(WTFMove(mutation));
activeMutationObservers().add(this);
@@ -221,15 +226,18 @@
{
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;
+ 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)
- registration->clearTransientRegistrations();
+ nodesToKeepAlive.append(registration->takeTransientRegistrations());
if (m_records.isEmpty())
return;
Modified: trunk/Source/WebCore/dom/MutationObserver.h (236800 => 236801)
--- trunk/Source/WebCore/dom/MutationObserver.h 2018-10-03 18:14:29 UTC (rev 236800)
+++ trunk/Source/WebCore/dom/MutationObserver.h 2018-10-03 18:16:39 UTC (rev 236801)
@@ -32,6 +32,7 @@
#pragma once
#include "ExceptionOr.h"
+#include "GCReachableRef.h"
#include <wtf/Forward.h>
#include <wtf/HashSet.h>
#include <wtf/IsoMalloc.h>
@@ -109,6 +110,7 @@
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 (236800 => 236801)
--- trunk/Source/WebCore/dom/MutationObserverRegistration.cpp 2018-10-03 18:14:29 UTC (rev 236800)
+++ trunk/Source/WebCore/dom/MutationObserverRegistration.cpp 2018-10-03 18:16:39 UTC (rev 236801)
@@ -48,13 +48,13 @@
MutationObserverRegistration::~MutationObserverRegistration()
{
- clearTransientRegistrations();
+ takeTransientRegistrations();
m_observer->observationEnded(*this);
}
void MutationObserverRegistration::resetObservation(MutationObserverOptions options, const HashSet<AtomicString>& attributeFilter)
{
- clearTransientRegistrations();
+ takeTransientRegistrations();
m_options = options;
m_attributeFilter = attributeFilter;
}
@@ -68,28 +68,30 @@
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);
- 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);
- return;
+ return nullptr;
}
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.
+
+ return returnValue;
}
bool MutationObserverRegistration::shouldReceiveMutationFrom(Node& node, MutationObserver::MutationType type, const QualifiedName* attributeName) const
@@ -116,7 +118,7 @@
if (!m_transientRegistrationNodes)
return;
for (auto& node : *m_transientRegistrationNodes)
- nodes.add(node.get());
+ nodes.add(node.ptr());
}
} // namespace WebCore
Modified: trunk/Source/WebCore/dom/MutationObserverRegistration.h (236800 => 236801)
--- trunk/Source/WebCore/dom/MutationObserverRegistration.h 2018-10-03 18:14:29 UTC (rev 236800)
+++ trunk/Source/WebCore/dom/MutationObserverRegistration.h 2018-10-03 18:16:39 UTC (rev 236801)
@@ -30,6 +30,7 @@
#pragma once
+#include "GCReachableRef.h"
#include "MutationObserver.h"
#include <wtf/HashSet.h>
#include <wtf/text/AtomicString.h>
@@ -47,7 +48,7 @@
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;
@@ -64,7 +65,7 @@
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;
};
