Title: [135305] trunk
Revision
135305
Author
commit-qu...@webkit.org
Date
2012-11-20 11:59:14 -0800 (Tue, 20 Nov 2012)

Log Message

Store MutationObserver callback in a hidden property for V8
https://bugs.webkit.org/show_bug.cgi?id=102555

Patch by Elliott Sprehn <espr...@chromium.org> on 2012-11-20
Reviewed by Adam Barth.

.:

Test for reference cycle leaks with mutation observers. There doesn't seem
to be a way to check this for v8, but if you manually run you can see if it
leaks observers.

* ManualTests/leak-cycle-observer-wrapper.html: Added.

Source/WebCore:

To prevent circular reference leaks we should store the MutationObserver
callback in a hidden property on the wrapper of the observer.

This is done by extending the code generator to support a new owner
argument to ::create() that lets you set the owner of the callback where
the hidden property should be stored.

Test: ManualTests/leak-cycle-observer-wrapper.html

* bindings/scripts/CodeGeneratorV8.pm:
(GenerateCallbackHeader):
(GenerateCallbackImplementation):
* bindings/scripts/test/V8/V8TestCallback.cpp: rebaselined.
* bindings/scripts/test/V8/V8TestCallback.h: rebaselined.
* bindings/v8/V8HiddenPropertyName.h:
* bindings/v8/custom/V8MutationObserverCustom.cpp:
(WebCore::V8MutationObserver::constructorCallback):

Modified Paths

Added Paths

Diff

Modified: trunk/ChangeLog (135304 => 135305)


--- trunk/ChangeLog	2012-11-20 19:58:45 UTC (rev 135304)
+++ trunk/ChangeLog	2012-11-20 19:59:14 UTC (rev 135305)
@@ -1,3 +1,16 @@
+2012-11-20  Elliott Sprehn  <espr...@chromium.org>
+
+        Store MutationObserver callback in a hidden property for V8
+        https://bugs.webkit.org/show_bug.cgi?id=102555
+
+        Reviewed by Adam Barth.
+
+        Test for reference cycle leaks with mutation observers. There doesn't seem
+        to be a way to check this for v8, but if you manually run you can see if it
+        leaks observers.
+
+        * ManualTests/leak-cycle-observer-wrapper.html: Added.
+
 2012-11-20  Carlos Garcia Campos  <cgar...@igalia.com>
 
         Unreviewed. Update NEWS and configure.ac for 1.11.2 release

Added: trunk/ManualTests/leak-cycle-observer-wrapper.html (0 => 135305)


--- trunk/ManualTests/leak-cycle-observer-wrapper.html	                        (rev 0)
+++ trunk/ManualTests/leak-cycle-observer-wrapper.html	2012-11-20 19:59:14 UTC (rev 135305)
@@ -0,0 +1,18 @@
+<!DOCTYPE html>
+
+<p>
+    Tests that reference cycles between the observer and the callback do not
+    create leaks.
+</p>
+
+<script>
+    if (window.testRunner)
+        testRunner.dumpAsText();
+
+    function leak() {
+        var observer = new WebKitMutationObserver(function() { observer.disconnect(); });
+    }
+
+    for (i=0; i < 1000; i++) leak();
+    gc();
+</script>

Modified: trunk/Source/WebCore/ChangeLog (135304 => 135305)


--- trunk/Source/WebCore/ChangeLog	2012-11-20 19:58:45 UTC (rev 135304)
+++ trunk/Source/WebCore/ChangeLog	2012-11-20 19:59:14 UTC (rev 135305)
@@ -1,3 +1,28 @@
+2012-11-20  Elliott Sprehn  <espr...@chromium.org>
+
+        Store MutationObserver callback in a hidden property for V8
+        https://bugs.webkit.org/show_bug.cgi?id=102555
+
+        Reviewed by Adam Barth.
+
+        To prevent circular reference leaks we should store the MutationObserver
+        callback in a hidden property on the wrapper of the observer.
+
+        This is done by extending the code generator to support a new owner
+        argument to ::create() that lets you set the owner of the callback where
+        the hidden property should be stored.
+
+        Test: ManualTests/leak-cycle-observer-wrapper.html
+
+        * bindings/scripts/CodeGeneratorV8.pm:
+        (GenerateCallbackHeader):
+        (GenerateCallbackImplementation):
+        * bindings/scripts/test/V8/V8TestCallback.cpp: rebaselined.
+        * bindings/scripts/test/V8/V8TestCallback.h: rebaselined.
+        * bindings/v8/V8HiddenPropertyName.h:
+        * bindings/v8/custom/V8MutationObserverCustom.cpp:
+        (WebCore::V8MutationObserver::constructorCallback):
+
 2012-11-20  Abhishek Arya  <infe...@chromium.org>
 
         Crash in FrameLoader::stopLoading.

Modified: trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm (135304 => 135305)


--- trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm	2012-11-20 19:58:45 UTC (rev 135304)
+++ trunk/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm	2012-11-20 19:59:14 UTC (rev 135305)
@@ -3246,11 +3246,11 @@
 
     push(@headerContent, <<END);
 public:
-    static PassRefPtr<${v8InterfaceName}> create(v8::Local<v8::Value> value, ScriptExecutionContext* context)
+    static PassRefPtr<${v8InterfaceName}> create(v8::Handle<v8::Value> value, ScriptExecutionContext* context, v8::Handle<v8::Object> owner = v8::Handle<v8::Object>())
     {
         ASSERT(value->IsObject());
         ASSERT(context);
-        return adoptRef(new ${v8InterfaceName}(value->ToObject(), context));
+        return adoptRef(new ${v8InterfaceName}(value->ToObject(), context, owner));
     }
 
     virtual ~${v8InterfaceName}();
@@ -3282,8 +3282,16 @@
     push(@headerContent, <<END);
 
 private:
-    ${v8InterfaceName}(v8::Local<v8::Object>, ScriptExecutionContext*);
+    ${v8InterfaceName}(v8::Handle<v8::Object>, ScriptExecutionContext*, v8::Handle<v8::Object>);
 
+    static void weakCallback(v8::Persistent<v8::Value> wrapper, void* parameter)
+    {
+        ${v8InterfaceName}* object = static_cast<${v8InterfaceName}*>(parameter);
+        object->m_callback.Dispose();
+        object->m_callback.Clear();
+    }
+
+    // FIXME: m_callback should be a ScopedPersistent.
     v8::Persistent<v8::Object> m_callback;
     WorldContextHandle m_worldContext;
 };
@@ -3314,16 +3322,21 @@
     push(@implContent, "#include <wtf/Assertions.h>\n\n");
     push(@implContent, "namespace WebCore {\n\n");
     push(@implContent, <<END);
-${v8InterfaceName}::${v8InterfaceName}(v8::Local<v8::Object> callback, ScriptExecutionContext* context)
+${v8InterfaceName}::${v8InterfaceName}(v8::Handle<v8::Object> callback, ScriptExecutionContext* context, v8::Handle<v8::Object> owner)
     : ActiveDOMCallback(context)
     , m_callback(v8::Persistent<v8::Object>::New(callback))
     , m_worldContext(UseCurrentWorld)
 {
+    if (!owner.IsEmpty()) {
+        owner->SetHiddenValue(V8HiddenPropertyName::callback(), callback);
+        m_callback.MakeWeak(this, &${v8InterfaceName}::weakCallback);
+    }
 }
 
 ${v8InterfaceName}::~${v8InterfaceName}()
 {
-    m_callback.Dispose();
+    if (!m_callback.IsEmpty())
+        m_callback.Dispose();
 }
 
 END

Modified: trunk/Source/WebCore/bindings/scripts/test/V8/V8TestCallback.cpp (135304 => 135305)


--- trunk/Source/WebCore/bindings/scripts/test/V8/V8TestCallback.cpp	2012-11-20 19:58:45 UTC (rev 135304)
+++ trunk/Source/WebCore/bindings/scripts/test/V8/V8TestCallback.cpp	2012-11-20 19:59:14 UTC (rev 135305)
@@ -39,16 +39,21 @@
 
 namespace WebCore {
 
-V8TestCallback::V8TestCallback(v8::Local<v8::Object> callback, ScriptExecutionContext* context)
+V8TestCallback::V8TestCallback(v8::Handle<v8::Object> callback, ScriptExecutionContext* context, v8::Handle<v8::Object> owner)
     : ActiveDOMCallback(context)
     , m_callback(v8::Persistent<v8::Object>::New(callback))
     , m_worldContext(UseCurrentWorld)
 {
+    if (!owner.IsEmpty()) {
+        owner->SetHiddenValue(V8HiddenPropertyName::callback(), callback);
+        m_callback.MakeWeak(this, &V8TestCallback::weakCallback);
+    }
 }
 
 V8TestCallback::~V8TestCallback()
 {
-    m_callback.Dispose();
+    if (!m_callback.IsEmpty())
+        m_callback.Dispose();
 }
 
 // Functions

Modified: trunk/Source/WebCore/bindings/scripts/test/V8/V8TestCallback.h (135304 => 135305)


--- trunk/Source/WebCore/bindings/scripts/test/V8/V8TestCallback.h	2012-11-20 19:58:45 UTC (rev 135304)
+++ trunk/Source/WebCore/bindings/scripts/test/V8/V8TestCallback.h	2012-11-20 19:59:14 UTC (rev 135305)
@@ -35,11 +35,11 @@
 
 class V8TestCallback : public TestCallback, public ActiveDOMCallback {
 public:
-    static PassRefPtr<V8TestCallback> create(v8::Local<v8::Value> value, ScriptExecutionContext* context)
+    static PassRefPtr<V8TestCallback> create(v8::Handle<v8::Value> value, ScriptExecutionContext* context, v8::Handle<v8::Object> owner = v8::Handle<v8::Object>())
     {
         ASSERT(value->IsObject());
         ASSERT(context);
-        return adoptRef(new V8TestCallback(value->ToObject(), context));
+        return adoptRef(new V8TestCallback(value->ToObject(), context, owner));
     }
 
     virtual ~V8TestCallback();
@@ -55,8 +55,16 @@
     virtual bool callbackRequiresThisToPass(Class8* class8Param, ThisClass* thisClassParam);
 
 private:
-    V8TestCallback(v8::Local<v8::Object>, ScriptExecutionContext*);
+    V8TestCallback(v8::Handle<v8::Object>, ScriptExecutionContext*, v8::Handle<v8::Object>);
 
+    static void weakCallback(v8::Persistent<v8::Value> wrapper, void* parameter)
+    {
+        V8TestCallback* object = static_cast<V8TestCallback*>(parameter);
+        object->m_callback.Dispose();
+        object->m_callback.Clear();
+    }
+
+    // FIXME: m_callback should be a ScopedPersistent.
     v8::Persistent<v8::Object> m_callback;
     WorldContextHandle m_worldContext;
 };

Modified: trunk/Source/WebCore/bindings/v8/V8HiddenPropertyName.h (135304 => 135305)


--- trunk/Source/WebCore/bindings/v8/V8HiddenPropertyName.h	2012-11-20 19:58:45 UTC (rev 135304)
+++ trunk/Source/WebCore/bindings/v8/V8HiddenPropertyName.h	2012-11-20 19:59:14 UTC (rev 135305)
@@ -37,6 +37,7 @@
 
 #define V8_HIDDEN_PROPERTIES(V) \
     V(attributeListener) \
+    V(callback) \
     V(detail) \
     V(document) \
     V(domStringMap) \

Modified: trunk/Source/WebCore/bindings/v8/custom/V8MutationObserverCustom.cpp (135304 => 135305)


--- trunk/Source/WebCore/bindings/v8/custom/V8MutationObserverCustom.cpp	2012-11-20 19:58:45 UTC (rev 135304)
+++ trunk/Source/WebCore/bindings/v8/custom/V8MutationObserverCustom.cpp	2012-11-20 19:59:14 UTC (rev 135305)
@@ -61,11 +61,11 @@
         return setDOMException(TYPE_MISMATCH_ERR, args.GetIsolate());
 
     ScriptExecutionContext* context = getScriptExecutionContext();
+    v8::Handle<v8::Object> wrapper = args.Holder();
 
-    RefPtr<MutationCallback> callback = V8MutationCallback::create(arg, context);
+    RefPtr<MutationCallback> callback = V8MutationCallback::create(arg, context, wrapper);
     RefPtr<MutationObserver> observer = MutationObserver::create(callback.release());
 
-    v8::Handle<v8::Object> wrapper = args.Holder();
     V8DOMWrapper::createDOMWrapper(observer.release(), &info, wrapper);
     return wrapper;
 }
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
http://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to