- Revision
- 113378
- Author
- [email protected]
- Date
- 2012-04-05 14:11:28 -0700 (Thu, 05 Apr 2012)
Log Message
Crash in MutationObservers due to an invalid HashSet iterator
https://bugs.webkit.org/show_bug.cgi?id=83304
Reviewed by Ojan Vafai.
Source/WebCore:
If the observed node has been GCed when we clear transient observers
from it, the HashSet iterator in WebKitMutationObserver::deliver would
be invalidated. This patch fixes that behavior by copying the relevant
registrations into a seperate vector first and operating on the copy.
This patch also fixes a bug: transient observers should be cleared
after every microtask, not just when delivering.
Tests: fast/mutation/clear-transient-without-delivery.html
fast/mutation/transient-gc-crash.html
* dom/MutationObserverRegistration.cpp:
(WebCore::MutationObserverRegistration::observedSubtreeNodeWillDetach):
Notify the observer that it has a transient registration so it can be properly cleared.
* dom/MutationObserverRegistration.h:
(WebCore::MutationObserverRegistration::hasTransientRegistrations):
Add an accessor for use when deliver() creates its vector of registrations.
* dom/WebKitMutationObserver.cpp:
(WebCore::WebKitMutationObserver::setHasTransientRegistration): Add this to the active observer set
to allow transient registrations to be cleared appropriately.
(WebCore::WebKitMutationObserver::deliver): Avoid modifying m_registrations while iterating over it.
Clear registrations before checking for a lack of records to deliver.
* dom/WebKitMutationObserver.h:
LayoutTests:
* fast/mutation/clear-transient-without-delivery-expected.txt: Added.
* fast/mutation/clear-transient-without-delivery.html: Added.
* fast/mutation/transient-gc-crash-expected.txt: Added.
* fast/mutation/transient-gc-crash.html: Added.
Modified Paths
Added Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (113377 => 113378)
--- trunk/LayoutTests/ChangeLog 2012-04-05 21:04:19 UTC (rev 113377)
+++ trunk/LayoutTests/ChangeLog 2012-04-05 21:11:28 UTC (rev 113378)
@@ -1,3 +1,15 @@
+2012-04-05 Adam Klein <[email protected]>
+
+ Crash in MutationObservers due to an invalid HashSet iterator
+ https://bugs.webkit.org/show_bug.cgi?id=83304
+
+ Reviewed by Ojan Vafai.
+
+ * fast/mutation/clear-transient-without-delivery-expected.txt: Added.
+ * fast/mutation/clear-transient-without-delivery.html: Added.
+ * fast/mutation/transient-gc-crash-expected.txt: Added.
+ * fast/mutation/transient-gc-crash.html: Added.
+
2012-04-05 Tony Chang <[email protected]>
[chromium] Unreviewed, update expectation for
Added: trunk/LayoutTests/fast/mutation/clear-transient-without-delivery-expected.txt (0 => 113378)
--- trunk/LayoutTests/fast/mutation/clear-transient-without-delivery-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/mutation/clear-transient-without-delivery-expected.txt 2012-04-05 21:11:28 UTC (rev 113378)
@@ -0,0 +1,10 @@
+Transient registrations should be cleared even without delivery.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS mutationsDelivered is false
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/mutation/clear-transient-without-delivery.html (0 => 113378)
--- trunk/LayoutTests/fast/mutation/clear-transient-without-delivery.html (rev 0)
+++ trunk/LayoutTests/fast/mutation/clear-transient-without-delivery.html 2012-04-05 21:11:28 UTC (rev 113378)
@@ -0,0 +1,26 @@
+<script src=""
+<script>
+window.jsTestIsAsync = true;
+description('Transient registrations should be cleared even without delivery.');
+
+var mutationsDelivered = false;
+function callback(mutations) {
+ mutationsDelivered = true;
+}
+var observer = new WebKitMutationObserver(callback);
+
+var div = document.createElement('div');
+var span = div.appendChild(document.createElement('span'));
+observer.observe(div, {attributes: true, subtree: true});
+div.removeChild(span);
+setTimeout(function() {
+ // By the time this function runs the transient registration should be cleared,
+ // so we expect not to be notified of this attribute mutation.
+ span.setAttribute('bar', 'baz');
+ setTimeout(function() {
+ shouldBeFalse('mutationsDelivered');
+ finishJSTest();
+ }, 0);
+}, 0);
+</script>
+<script src=""
Added: trunk/LayoutTests/fast/mutation/transient-gc-crash-expected.txt (0 => 113378)
--- trunk/LayoutTests/fast/mutation/transient-gc-crash-expected.txt (rev 0)
+++ trunk/LayoutTests/fast/mutation/transient-gc-crash-expected.txt 2012-04-05 21:11:28 UTC (rev 113378)
@@ -0,0 +1,10 @@
+Clearing transient observers after observation node is GCed should not cause a crash.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS mutations.length is 1
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
Added: trunk/LayoutTests/fast/mutation/transient-gc-crash.html (0 => 113378)
--- trunk/LayoutTests/fast/mutation/transient-gc-crash.html (rev 0)
+++ trunk/LayoutTests/fast/mutation/transient-gc-crash.html 2012-04-05 21:11:28 UTC (rev 113378)
@@ -0,0 +1,23 @@
+<script src=""
+<script>
+window.jsTestIsAsync = true;
+description('Clearing transient observers after observation node is GCed should not cause a crash.');
+
+function callback(mutations) {
+ window.mutations = mutations;
+}
+var observer = new WebKitMutationObserver(callback);
+
+var div = document.createElement('div');
+var span = div.appendChild(document.createElement('span'));
+observer.observe(div, {attributes: true, subtree: true});
+div.removeChild(span);
+div = null;
+gc();
+span.setAttribute('foo', 'bar');
+setTimeout(function() {
+ shouldBe('mutations.length', '1');
+ finishJSTest();
+}, 0);
+</script>
+<script src=""
Modified: trunk/Source/WebCore/ChangeLog (113377 => 113378)
--- trunk/Source/WebCore/ChangeLog 2012-04-05 21:04:19 UTC (rev 113377)
+++ trunk/Source/WebCore/ChangeLog 2012-04-05 21:11:28 UTC (rev 113378)
@@ -1,5 +1,36 @@
2012-04-05 Adam Klein <[email protected]>
+ Crash in MutationObservers due to an invalid HashSet iterator
+ https://bugs.webkit.org/show_bug.cgi?id=83304
+
+ Reviewed by Ojan Vafai.
+
+ If the observed node has been GCed when we clear transient observers
+ from it, the HashSet iterator in WebKitMutationObserver::deliver would
+ be invalidated. This patch fixes that behavior by copying the relevant
+ registrations into a seperate vector first and operating on the copy.
+
+ This patch also fixes a bug: transient observers should be cleared
+ after every microtask, not just when delivering.
+
+ Tests: fast/mutation/clear-transient-without-delivery.html
+ fast/mutation/transient-gc-crash.html
+
+ * dom/MutationObserverRegistration.cpp:
+ (WebCore::MutationObserverRegistration::observedSubtreeNodeWillDetach):
+ Notify the observer that it has a transient registration so it can be properly cleared.
+ * dom/MutationObserverRegistration.h:
+ (WebCore::MutationObserverRegistration::hasTransientRegistrations):
+ Add an accessor for use when deliver() creates its vector of registrations.
+ * dom/WebKitMutationObserver.cpp:
+ (WebCore::WebKitMutationObserver::setHasTransientRegistration): Add this to the active observer set
+ to allow transient registrations to be cleared appropriately.
+ (WebCore::WebKitMutationObserver::deliver): Avoid modifying m_registrations while iterating over it.
+ Clear registrations before checking for a lack of records to deliver.
+ * dom/WebKitMutationObserver.h:
+
+2012-04-05 Adam Klein <[email protected]>
+
Rebaseline binding tests after r113272.
* bindings/scripts/test/V8/V8TestInterface.cpp:
Modified: trunk/Source/WebCore/dom/MutationObserverRegistration.cpp (113377 => 113378)
--- trunk/Source/WebCore/dom/MutationObserverRegistration.cpp 2012-04-05 21:04:19 UTC (rev 113377)
+++ trunk/Source/WebCore/dom/MutationObserverRegistration.cpp 2012-04-05 21:11:28 UTC (rev 113378)
@@ -71,6 +71,7 @@
return;
node->registerTransientMutationObserver(this);
+ m_observer->setHasTransientRegistration();
if (!m_transientRegistrationNodes) {
m_transientRegistrationNodes = adoptPtr(new NodeHashSet);
Modified: trunk/Source/WebCore/dom/MutationObserverRegistration.h (113377 => 113378)
--- trunk/Source/WebCore/dom/MutationObserverRegistration.h 2012-04-05 21:04:19 UTC (rev 113377)
+++ trunk/Source/WebCore/dom/MutationObserverRegistration.h 2012-04-05 21:11:28 UTC (rev 113378)
@@ -50,6 +50,7 @@
void resetObservation(MutationObserverOptions, const HashSet<AtomicString>& attributeFilter);
void observedSubtreeNodeWillDetach(PassRefPtr<Node>);
void clearTransientRegistrations();
+ bool hasTransientRegistrations() { return m_transientRegistrationNodes && !m_transientRegistrationNodes->isEmpty(); }
void unregister();
bool shouldReceiveMutationFrom(Node*, WebKitMutationObserver::MutationType, const AtomicString& attributeName);
Modified: trunk/Source/WebCore/dom/WebKitMutationObserver.cpp (113377 => 113378)
--- trunk/Source/WebCore/dom/WebKitMutationObserver.cpp 2012-04-05 21:04:19 UTC (rev 113377)
+++ trunk/Source/WebCore/dom/WebKitMutationObserver.cpp 2012-04-05 21:11:28 UTC (rev 113378)
@@ -142,17 +142,30 @@
activeMutationObservers().add(this);
}
+void WebKitMutationObserver::setHasTransientRegistration()
+{
+ ASSERT(isMainThread());
+ activeMutationObservers().add(this);
+}
+
void WebKitMutationObserver::deliver()
{
+ // 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;
+ for (HashSet<MutationObserverRegistration*>::iterator iter = m_registrations.begin(); iter != m_registrations.end(); ++iter) {
+ if ((*iter)->hasTransientRegistrations())
+ transientRegistrations.append(*iter);
+ }
+ for (size_t i = 0; i < transientRegistrations.size(); ++i)
+ transientRegistrations[i]->clearTransientRegistrations();
+
if (m_records.isEmpty())
return;
Vector<RefPtr<MutationRecord> > records;
records.swap(m_records);
- for (HashSet<MutationObserverRegistration*>::iterator iter = m_registrations.begin(); iter != m_registrations.end(); ++iter)
- (*iter)->clearTransientRegistrations();
-
m_callback->handleEvent(&records, this);
}
Modified: trunk/Source/WebCore/dom/WebKitMutationObserver.h (113377 => 113378)
--- trunk/Source/WebCore/dom/WebKitMutationObserver.h 2012-04-05 21:04:19 UTC (rev 113377)
+++ trunk/Source/WebCore/dom/WebKitMutationObserver.h 2012-04-05 21:11:28 UTC (rev 113378)
@@ -84,6 +84,7 @@
void observationStarted(MutationObserverRegistration*);
void observationEnded(MutationObserverRegistration*);
void enqueueMutationRecord(PassRefPtr<MutationRecord>);
+ void setHasTransientRegistration();
private:
struct ObserverLessThan;
