From f61a46a837e49d0e52e9a286a85d36a6dc7bbdce Mon Sep 17 00:00:00 2001 From: "commit-queue@webkit.org" Date: Tue, 20 Nov 2012 19:59:14 +0000 Subject: [PATCH] Store MutationObserver callback in a hidden property for V8 https://bugs.webkit.org/show_bug.cgi?id=102555 Patch by Elliott Sprehn 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): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@135305 268f45cc-cd09-0410-ab3c-d52691b4dbfc --- ChangeLog | 13 ++++++++++ ManualTests/leak-cycle-observer-wrapper.html | 18 +++++++++++++ Source/WebCore/ChangeLog | 25 +++++++++++++++++++ .../bindings/scripts/CodeGeneratorV8.pm | 23 +++++++++++++---- .../scripts/test/V8/V8TestCallback.cpp | 9 +++++-- .../bindings/scripts/test/V8/V8TestCallback.h | 14 ++++++++--- .../bindings/v8/V8HiddenPropertyName.h | 1 + .../v8/custom/V8MutationObserverCustom.cpp | 4 +-- 8 files changed, 95 insertions(+), 12 deletions(-) create mode 100644 ManualTests/leak-cycle-observer-wrapper.html diff --git a/ChangeLog b/ChangeLog index e267b5fa8f0..c37b583969b 100644 --- a/ChangeLog +++ b/ChangeLog @@ -1,3 +1,16 @@ +2012-11-20 Elliott Sprehn + + 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 Unreviewed. Update NEWS and configure.ac for 1.11.2 release diff --git a/ManualTests/leak-cycle-observer-wrapper.html b/ManualTests/leak-cycle-observer-wrapper.html new file mode 100644 index 00000000000..e813f37bd06 --- /dev/null +++ b/ManualTests/leak-cycle-observer-wrapper.html @@ -0,0 +1,18 @@ + + +

+ Tests that reference cycles between the observer and the callback do not + create leaks. +

+ + diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog index c005dbc839a..f84810eea43 100644 --- a/Source/WebCore/ChangeLog +++ b/Source/WebCore/ChangeLog @@ -1,3 +1,28 @@ +2012-11-20 Elliott Sprehn + + 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 Crash in FrameLoader::stopLoading. diff --git a/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm b/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm index 7e453beab1d..086b27852c9 100644 --- a/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm +++ b/Source/WebCore/bindings/scripts/CodeGeneratorV8.pm @@ -3246,11 +3246,11 @@ sub GenerateCallbackHeader push(@headerContent, < create(v8::Local value, ScriptExecutionContext* context) + static PassRefPtr<${v8InterfaceName}> create(v8::Handle value, ScriptExecutionContext* context, v8::Handle owner = v8::Handle()) { 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 @@ END push(@headerContent, <, ScriptExecutionContext*); + ${v8InterfaceName}(v8::Handle, ScriptExecutionContext*, v8::Handle); + static void weakCallback(v8::Persistent 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 m_callback; WorldContextHandle m_worldContext; }; @@ -3314,16 +3322,21 @@ sub GenerateCallbackImplementation push(@implContent, "#include \n\n"); push(@implContent, "namespace WebCore {\n\n"); push(@implContent, < callback, ScriptExecutionContext* context) +${v8InterfaceName}::${v8InterfaceName}(v8::Handle callback, ScriptExecutionContext* context, v8::Handle owner) : ActiveDOMCallback(context) , m_callback(v8::Persistent::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 diff --git a/Source/WebCore/bindings/scripts/test/V8/V8TestCallback.cpp b/Source/WebCore/bindings/scripts/test/V8/V8TestCallback.cpp index ed2de6ee6bb..8fa4845eda2 100644 --- a/Source/WebCore/bindings/scripts/test/V8/V8TestCallback.cpp +++ b/Source/WebCore/bindings/scripts/test/V8/V8TestCallback.cpp @@ -39,16 +39,21 @@ namespace WebCore { -V8TestCallback::V8TestCallback(v8::Local callback, ScriptExecutionContext* context) +V8TestCallback::V8TestCallback(v8::Handle callback, ScriptExecutionContext* context, v8::Handle owner) : ActiveDOMCallback(context) , m_callback(v8::Persistent::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 diff --git a/Source/WebCore/bindings/scripts/test/V8/V8TestCallback.h b/Source/WebCore/bindings/scripts/test/V8/V8TestCallback.h index 31ca7e1776a..e854c3d92ac 100644 --- a/Source/WebCore/bindings/scripts/test/V8/V8TestCallback.h +++ b/Source/WebCore/bindings/scripts/test/V8/V8TestCallback.h @@ -35,11 +35,11 @@ class ScriptExecutionContext; class V8TestCallback : public TestCallback, public ActiveDOMCallback { public: - static PassRefPtr create(v8::Local value, ScriptExecutionContext* context) + static PassRefPtr create(v8::Handle value, ScriptExecutionContext* context, v8::Handle owner = v8::Handle()) { 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 @@ class V8TestCallback : public TestCallback, public ActiveDOMCallback { virtual bool callbackRequiresThisToPass(Class8* class8Param, ThisClass* thisClassParam); private: - V8TestCallback(v8::Local, ScriptExecutionContext*); + V8TestCallback(v8::Handle, ScriptExecutionContext*, v8::Handle); + static void weakCallback(v8::Persistent wrapper, void* parameter) + { + V8TestCallback* object = static_cast(parameter); + object->m_callback.Dispose(); + object->m_callback.Clear(); + } + + // FIXME: m_callback should be a ScopedPersistent. v8::Persistent m_callback; WorldContextHandle m_worldContext; }; diff --git a/Source/WebCore/bindings/v8/V8HiddenPropertyName.h b/Source/WebCore/bindings/v8/V8HiddenPropertyName.h index 2f379d09f23..c16ce356fcb 100644 --- a/Source/WebCore/bindings/v8/V8HiddenPropertyName.h +++ b/Source/WebCore/bindings/v8/V8HiddenPropertyName.h @@ -37,6 +37,7 @@ namespace WebCore { #define V8_HIDDEN_PROPERTIES(V) \ V(attributeListener) \ + V(callback) \ V(detail) \ V(document) \ V(domStringMap) \ diff --git a/Source/WebCore/bindings/v8/custom/V8MutationObserverCustom.cpp b/Source/WebCore/bindings/v8/custom/V8MutationObserverCustom.cpp index f80ce598a97..9690889a489 100644 --- a/Source/WebCore/bindings/v8/custom/V8MutationObserverCustom.cpp +++ b/Source/WebCore/bindings/v8/custom/V8MutationObserverCustom.cpp @@ -61,11 +61,11 @@ v8::Handle V8MutationObserver::constructorCallback(const v8::Argument return setDOMException(TYPE_MISMATCH_ERR, args.GetIsolate()); ScriptExecutionContext* context = getScriptExecutionContext(); + v8::Handle wrapper = args.Holder(); - RefPtr callback = V8MutationCallback::create(arg, context); + RefPtr callback = V8MutationCallback::create(arg, context, wrapper); RefPtr observer = MutationObserver::create(callback.release()); - v8::Handle wrapper = args.Holder(); V8DOMWrapper::createDOMWrapper(observer.release(), &info, wrapper); return wrapper; }