Changeset 158695 in webkit for trunk/Source/JavaScriptCore/API/ObjCCallbackFunction.mm

Timestamp:

Nov 5, 2013, 2:51:52 PM (12 years ago)

Author:

[email protected]

Message:

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):

File:

• trunk/Source/JavaScriptCore/API/ObjCCallbackFunction.mm (modified) (5 diffs)

trunk/Source/JavaScriptCore/API/ObjCCallbackFunction.mm

@@ -408 +408 @@
     ~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];
     }
@@ -540 +544 @@
     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 "";
 }
@@ -664 +670 @@
     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()));
 }
@@ -679 +684 @@
     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));
 }
@@ -690 +697 @@
     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);
 }