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

Mark Hahnenberg
Reported 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.
Attachments
Patch (5.91 KB, patch)
2013-02-21 18:51 PST, Mark Hahnenberg
oliver: review+
patch (25.52 KB, patch)
2013-03-01 12:06 PST, Mark Hahnenberg
no flags
Radar WebKit Bug Importer
Comment 1 2013-01-24 11:47:38 PST
Mark Hahnenberg
Comment 2 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.
Mark Hahnenberg
Comment 3 2013-02-21 18:51:28 PST
Oliver Hunt
Comment 4 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?
Mark Hahnenberg
Comment 5 2013-02-21 19:15:39 PST
> mange = manage? Right you are.
Geoffrey Garen
Comment 6 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.
Mark Hahnenberg
Comment 7 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).
Geoffrey Garen
Comment 8 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?
Mark Hahnenberg
Comment 9 2013-03-01 12:06:38 PST
WebKit Review Bot
Comment 10 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>
WebKit Review Bot
Comment 11 2013-03-01 13:16:29 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.