- Revision
- 236781
- Author
- [email protected]
- Date
- 2018-10-02 17:29:46 -0700 (Tue, 02 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.
Source/WebCore:
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.
Test: fast/dom/MutationObserver/mutation-observer-retains-js-wrappers-of-targets-alive.html
* 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:
LayoutTests:
Added a regression test.
* fast/dom/MutationObserver/mutation-observer-retains-js-wrappers-of-targets-alive-expected.txt: Added.
* fast/dom/MutationObserver/mutation-observer-retains-js-wrappers-of-targets-alive.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (236780 => 236781)
--- trunk/LayoutTests/ChangeLog 2018-10-03 00:29:42 UTC (rev 236780)
+++ trunk/LayoutTests/ChangeLog 2018-10-03 00:29:46 UTC (rev 236781)
@@ -1,3 +1,15 @@
+2018-10-01 Ryosuke Niwa <[email protected]>
+
+ 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.
+
+ Added a regression test.
+
+ * fast/dom/MutationObserver/mutation-observer-retains-js-wrappers-of-targets-alive-expected.txt: Added.
+ * fast/dom/MutationObserver/mutation-observer-retains-js-wrappers-of-targets-alive.html: Added.
+
2018-10-02 Chris Dumez <[email protected]>
radio / checkbox inputs should fire "click, input, change" events in order when clicked
Added: trunk/LayoutTests/fast/dom/MutationObserver/mutation-observer-retains-js-wrappers-of-targets-alive-expected.txt (0 => 236781)
--- trunk/LayoutTests/fast/dom/MutationObserver/mutation-observer-retains-js-wrappers-of-targets-alive-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/dom/MutationObserver/mutation-observer-retains-js-wrappers-of-targets-alive-expected.txt 2018-10-03 00:29:46 UTC (rev 236781)
@@ -0,0 +1,23 @@
+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
+
Added: trunk/LayoutTests/fast/dom/MutationObserver/mutation-observer-retains-js-wrappers-of-targets-alive.html (0 => 236781)
--- trunk/LayoutTests/fast/dom/MutationObserver/mutation-observer-retains-js-wrappers-of-targets-alive.html (rev 0)
+++ trunk/LayoutTests/fast/dom/MutationObserver/mutation-observer-retains-js-wrappers-of-targets-alive.html 2018-10-03 00:29:46 UTC (rev 236781)
@@ -0,0 +1,83 @@
+<!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 (236780 => 236781)
--- trunk/Source/WebCore/ChangeLog 2018-10-03 00:29:42 UTC (rev 236780)
+++ trunk/Source/WebCore/ChangeLog 2018-10-03 00:29:46 UTC (rev 236781)
@@ -1,3 +1,52 @@
+2018-10-01 Ryosuke Niwa <[email protected]>
+
+ 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.
+
+ Test: fast/dom/MutationObserver/mutation-observer-retains-js-wrappers-of-targets-alive.html
+
+ * 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-02 Chris Dumez <[email protected]>
radio / checkbox inputs should fire "click, input, change" events in order when clicked
Modified: trunk/Source/WebCore/dom/GCReachableRef.h (236780 => 236781)
--- trunk/Source/WebCore/dom/GCReachableRef.h 2018-10-03 00:29:42 UTC (rev 236780)
+++ trunk/Source/WebCore/dom/GCReachableRef.h 2018-10-03 00:29:46 UTC (rev 236781)
@@ -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 (236780 => 236781)
--- trunk/Source/WebCore/dom/MutationObserver.cpp 2018-10-03 00:29:42 UTC (rev 236780)
+++ trunk/Source/WebCore/dom/MutationObserver.cpp 2018-10-03 00:29:46 UTC (rev 236781)
@@ -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 (236780 => 236781)
--- trunk/Source/WebCore/dom/MutationObserver.h 2018-10-03 00:29:42 UTC (rev 236780)
+++ trunk/Source/WebCore/dom/MutationObserver.h 2018-10-03 00:29:46 UTC (rev 236781)
@@ -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 (236780 => 236781)
--- trunk/Source/WebCore/dom/MutationObserverRegistration.cpp 2018-10-03 00:29:42 UTC (rev 236780)
+++ trunk/Source/WebCore/dom/MutationObserverRegistration.cpp 2018-10-03 00:29:46 UTC (rev 236781)
@@ -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 (236780 => 236781)
--- trunk/Source/WebCore/dom/MutationObserverRegistration.h 2018-10-03 00:29:42 UTC (rev 236780)
+++ trunk/Source/WebCore/dom/MutationObserverRegistration.h 2018-10-03 00:29:46 UTC (rev 236781)
@@ -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;
};
