Objective-C API: Passing JS functions to Objective-C callbacks causes JSValue to leak

[Mark Hahnenberg]

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.

[Radar WebKit Bug Importer]

<rdar://problem/13079708>

[Mark Hahnenberg]

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]

Created attachment 189659 [details]
Patch

[Oliver Hunt]

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]

> mange = manage?

 Right you are.

[Geoffrey Garen]

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]

> 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]

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]

Created attachment 191004 [details]
patch

[WebKit Review Bot]

Comment on attachment 191004 [details]
patch

Clearing flags on attachment: 191004

Committed r144489: <http://trac.webkit.org/changeset/144489>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.