ObjCCallbackFunctionImpl's NSInvocation shouldn't retain its target or arguments
authormhahnenberg@apple.com <mhahnenberg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Nov 2013 22:51:52 +0000 (22:51 +0000)
committermhahnenberg@apple.com <mhahnenberg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 5 Nov 2013 22:51:52 +0000 (22:51 +0000)
https://bugs.webkit.org/show_bug.cgi?id=123822

Reviewed by Geoffrey Garen.

Using -retainArguments on ObjCCallbackFunctionImpl's NSInvocation leads to memory leaks.
We should handle retaining/releasing the target ourselves, and we should never retain the arguments.

* API/ObjCCallbackFunction.mm:
(JSC::ObjCCallbackFunctionImpl::~ObjCCallbackFunctionImpl):
(JSC::ObjCCallbackFunctionImpl::name):
(objCCallbackFunctionForInvocation):
(objCCallbackFunctionForMethod):
(objCCallbackFunctionForBlock):

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

Source/JavaScriptCore/API/ObjCCallbackFunction.mm
Source/JavaScriptCore/ChangeLog

index 2203013..a668251 100644 (file)
@@ -407,6 +407,10 @@ public:
 
     ~ObjCCallbackFunctionImpl()
     {
+        // We need to explicity release the target since we didn't call 
+        // -retainArguments on m_invocation (and we don't want to do so).
+        if (m_type == CallbackBlock || m_type == CallbackClassMethod)
+            [[m_invocation.get() target] release];
         [m_instanceClass release];
     }
 
@@ -539,6 +543,8 @@ String ObjCCallbackFunctionImpl::name()
 {
     if (m_type == CallbackInitMethod)
         return class_getName(m_instanceClass);
+    // FIXME: Maybe we could support having the selector as the name of the non-init 
+    // functions to make it a bit more user-friendly from the JS side?
     return "";
 }
 
@@ -663,7 +669,6 @@ static JSObjectRef objCCallbackFunctionForInvocation(JSContext *context, NSInvoc
     JSC::ExecState* exec = toJS([context JSGlobalContextRef]);
     JSC::APIEntryShim shim(exec);
     OwnPtr<JSC::ObjCCallbackFunctionImpl> impl = adoptPtr(new JSC::ObjCCallbackFunctionImpl(invocation, type, instanceClass, arguments.release(), result.release()));
-    // FIXME: Maybe we could support having the selector as the name of the function to make it a bit more user-friendly from the JS side?
     return toRef(JSC::ObjCCallbackFunction::create(exec->vm(), exec->lexicalGlobalObject(), impl->name(), impl.release()));
 }
 
@@ -678,8 +683,10 @@ JSObjectRef objCCallbackFunctionForMethod(JSContext *context, Class cls, Protoco
 {
     NSInvocation *invocation = [NSInvocation invocationWithMethodSignature:[NSMethodSignature signatureWithObjCTypes:types]];
     [invocation setSelector:sel];
+    // We need to retain the target Class because m_invocation doesn't retain it
+    // by default (and we don't want it to).
     if (!isInstanceMethod)
-        [invocation setTarget:cls];
+        [invocation setTarget:[cls retain]];
     return objCCallbackFunctionForInvocation(context, invocation, isInstanceMethod ? CallbackInstanceMethod : CallbackClassMethod, isInstanceMethod ? cls : nil, _protocol_getMethodTypeEncoding(protocol, sel, YES, isInstanceMethod));
 }
 
@@ -689,10 +696,13 @@ JSObjectRef objCCallbackFunctionForBlock(JSContext *context, id target)
         return 0;
     const char* signature = _Block_signature(target);
     NSInvocation *invocation = [NSInvocation invocationWithMethodSignature:[NSMethodSignature signatureWithObjCTypes:signature]];
-    [invocation retainArguments];
-    id targetCopy = [target copy];
-    [invocation setTarget:targetCopy];
-    [targetCopy release];
+
+    // We don't want to use -retainArguments because that leaks memory. Arguments 
+    // would be retained indefinitely between invocations of the callback.
+    // Additionally, we copy the target because we want the block to stick around
+    // until the ObjCCallbackFunctionImpl is destroyed.
+    [invocation setTarget:[target copy]];
+
     return objCCallbackFunctionForInvocation(context, invocation, CallbackBlock, nil, signature);
 }
 
index 751d199..32e7dd5 100644 (file)
@@ -1,3 +1,20 @@
+2013-11-05  Mark Hahnenberg  <mhahnenberg@apple.com>
+
+        ObjCCallbackFunctionImpl's NSInvocation shouldn't retain its target or arguments
+        https://bugs.webkit.org/show_bug.cgi?id=123822
+
+        Reviewed by Geoffrey Garen.
+
+        Using -retainArguments on ObjCCallbackFunctionImpl's NSInvocation leads to memory leaks.
+        We should handle retaining/releasing the target ourselves, and we should never retain the arguments.
+
+        * API/ObjCCallbackFunction.mm:
+        (JSC::ObjCCallbackFunctionImpl::~ObjCCallbackFunctionImpl):
+        (JSC::ObjCCallbackFunctionImpl::name):
+        (objCCallbackFunctionForInvocation):
+        (objCCallbackFunctionForMethod):
+        (objCCallbackFunctionForBlock):
+
 2013-11-05  Julien Brianceau  <jbriance@cisco.com>
 
         Fix build for architectures with 4 argument registers (broken since r158681).