Attachment 175088 Details for Bug 102328 – Patch for landing

[patch] Patch for landing

bug-102328-20121119171715.patch (text/plain), 15.33 KB, created by Adam Klein on 2012-11-19 17:19:25 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Adam Klein

Created: 2012-11-19 17:19:25 PST

Size: 15.33 KB

patch

obsolete

>Subversion Revision: 135199
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index c51db5481e7584f5b0cecdce8d7bb52e51b5ee19..1283f2b287b82d8e0216cea17d9d54b59f09a8f4 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,32 @@
>+2012-11-19  Adam Klein  <adamk@chromium.org>
>+
>+        MutationObserver wrapper should not be collected while still observing
>+        https://bugs.webkit.org/show_bug.cgi?id=102328
>+
>+        Reviewed by Adam Barth.
>+
>+        Use the new opaqueRootForGC helper in V8GCController to put each
>+        MutationObserver wrapper in the same object group as the nodes it's
>+        observing.
>+
>+        Only includes V8 impl for now, JSC impl coming soon.
>+
>+        Tests: fast/mutation/observer-wrapper-dropoff-transient.html
>+               fast/mutation/observer-wrapper-dropoff.html
>+
>+        * bindings/v8/V8GCController.cpp: Add custom code for MutationObserver
>+        with a FIXME to move this out once we update the opaque roots API.
>+        * dom/MutationObserver.cpp:
>+        (WebCore::MutationObserver::getObservedNodes): Plumbing to expose the observed nodes
>+        to the GC controller.
>+        (WebCore):
>+        * dom/MutationObserver.h:
>+        * dom/MutationObserverRegistration.cpp:
>+        (WebCore::MutationObserverRegistration::addRegistrationNodesToSet): More plumbing.
>+        (WebCore):
>+        * dom/MutationObserverRegistration.h:
>+        (MutationObserverRegistration):
>+
> 2012-11-19  Alpha Lam  <hclam@chromium.org>
> 
>         [chromium] Lazy image decoding without cache
>diff --git a/Source/WebCore/bindings/v8/V8GCController.cpp b/Source/WebCore/bindings/v8/V8GCController.cpp
>index 98737b3bb5760e3e8b0c31d1b3de2529e32a8c34..4cc03fbdc38f11b52dff085b288b7ca4b33ea6e5 100644
>--- a/Source/WebCore/bindings/v8/V8GCController.cpp
>+++ b/Source/WebCore/bindings/v8/V8GCController.cpp
>@@ -38,6 +38,7 @@
> #include "V8AbstractEventListener.h"
> #include "V8Binding.h"
> #include "V8MessagePort.h"
>+#include "V8MutationObserver.h"
> #include "V8Node.h"
> #include "V8RecursionScope.h"
> #include "WrapperTypeInfo.h"
>@@ -176,6 +177,14 @@ public:
>             MessagePort* port = static_cast<MessagePort*>(object);
>             if (port->isEntangled() || port->hasPendingActivity())
>                 m_grouper.keepAlive(wrapper);
>+#if ENABLE(MUTATION_OBSERVERS)
>+        } else if (V8MutationObserver::info.equals(type)) {
>+            // FIXME: Allow opaqueRootForGC to operate on multiple roots and move this logic into V8MutationObserverCustom.
>+            MutationObserver* observer = static_cast<MutationObserver*>(object);
>+            HashSet<Node*> observedNodes = observer->getObservedNodes();
>+            for (HashSet<Node*>::iterator it = observedNodes.begin(); it != observedNodes.end(); ++it)
>+                m_grouper.addToGroup(V8GCController::opaqueRootForGC(*it), wrapper);
>+#endif // ENABLE(MUTATION_OBSERVERS)
>         } else {
>             ActiveDOMObject* activeDOMObject = type->toActiveDOMObject(wrapper);
>             if (activeDOMObject && activeDOMObject->hasPendingActivity())
>diff --git a/Source/WebCore/dom/MutationObserver.cpp b/Source/WebCore/dom/MutationObserver.cpp
>index 0c210fa46bada69263c4ca8b01d925bcf89a34d9..7b1ebab4c225547206426526571348dfa3fb4ff2 100644
>--- a/Source/WebCore/dom/MutationObserver.cpp
>+++ b/Source/WebCore/dom/MutationObserver.cpp
>@@ -167,6 +167,14 @@ void MutationObserver::setHasTransientRegistration()
>     activeMutationObservers().add(this);
> }
> 
>+HashSet<Node*> MutationObserver::getObservedNodes() const
>+{
>+    HashSet<Node*> observedNodes;
>+    for (HashSet<MutationObserverRegistration*>::const_iterator iter = m_registrations.begin(); iter != m_registrations.end(); ++iter)
>+        (*iter)->addRegistrationNodesToSet(observedNodes);
>+    return observedNodes;
>+}
>+
> void MutationObserver::deliver()
> {
>     // Calling clearTransientRegistrations() can modify m_registrations, so it's necessary
>diff --git a/Source/WebCore/dom/MutationObserver.h b/Source/WebCore/dom/MutationObserver.h
>index fadb378d1efdebd1cbce3247ed9f9fb103773401..677c637ff4100629dbd3dc408711033cef98bb71 100644
>--- a/Source/WebCore/dom/MutationObserver.h
>+++ b/Source/WebCore/dom/MutationObserver.h
>@@ -87,6 +87,8 @@ public:
>     void enqueueMutationRecord(PassRefPtr<MutationRecord>);
>     void setHasTransientRegistration();
> 
>+    HashSet<Node*> getObservedNodes() const;
>+
> private:
>     struct ObserverLessThan;
> 
>diff --git a/Source/WebCore/dom/MutationObserverRegistration.cpp b/Source/WebCore/dom/MutationObserverRegistration.cpp
>index 4c30ac30cc8bb97badfe00843952ed0bd523956a..ed7a3508a840e4c9c5479ff60bfaa7a688e35d07 100644
>--- a/Source/WebCore/dom/MutationObserverRegistration.cpp
>+++ b/Source/WebCore/dom/MutationObserverRegistration.cpp
>@@ -124,6 +124,15 @@ bool MutationObserverRegistration::shouldReceiveMutationFrom(Node* node, Mutatio
>     return m_attributeFilter.contains(attributeName->localName());
> }
> 
>+void MutationObserverRegistration::addRegistrationNodesToSet(HashSet<Node*>& nodes) const
>+{
>+    nodes.add(m_registrationNode);
>+    if (!m_transientRegistrationNodes)
>+        return;
>+    for (NodeHashSet::const_iterator iter = m_transientRegistrationNodes->begin(); iter != m_transientRegistrationNodes->end(); ++iter)
>+        nodes.add(iter->get());
>+}
>+
> } // namespace WebCore
> 
> #endif // ENABLE(MUTATION_OBSERVERS)
>diff --git a/Source/WebCore/dom/MutationObserverRegistration.h b/Source/WebCore/dom/MutationObserverRegistration.h
>index af222e8c5b4ad842234456cfb62edd031b85cab3..f72b6447920eef3a1b556b3d1e79b96964fb446a 100644
>--- a/Source/WebCore/dom/MutationObserverRegistration.h
>+++ b/Source/WebCore/dom/MutationObserverRegistration.h
>@@ -60,6 +60,8 @@ public:
>     MutationRecordDeliveryOptions deliveryOptions() const { return m_options & (MutationObserver::AttributeOldValue | MutationObserver::CharacterDataOldValue); }
>     MutationObserverOptions mutationTypes() const { return m_options & MutationObserver::AllMutationTypes; }
> 
>+    void addRegistrationNodesToSet(HashSet<Node*>&) const;
>+
> private:
>     MutationObserverRegistration(PassRefPtr<MutationObserver>, Node*, MutationObserverOptions, const HashSet<AtomicString>& attributeFilter);
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 4ef38c5811e41e3945e652098745aafc9678044b..033f0a68fe6c37b4cb1bf2732416b996620812d5 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,20 @@
>+2012-11-19  Adam Klein  <adamk@chromium.org>
>+
>+        MutationObserver wrapper should not be collected while still observing
>+        https://bugs.webkit.org/show_bug.cgi?id=102328
>+
>+        Reviewed by Adam Barth.
>+
>+        Tests showing that observers are kept alive, both in the simple case
>+        and in the transient registered observer case (where the original
>+        registration node is GCed but the transient observation node is still
>+        alive).
>+
>+        * fast/mutation/observer-wrapper-dropoff-expected.txt: Added.
>+        * fast/mutation/observer-wrapper-dropoff-transient-expected.txt: Added.
>+        * fast/mutation/observer-wrapper-dropoff-transient.html: Added.
>+        * fast/mutation/observer-wrapper-dropoff.html: Added.
>+
> 2012-11-19  Abhishek Arya  <inferno@chromium.org>
> 
>         Crash in ApplyStyleCommand::cleanupUnstyledAppleStyleSpans.
>diff --git a/LayoutTests/fast/mutation/observer-wrapper-dropoff-expected.txt b/LayoutTests/fast/mutation/observer-wrapper-dropoff-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..c8c0f2e9dcc64165dd82a7d124c5f329ec3be93c
>--- /dev/null
>+++ b/LayoutTests/fast/mutation/observer-wrapper-dropoff-expected.txt
>@@ -0,0 +1,10 @@
>+MutationObserver wrappers should survive GC for passing into the callback even if JS has lost references.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+PASS observer.testProperty is true
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/fast/mutation/observer-wrapper-dropoff-transient-expected.txt b/LayoutTests/fast/mutation/observer-wrapper-dropoff-transient-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..963a553ec2236cc1fedd31e8edf11a441c3b6bb0
>--- /dev/null
>+++ b/LayoutTests/fast/mutation/observer-wrapper-dropoff-transient-expected.txt
>@@ -0,0 +1,10 @@
>+MutationObserver wrappers should survive GC for passing into the callback even if JS has lost references and the only remaining observations are transient.
>+
>+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
>+
>+
>+PASS observer.testProperty is true
>+PASS successfullyParsed is true
>+
>+TEST COMPLETE
>+
>diff --git a/LayoutTests/fast/mutation/observer-wrapper-dropoff-transient.html b/LayoutTests/fast/mutation/observer-wrapper-dropoff-transient.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..cdc84258c0e7a87909af22106b6c1528b885d465
>--- /dev/null
>+++ b/LayoutTests/fast/mutation/observer-wrapper-dropoff-transient.html
>@@ -0,0 +1,33 @@
>+<!DOCTYPE html>
>+
>+<script src="../js/resources/js-test-pre.js"></script>
>+
>+<script>
>+description('MutationObserver wrappers should survive GC for passing into the callback even if JS has lost references and the only remaining observations are transient.');
>+
>+jsTestIsAsync = true;
>+
>+function addObserver(node, fn) {
>+    var observer = new WebKitMutationObserver(fn);
>+    observer.testProperty = true;
>+    observer.observe(node, {attributes:true, subtree: true});
>+}
>+
>+onload = function() {
>+    var root = document.createElement('div');
>+    var child = root.appendChild(document.createElement('span'));
>+    addObserver(root, function(records, observer) {
>+        window.observer = observer;
>+        shouldBe('observer.testProperty', 'true');
>+        finishJSTest();
>+    });
>+
>+    root.removeChild(child);
>+    child.setAttribute('foo', 'bar');
>+    root = null;
>+
>+    gc();
>+};
>+</script>
>+
>+<script src="../js/resources/js-test-post.js"></script>
>diff --git a/LayoutTests/fast/mutation/observer-wrapper-dropoff.html b/LayoutTests/fast/mutation/observer-wrapper-dropoff.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..c5dbcbbf095edf4450f456ce13a6363673ce14b8
>--- /dev/null
>+++ b/LayoutTests/fast/mutation/observer-wrapper-dropoff.html
>@@ -0,0 +1,29 @@
>+<!DOCTYPE html>
>+
>+<script src="../js/resources/js-test-pre.js"></script>
>+
>+<script>
>+description('MutationObserver wrappers should survive GC for passing into the callback even if JS has lost references.');
>+
>+jsTestIsAsync = true;
>+
>+function addObserver(node, fn) {
>+    var observer = new WebKitMutationObserver(fn);
>+    observer.testProperty = true;
>+    observer.observe(node, {attributes:true});
>+}
>+
>+onload = function() {
>+    addObserver(document.body, function(records, observer) {
>+        window.observer = observer;
>+        shouldBe('observer.testProperty', 'true');
>+        finishJSTest();
>+    });
>+
>+    gc();
>+
>+    document.body.setAttribute('touch', 'the node');
>+};
>+</script>
>+
>+<script src="../js/resources/js-test-post.js"></script>
>diff --git a/LayoutTests/platform/efl/TestExpectations b/LayoutTests/platform/efl/TestExpectations
>index 26dc41139a18b600fa24559a153aef33111796f4..667867ada83f1a6cb16eb0ced754dd3f9b37dbb0 100644
>--- a/LayoutTests/platform/efl/TestExpectations
>+++ b/LayoutTests/platform/efl/TestExpectations
>@@ -1713,3 +1713,6 @@ webkit.org/b/102364 compositing/overflow/scrolling-without-painting.html [ Failu
> webkit.org/b/102367 fast/forms/zoomed-controls.html [ ImageOnlyFailure Missing ]
> 
> webkit.org/b/102493 fast/events/mouse-cursor.html [ Failure ]
>+
>+webkit.org/b/102744 fast/mutation/observer-wrapper-dropoff.html [ Skip ]
>+webkit.org/b/102744 fast/mutation/observer-wrapper-dropoff-transient.html [ Skip ]
>diff --git a/LayoutTests/platform/gtk/TestExpectations b/LayoutTests/platform/gtk/TestExpectations
>index 57a1030e09c25be17496249b20d3b031bb526889..cee57089ef8add7a7338c73082fb432edbffa51e 100644
>--- a/LayoutTests/platform/gtk/TestExpectations
>+++ b/LayoutTests/platform/gtk/TestExpectations
>@@ -1387,6 +1387,9 @@ webkit.org/b/102349 fast/dom/shadow/pseudoclass-update-enabled-area.html [ Image
> 
> webkit.org/b/102586 media/video-src-blob.html [ Failure ]
> 
>+webkit.org/b/102744 fast/mutation/observer-wrapper-dropoff.html [ Skip ]
>+webkit.org/b/102744 fast/mutation/observer-wrapper-dropoff-transient.html [ Skip ]
>+
> #////////////////////////////////////////////////////////////////////////////////////////
> # End of Tests failing
> #////////////////////////////////////////////////////////////////////////////////////////
>diff --git a/LayoutTests/platform/mac/TestExpectations b/LayoutTests/platform/mac/TestExpectations
>index 0400dfa6212b38e897c74c73d720d4987223b9e3..e8d3f766c79b9ed6b876ff318460155109af0439 100644
>--- a/LayoutTests/platform/mac/TestExpectations
>+++ b/LayoutTests/platform/mac/TestExpectations
>@@ -1193,3 +1193,6 @@ Bug(jernoble) [ MountainLion Lion SnowLeopard ] media/video-src-blob.html
> 
> # Hangs safari apparently due to an issue with weird multi-frame CUR image files
> webkit.org/b/101811 fast/events/mouse-cursor-multiframecur.html [ Skip ]
>+
>+webkit.org/b/102744 fast/mutation/observer-wrapper-dropoff.html [ Skip ]
>+webkit.org/b/102744 fast/mutation/observer-wrapper-dropoff-transient.html [ Skip ]
>diff --git a/LayoutTests/platform/qt/TestExpectations b/LayoutTests/platform/qt/TestExpectations
>index 94ac65c228c044c8b54af26aa731d8eb61bf0d52..e51a81950940aac82601f6b6e6636c8acf35a01c 100644
>--- a/LayoutTests/platform/qt/TestExpectations
>+++ b/LayoutTests/platform/qt/TestExpectations
>@@ -2386,3 +2386,6 @@ webkit.org/b/101925 css3/filters/effect-reference-ordering-hw.html
> 
> # [Qt] fast/block/float/overhanging-tall-block.html asserts after r135025
> webkit.org/b/102675 [ Debug ] fast/block/float/overhanging-tall-block.html
>+
>+webkit.org/b/102744 fast/mutation/observer-wrapper-dropoff.html [ Skip ]
>+webkit.org/b/102744 fast/mutation/observer-wrapper-dropoff-transient.html [ Skip ]
>diff --git a/LayoutTests/platform/win/TestExpectations b/LayoutTests/platform/win/TestExpectations
>index 78a9963a3f02450bd636c69c1e7a358925e3fff0..6d0742d56f13f8177305653d605b90ae6f94cf60 100644
>--- a/LayoutTests/platform/win/TestExpectations
>+++ b/LayoutTests/platform/win/TestExpectations
>@@ -2430,3 +2430,6 @@ fast/xmlhttprequest/xmlhttprequest-recursive-sync-event.html
> fast/css3-text/css3-text-align-last/getComputedStyle/getComputedStyle-text-align-last-inherited.html
> fast/css3-text/css3-text-align-last/getComputedStyle/getComputedStyle-text-align-last.html
> fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-line.html
>+
>+webkit.org/b/102744 fast/mutation/observer-wrapper-dropoff.html [ Skip ]
>+webkit.org/b/102744 fast/mutation/observer-wrapper-dropoff-transient.html [ Skip ]
>diff --git a/LayoutTests/platform/wincairo/TestExpectations b/LayoutTests/platform/wincairo/TestExpectations
>index 4378297c972f12c33ab615c6c8a90b88b7287aee..0810d9bff48808e663a58e48821d577780afcf46 100644
>--- a/LayoutTests/platform/wincairo/TestExpectations
>+++ b/LayoutTests/platform/wincairo/TestExpectations
>@@ -2961,3 +2961,6 @@ fast/xmlhttprequest/xmlhttprequest-recursive-sync-event.html
> fast/css3-text/css3-text-align-last/getComputedStyle/getComputedStyle-text-align-last-inherited.html
> fast/css3-text/css3-text-align-last/getComputedStyle/getComputedStyle-text-align-last.html
> fast/css3-text/css3-text-decoration/getComputedStyle/getComputedStyle-text-decoration-line.html
>+
>+webkit.org/b/102744 fast/mutation/observer-wrapper-dropoff.html [ Skip ]
>+webkit.org/b/102744 fast/mutation/observer-wrapper-dropoff-transient.html [ Skip ]

Actions: View | Formatted Diff | Diff

Attachments on bug 102328: 174338 | 174350 | 174483 | 175083 | 175088