Bug 123380 - JSExport doesn't support constructors
Summary: JSExport doesn't support constructors
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
Depends on:
Reported: 2013-10-25 18:25 PDT by Mark Hahnenberg
Modified: 2013-11-06 11:19 PST (History)
1 user (show)

See Also:

Patch (17.87 KB, patch)
2013-10-25 18:34 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (19.25 KB, patch)
2013-10-28 16:45 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (2.41 KB, patch)
2013-10-30 11:25 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (8.65 KB, patch)
2013-11-06 10:52 PST, Mark Hahnenberg
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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]
Comment 2 Geoffrey Garen 2013-10-26 16:19:27 PDT
Comment on attachment 215235 [details]

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:


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]
Comment 5 Geoffrey Garen 2013-10-29 11:38:57 PDT
Comment on attachment 215345 [details]

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
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]
Comment 11 Darin Adler 2013-10-31 13:59:41 PDT
Comment on attachment 215536 [details]

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

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


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

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


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