Bug 123380 – JSExport doesn't support constructors

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Hahnenberg]

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.

[Mark Hahnenberg]

Created attachment 215235 [details]
Patch

[Geoffrey Garen]

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."

[Geoffrey Garen]

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

[Mark Hahnenberg]

Created attachment 215345 [details]
Patch

[Geoffrey Garen]

Comment on attachment 215345 [details]
Patch

Should we add some linked-on-or-after action here?

[Mark Hahnenberg]

(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...

[Mark Hahnenberg]

Committed r158286: <http://trac.webkit.org/changeset/158286>

[Mark Hahnenberg]

<rdar://problem/15147348>

[Mark Hahnenberg]

Reopening because I forgot part of the linked-on-or-after stuff.

[Mark Hahnenberg]

Created attachment 215536 [details]
Patch

[Darin Adler]

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”.

[Mark Hahnenberg]

Created attachment 216193 [details]
Patch

[Mark Hahnenberg]

(In reply to comment #12)
> Created an attachment (id=216193) [details]
> Patch

I changed the patch significantly enough to warrant another review I think.

[Geoffrey Garen]

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.

[Mark Hahnenberg]

Committed r158762: <http://trac.webkit.org/changeset/158762>