Bug 123380

Summary: JSExport doesn't support constructors
Product: WebKit Reporter: Mark Hahnenberg <mhahnenberg>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: ggaren
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch ggaren: review+

Description Mark Hahnenberg 2013-10-25 18:25:36 PDT
Support for constructor-style callbacks for the Objective-C API to JSC is currently limited to Objective-C blocks. Any clients who try to call the constructor of a JSExport-ed Objective-C class are met with a type error stating that it cannot be called as a constructor.

It would be nice to expand JSExport's functionality to support this idiom. It is a natural extension to JSExport and would increase the expressiveness and simplicity in both Objective-C and JavaScript client code.

The way we'll do this is to expand the capabilities of ObjCCallbackFunction and associated classes. Instead of constructing a normal C API object for the constructor, we'll instead allocate a full-blown ObjCCallbackFunction object which can already properly handle being invoked as a constructor.
Comment 1 Mark Hahnenberg 2013-10-25 18:34:30 PDT
Created attachment 215235 [details]
Patch
Comment 2 Geoffrey Garen 2013-10-26 16:19:27 PDT
Comment on attachment 215235 [details]
Patch

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

> Source/JavaScriptCore/API/JSWrapperMap.mm:167
> +        // Don't copy over init* methods because we handle those specially 

Let's say "init family" instead of "init*", since there's a term of art for it.

> Source/JavaScriptCore/API/JSWrapperMap.mm:341
> +    __block HashMap<String, Protocol *> initTable;

This is a dangerous use of __block with C++. If any of our blocks are copied, they will copy initTable, instead of sharing it:

-----
<https://developer.apple.com/library/ios/documentation/cocoa/conceptual/Blocks/Articles/bxVariables.html>

C++ Objects
In general you can use C++ objects within a block. Within a member function, references to member variables and functions are via an implicitly imported this pointer and thus appear mutable. There are two considerations that apply if a block is copied:

If you have a __block storage class for what would have been a stack-based C++ object, then the usual copy constructor is used.
If you use any other C++ stack-based object from within a block, it must have a const copy constructor. The C++ object is then copied using that constructor.
-----

I guess this usage ends up succeeding because forEachProtocolImplementingProtocol doesn't copy the passed-in block. I wonder what the best practice is in a case like this.

> Source/JavaScriptCore/API/JSWrapperMap.mm:344
> +    forEachProtocolImplementingProtocol(cls, exportProtocol, ^(Protocol *protocol){
> +        forEachMethodInProtocol(protocol, YES, YES, ^(SEL selector, const char*){

Need spaces before "{".

> Source/JavaScriptCore/API/JSWrapperMap.mm:377
> +            // FIXME: Do we need to log here?
> +            NSLog(@"Too many inits found, abandoning ship.");

Let's log, but try to write something that will be meaningful to an app developer: "ERROR: Class @{class} exported more than one init family method through JSExport. Class @{class} will not have a callable constructor function."

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:589
>      [m_invocation invoke];

This is a bug in the init case because it doesn't account for init releasing the allocated object and returning a different object in its place (or nil). You should fix this and add a test for it. c.f. <https://developer.apple.com/library/ios/releasenotes/ObjectiveC/RN-TransitioningToARC/Introduction/Introduction.html>.

> Source/JavaScriptCore/API/ObjCCallbackFunction.mm:591
> +    // After calling init we need to autorelease.

This comment could say more: "Balance our call to -alloc with a call to -autorelease. We have to do this after calling -init because init family methods are allowed to release the allocated object and return something else in its place."
Comment 3 Geoffrey Garen 2013-10-26 16:56:21 PDT
> This is a dangerous use of __block with C++. If any of our blocks are copied, they will copy initTable, instead of sharing it:

Actually, that's only half true: block copy will copy initTable, but all blocks will point to the copy, so I guess we're cool.
Comment 4 Mark Hahnenberg 2013-10-28 16:45:10 PDT
Created attachment 215345 [details]
Patch
Comment 5 Geoffrey Garen 2013-10-29 11:38:57 PDT
Comment on attachment 215345 [details]
Patch

Should we add some linked-on-or-after action here?
Comment 6 Mark Hahnenberg 2013-10-29 12:04:50 PDT
(In reply to comment #5)
> (From update of attachment 215345 [details])
> Should we add some linked-on-or-after action here?

Sure. Right now it seems like the shared VM stuff uses the webkit version to decide when it was linked. I'm not sure which WebKit version to use...
Comment 7 Mark Hahnenberg 2013-10-30 10:56:30 PDT
Committed r158286: <http://trac.webkit.org/changeset/158286>
Comment 8 Mark Hahnenberg 2013-10-30 10:59:18 PDT
<rdar://problem/15147348>
Comment 9 Mark Hahnenberg 2013-10-30 11:20:46 PDT
Reopening because I forgot part of the linked-on-or-after stuff.
Comment 10 Mark Hahnenberg 2013-10-30 11:25:00 PDT
Created attachment 215536 [details]
Patch
Comment 11 Darin Adler 2013-10-31 13:59:41 PDT
Comment on attachment 215536 [details]
Patch

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

> Source/JavaScriptCore/API/JSWrapperMap.mm:162
> +static bool shouldSkipMethodWithName(NSString *name)

inline?

> Source/JavaScriptCore/API/JSWrapperMap.mm:169
> +#if OS(DARWIN)
> +    if (NSVersionOfLinkTimeLibrary("JavaScriptCore") < webkitFirstVersionWithInitConstructorSupport)
> +        return false;
> +#endif

Is this fast enough to re-evaluate every time, or should we cache it in a global?

> Source/JavaScriptCore/API/JSWrapperMap.mm:173
> +    // Skip over init family methods because we handle those specially 
> +    // for the purposes of hooking up the constructor correctly.
> +    return [name hasPrefix:@"init"];

Is this the correct definition for “init family”? It would return true for a property named “initials” or “initialCharacter” or “initialSize”.
Comment 12 Mark Hahnenberg 2013-11-06 10:52:37 PST
Created attachment 216193 [details]
Patch
Comment 13 Mark Hahnenberg 2013-11-06 10:53:01 PST
(In reply to comment #12)
> Created an attachment (id=216193) [details]
> Patch

I changed the patch significantly enough to warrant another review I think.
Comment 14 Geoffrey Garen 2013-11-06 11:13:08 PST
Comment on attachment 216193 [details]
Patch

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

r=me

> Source/JavaScriptCore/API/JSWrapperMap.mm:623
> +#if OS(DARWIN)

I think it's fair to assume that all ObjC OS's are DARWIN. So, you can remove this #ifdef.
Comment 15 Mark Hahnenberg 2013-11-06 11:19:53 PST
Committed r158762: <http://trac.webkit.org/changeset/158762>