Bug 107836

Summary: Objective-C API: Passing JS functions to Objective-C callbacks causes JSValue to leak
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: ggaren, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
oliver: review+
patch none

Description Mark Hahnenberg 2013-01-24 09:20:22 PST
To fix the immediate problem, the block allocated by __NSMakeSpecialForwardingCaptureBlock in JSBlockAdaptor.mm should be autoreleased.

There's another slight complication with this bit of code. From Gavin:

"If the forwarding block retains the JS function, it will be keeping the global object alive.  If there is any reference from the global object to a wrapper to an objective C object [that] retained the forwarding block, we'd have a cycle."

Fixing this will require a bit more thought.
Comment 1 Radar WebKit Bug Importer 2013-01-24 11:47:38 PST
<rdar://problem/13079708>
Comment 2 Mark Hahnenberg 2013-02-21 18:35:46 PST
We've decided to remove support for this feature from the API because there's no way to automatically mange the memory for clients in a satisfactory manner. Clients can still pass JS functions to Objective-C methods, but the methods must accept plain JSValues instead of Objective-C blocks.
Comment 3 Mark Hahnenberg 2013-02-21 18:51:28 PST
Created attachment 189659 [details]
Patch
Comment 4 Oliver Hunt 2013-02-21 19:14:32 PST
Comment on attachment 189659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189659&action=review

> Source/JavaScriptCore/ChangeLog:9
> +        mange the memory for clients in a satisfactory manner. Clients can still pass JS functions to Objective-C 

mange = manage?
Comment 5 Mark Hahnenberg 2013-02-21 19:15:39 PST
> mange = manage?

 Right you are.
Comment 6 Geoffrey Garen 2013-02-21 21:37:25 PST
Comment on attachment 189659 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=189659&action=review

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:219
> -        id block = [m_adaptor blockFromValue:argument inContext:context withException:exception];
> -        [invocation setArgument:&block atIndex:argumentNumber];
> +        NSLog(@"Passing JS functions as blocks to Objective-C methods is not supported.");
> +        CRASH();

This seems wrong. We should just do whatever we usually do when an argument type is not supported. I believe that means returning nil from parseObjCType.
Comment 7 Mark Hahnenberg 2013-02-22 10:51:08 PST
> This seems wrong. We should just do whatever we usually do when an argument type is not supported. I believe that means returning nil from parseObjCType.

I think the issue with returning nil from parseObjCType is that we do support blocks in the API, just not auto-wrapping JS functions to be passed as blocks. I believe parseObjCType is used in more places than just for this auto-wrapping feature (e.g. we support returning blocks from Obj-C methods that can be called from JS code).
Comment 8 Geoffrey Garen 2013-02-22 14:28:19 PST
Got it. The behavior we want here is to do whatever we do for any other exported type that isn't bridgeable -- for example, what do we do for a function pointer, or an ObjC class we don't recognize?
Comment 9 Mark Hahnenberg 2013-03-01 12:06:38 PST
Created attachment 191004 [details]
patch
Comment 10 WebKit Review Bot 2013-03-01 13:16:24 PST
Comment on attachment 191004 [details]
patch

Clearing flags on attachment: 191004

Committed r144489: <http://trac.webkit.org/changeset/144489>
Comment 11 WebKit Review Bot 2013-03-01 13:16:29 PST
All reviewed patches have been landed.  Closing bug.