NodeFilter leaks memory in V8
authorabarth@webkit.org <abarth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 4 Dec 2012 21:46:07 +0000 (21:46 +0000)
committerabarth@webkit.org <abarth@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 4 Dec 2012 21:46:07 +0000 (21:46 +0000)
https://bugs.webkit.org/show_bug.cgi?id=104027

Reviewed by Eric Seidel.

NodeFilter holds a RefPtr to NodeFilterCondition, which holds a
ScopedPersistent to its callback function. If the callback function can
see the NodeFilter wrapper, we have a cycle and a leak.

This patch makes the NodeFilterCondition hold a weak pointer to the
callback function and ties the lifetime of that callback function to
the NodeFilter wrapper (so they live and die together).

* bindings/v8/V8GCController.cpp:
* bindings/v8/V8NodeFilterCondition.cpp:
(WebCore::V8NodeFilterCondition::V8NodeFilterCondition):
(WebCore::V8NodeFilterCondition::weakCallback):
(WebCore):
(WebCore::V8NodeFilterCondition::acceptNode):
* bindings/v8/V8NodeFilterCondition.h:
(WebCore::V8NodeFilterCondition::create):
(V8NodeFilterCondition):
(WebCore::V8NodeFilterCondition::callback):
* dom/NodeFilter.h:
(WebCore::NodeFilter::condition):

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

Source/WebCore/ChangeLog
Source/WebCore/bindings/v8/V8GCController.cpp
Source/WebCore/bindings/v8/V8NodeFilterCondition.cpp
Source/WebCore/bindings/v8/V8NodeFilterCondition.h
Source/WebCore/dom/NodeFilter.h

index 7b2acb9..02b222a 100644 (file)
@@ -1,3 +1,31 @@
+2012-12-04  Adam Barth  <abarth@webkit.org>
+
+        NodeFilter leaks memory in V8
+        https://bugs.webkit.org/show_bug.cgi?id=104027
+
+        Reviewed by Eric Seidel.
+
+        NodeFilter holds a RefPtr to NodeFilterCondition, which holds a
+        ScopedPersistent to its callback function. If the callback function can
+        see the NodeFilter wrapper, we have a cycle and a leak.
+
+        This patch makes the NodeFilterCondition hold a weak pointer to the
+        callback function and ties the lifetime of that callback function to
+        the NodeFilter wrapper (so they live and die together).
+
+        * bindings/v8/V8GCController.cpp:
+        * bindings/v8/V8NodeFilterCondition.cpp:
+        (WebCore::V8NodeFilterCondition::V8NodeFilterCondition):
+        (WebCore::V8NodeFilterCondition::weakCallback):
+        (WebCore):
+        (WebCore::V8NodeFilterCondition::acceptNode):
+        * bindings/v8/V8NodeFilterCondition.h:
+        (WebCore::V8NodeFilterCondition::create):
+        (V8NodeFilterCondition):
+        (WebCore::V8NodeFilterCondition::callback):
+        * dom/NodeFilter.h:
+        (WebCore::NodeFilter::condition):
+
 2012-12-04  Abhishek Arya  <inferno@chromium.org>
 
         Crash in CachedResource::checkNotify due to -webkit-crossfade.
index 99ce359..c69d19e 100644 (file)
@@ -40,6 +40,8 @@
 #include "V8MessagePort.h"
 #include "V8MutationObserver.h"
 #include "V8Node.h"
+#include "V8NodeFilter.h"
+#include "V8NodeFilterCondition.h"
 #include "V8RecursionScope.h"
 #include "WrapperTypeInfo.h"
 #include <algorithm>
@@ -170,6 +172,7 @@ public:
         WrapperTypeInfo* type = V8DOMWrapper::domWrapperType(wrapper);
         void* object = toNative(wrapper);
 
+        // FIXME: Abstract this if cascade into a WrapperTypeInfo function.
         if (V8MessagePort::info.equals(type)) {
             // Mark each port as in-use if it's entangled. For simplicity's sake,
             // we assume all ports are remotely entangled, since the Chromium port
@@ -185,6 +188,9 @@ public:
             for (HashSet<Node*>::iterator it = observedNodes.begin(); it != observedNodes.end(); ++it)
                 m_grouper.addToGroup(V8GCController::opaqueRootForGC(*it), wrapper);
 #endif // ENABLE(MUTATION_OBSERVERS)
+        } else if (V8NodeFilter::info.equals(type)) {
+            NodeFilter* filter = static_cast<NodeFilter*>(object);
+            m_grouper.addToGroup(type->opaqueRootForGC(object, wrapper), static_cast<V8NodeFilterCondition*>(filter->condition())->callback());
         } else {
             ActiveDOMObject* activeDOMObject = type->toActiveDOMObject(wrapper);
             if (activeDOMObject && activeDOMObject->hasPendingActivity())
index 93339ec..c3b672a 100644 (file)
 
 namespace WebCore {
 
-V8NodeFilterCondition::V8NodeFilterCondition(v8::Handle<v8::Value> filter)
-    : m_filter(filter)
+V8NodeFilterCondition::V8NodeFilterCondition(v8::Handle<v8::Value> callback)
+    : m_callback(callback)
 {
+    m_callback.get().MakeWeak(this, weakCallback);
 }
 
 V8NodeFilterCondition::~V8NodeFilterCondition()
 {
 }
 
+void V8NodeFilterCondition::weakCallback(v8::Persistent<v8::Value> value, void* context)
+{
+    V8NodeFilterCondition* condition = static_cast<V8NodeFilterCondition*>(context);
+    ASSERT(condition->callback() == value);
+    condition->m_callback.clear();
+}
+
 short V8NodeFilterCondition::acceptNode(ScriptState* state, Node* node) const
 {
     ASSERT(v8::Context::InContext());
 
-    if (!m_filter->IsObject())
+    if (!m_callback->IsObject())
         return NodeFilter::FILTER_ACCEPT;
 
     v8::TryCatch exceptionCatcher;
 
     v8::Handle<v8::Function> callback;
-    if (m_filter->IsFunction())
-        callback = v8::Handle<v8::Function>::Cast(m_filter.get());
+    if (m_callback->IsFunction())
+        callback = v8::Handle<v8::Function>::Cast(m_callback.get());
     else {
-        v8::Local<v8::Value> value = m_filter->ToObject()->Get(v8::String::New("acceptNode"));
+        v8::Local<v8::Value> value = m_callback->ToObject()->Get(v8::String::New("acceptNode"));
         if (!value->IsFunction()) {
             throwTypeError("NodeFilter object does not have an acceptNode function");
             return NodeFilter::FILTER_REJECT;
index 795f834..7f244df 100644 (file)
@@ -43,19 +43,22 @@ class ScriptState;
 
 class V8NodeFilterCondition : public NodeFilterCondition {
 public:
-    static PassRefPtr<V8NodeFilterCondition> create(v8::Handle<v8::Value> filter)
+    static PassRefPtr<V8NodeFilterCondition> create(v8::Handle<v8::Value> callback)
     {
-        return adoptRef(new V8NodeFilterCondition(filter));
+        return adoptRef(new V8NodeFilterCondition(callback));
     }
 
     virtual ~V8NodeFilterCondition();
 
-    virtual short acceptNode(ScriptState*, Node*) const;
+    virtual short acceptNode(ScriptState*, Node*) const OVERRIDE;
+    v8::Persistent<v8::Value> callback() const { return m_callback.get(); }
 
 private:
-    explicit V8NodeFilterCondition(v8::Handle<v8::Value> filter);
+    explicit V8NodeFilterCondition(v8::Handle<v8::Value> callback);
 
-    ScopedPersistent<v8::Value> m_filter;
+    static void weakCallback(v8::Persistent<v8::Value>, void*);
+
+    ScopedPersistent<v8::Value> m_callback;
 };
 
 } // namespace WebCore
index e7e906a..0515598 100644 (file)
@@ -82,6 +82,7 @@ namespace WebCore {
         short acceptNode(Node* node) const { return acceptNode(scriptStateFromNode(mainThreadNormalWorld(), node), node); }
         
         void setCondition(PassRefPtr<NodeFilterCondition> condition) { ASSERT(!m_condition); m_condition = condition; }
+        NodeFilterCondition* condition() const { return m_condition.get(); }
 
     private:
         explicit NodeFilter(PassRefPtr<NodeFilterCondition> condition) : m_condition(condition) { }