[V8] Use implicit references instead of object groups to keep registered MutationObse...
authoradamk@chromium.org <adamk@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 6 Mar 2013 23:56:27 +0000 (23:56 +0000)
committeradamk@chromium.org <adamk@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 6 Mar 2013 23:56:27 +0000 (23:56 +0000)
https://bugs.webkit.org/show_bug.cgi?id=111382

Reviewed by Adam Barth.

.:

* ManualTests/mutation-observer-leaks-nodes.html: Added.

Source/WebCore:

Two-phase approach to implicit references: after grouping objects
together, add an implicit reference between each registered node's
group and the MutationObserver's group (which includes wrappers from
all worlds).

Also changed many uses of v8::Value to v8::Object where we know we're
dealing with Object and the V8 API expects them.

Test: ManualTests/mutation-observer-leaks-nodes.html

* bindings/v8/V8GCController.cpp:
(WebCore::ImplicitConnection::ImplicitConnection):
(WebCore::ImplicitConnection::wrapper):
(ImplicitConnection):
(WebCore::ImplicitReference::ImplicitReference): Wrapper class holding a parent who should have an implicit reference to a child.
(ImplicitReference):
(WebCore::operator<): Needed for std::sort() call to avoid the overhead of using a HashMap
(WebCore::WrapperGrouper::addObjectWrapperToGroup):
(WebCore::WrapperGrouper::addNodeWrapperToGroup):
(WebCore::WrapperGrouper::addImplicitReference):
(WrapperGrouper):
(WebCore::WrapperGrouper::apply):

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

ChangeLog
ManualTests/mutation-observer-leaks-nodes.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/bindings/v8/V8GCController.cpp

index 830a29d..0d6640b 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,12 @@
+2013-03-06  Adam Klein  <adamk@chromium.org>
+
+        [V8] Use implicit references instead of object groups to keep registered MutationObservers alive
+        https://bugs.webkit.org/show_bug.cgi?id=111382
+
+        Reviewed by Adam Barth.
+
+        * ManualTests/mutation-observer-leaks-nodes.html: Added.
+
 2013-03-06  Gustavo Noronha Silva  <gns@gnome.org>
 
         Build fix. Fixes problems building code that uses deprecated functions from GTK+ 2,
diff --git a/ManualTests/mutation-observer-leaks-nodes.html b/ManualTests/mutation-observer-leaks-nodes.html
new file mode 100644 (file)
index 0000000..0efc6e8
--- /dev/null
@@ -0,0 +1,14 @@
+<!DOCTYPE html>
+<body>
+<script>
+testRunner.dumpAsText();
+var count = 100;
+var observer = new MutationObserver(function(){});
+for (var i = 0; i < count; i++) {
+    var span = document.createElement('span');
+    observer.observe(span, {attributes:true});
+};
+GCController.collect();
+</script>
+<p>Number of leaked nodes reported by DRT should be less than 100</p>
+</body>
index d152af2..045fccc 100644 (file)
@@ -1,3 +1,33 @@
+2013-03-06  Adam Klein  <adamk@chromium.org>
+
+        [V8] Use implicit references instead of object groups to keep registered MutationObservers alive
+        https://bugs.webkit.org/show_bug.cgi?id=111382
+
+        Reviewed by Adam Barth.
+
+        Two-phase approach to implicit references: after grouping objects
+        together, add an implicit reference between each registered node's
+        group and the MutationObserver's group (which includes wrappers from
+        all worlds).
+
+        Also changed many uses of v8::Value to v8::Object where we know we're
+        dealing with Object and the V8 API expects them.
+
+        Test: ManualTests/mutation-observer-leaks-nodes.html
+
+        * bindings/v8/V8GCController.cpp:
+        (WebCore::ImplicitConnection::ImplicitConnection):
+        (WebCore::ImplicitConnection::wrapper):
+        (ImplicitConnection):
+        (WebCore::ImplicitReference::ImplicitReference): Wrapper class holding a parent who should have an implicit reference to a child.
+        (ImplicitReference):
+        (WebCore::operator<): Needed for std::sort() call to avoid the overhead of using a HashMap
+        (WebCore::WrapperGrouper::addObjectWrapperToGroup):
+        (WebCore::WrapperGrouper::addNodeWrapperToGroup):
+        (WebCore::WrapperGrouper::addImplicitReference):
+        (WrapperGrouper):
+        (WebCore::WrapperGrouper::apply):
+
 2013-03-06  Ankur Taly  <ataly@google.com>
 
         Modify log method in V8DOMActivityLogger so that the apiName and
index fd01a9e..f387752 100644 (file)
@@ -49,13 +49,13 @@ namespace WebCore {
 
 class ImplicitConnection {
 public:
-    ImplicitConnection(void* root, v8::Persistent<v8::Value> wrapper)
+    ImplicitConnection(void* root, v8::Persistent<v8::Object> wrapper)
         : m_root(root)
         , m_wrapper(wrapper)
         , m_rootNode(0)
     {
     }
-    ImplicitConnection(Node* root, v8::Persistent<v8::Value> wrapper)
+    ImplicitConnection(Node* root, v8::Persistent<v8::Object> wrapper)
         : m_root(root)
         , m_wrapper(wrapper)
         , m_rootNode(root)
@@ -63,7 +63,7 @@ public:
     }
 
     void* root() const { return m_root; }
-    v8::Persistent<v8::Value> wrapper() const { return m_wrapper; }
+    v8::Persistent<v8::Object> wrapper() const { return m_wrapper; }
 
     PassOwnPtr<RetainedObjectInfo> retainedObjectInfo()
     {
@@ -74,7 +74,7 @@ public:
 
 private:
     void* m_root;
-    v8::Persistent<v8::Value> m_wrapper;
+    v8::Persistent<v8::Object> m_wrapper;
     Node* m_rootNode;
 };
 
@@ -83,6 +83,22 @@ bool operator<(const ImplicitConnection& left, const ImplicitConnection& right)
     return left.root() < right.root();
 }
 
+struct ImplicitReference {
+    ImplicitReference(void* parent, v8::Persistent<v8::Object> child)
+        : parent(parent)
+        , child(child)
+    {
+    }
+
+    void* parent;
+    v8::Persistent<v8::Object> child;
+};
+
+bool operator<(const ImplicitReference& left, const ImplicitReference& right)
+{
+    return left.parent < right.parent;
+}
+
 class WrapperGrouper {
 public:
     WrapperGrouper()
@@ -90,16 +106,22 @@ public:
         m_liveObjects.append(V8PerIsolateData::current()->ensureLiveRoot());
     }
 
-    void addObjectWrapperToGroup(void* root, v8::Persistent<v8::Value> wrapper)
+    void addObjectWrapperToGroup(void* root, v8::Persistent<v8::Object> wrapper)
     {
         m_connections.append(ImplicitConnection(root, wrapper));
     }
 
-    void addNodeWrapperToGroup(Node* root, v8::Persistent<v8::Value> wrapper)
+    void addNodeWrapperToGroup(Node* root, v8::Persistent<v8::Object> wrapper)
     {
         m_connections.append(ImplicitConnection(root, wrapper));
     }
 
+    void addImplicitReference(void* parent, v8::Persistent<v8::Object> child)
+    {
+        m_references.append(ImplicitReference(parent, child));
+        m_rootGroupMap.add(parent, v8::Persistent<v8::Object>());
+    }
+
     void keepAlive(v8::Persistent<v8::Value> wrapper)
     {
         m_liveObjects.append(wrapper);
@@ -115,6 +137,7 @@ public:
         size_t i = 0;
         while (i < m_connections.size()) {
             void* root = m_connections[i].root();
+            v8::Persistent<v8::Object> groupRepresentativeWrapper = m_connections[i].wrapper();
             OwnPtr<RetainedObjectInfo> retainedObjectInfo = m_connections[i].retainedObjectInfo();
 
             do {
@@ -124,13 +147,37 @@ public:
             if (group.size() > 1)
                 v8::V8::AddObjectGroup(group.data(), group.size(), retainedObjectInfo.leakPtr());
 
+            HashMap<void*, v8::Persistent<v8::Object> >::iterator iter = m_rootGroupMap.find(root);
+            if (iter != m_rootGroupMap.end())
+                iter->value = groupRepresentativeWrapper;
+
             group.shrink(0);
         }
+
+        std::sort(m_references.begin(), m_references.end());
+        i = 0;
+        while (i < m_references.size()) {
+            void* parent = m_references[i].parent;
+            v8::Persistent<v8::Object> parentWrapper = m_rootGroupMap.get(parent);
+            if (parentWrapper.IsEmpty()) {
+                ++i;
+                continue;
+            }
+
+            Vector<v8::Persistent<v8::Value> > children;
+            do {
+                children.append(m_references[i++].child);
+            } while (i < m_references.size() && parent == m_references[i].parent);
+
+            v8::V8::AddImplicitReferences(parentWrapper, children.data(), children.size());
+        }
     }
 
 private:
     Vector<v8::Persistent<v8::Value> > m_liveObjects;
     Vector<ImplicitConnection> m_connections;
+    Vector<ImplicitReference> m_references;
+    HashMap<void*, v8::Persistent<v8::Object> > m_rootGroupMap;
 };
 
 // FIXME: This should use opaque GC roots.
@@ -316,7 +363,7 @@ public:
             MutationObserver* observer = static_cast<MutationObserver*>(object);
             HashSet<Node*> observedNodes = observer->getObservedNodes();
             for (HashSet<Node*>::iterator it = observedNodes.begin(); it != observedNodes.end(); ++it)
-                m_grouper.addObjectWrapperToGroup(V8GCController::opaqueRootForGC(*it, m_isolate), wrapper);
+                m_grouper.addImplicitReference(V8GCController::opaqueRootForGC(*it, m_isolate), wrapper);
         } else {
             ActiveDOMObject* activeDOMObject = type->toActiveDOMObject(wrapper);
             if (activeDOMObject && activeDOMObject->hasPendingActivity())