Bug 106059

Summary: Objective-C API: Need a way to use the Objective-C JavaScript API with WebKit
Product: WebKit Reporter: Geoffrey Garen <ggaren>
Component: JavaScriptCoreAssignee: Mark Hahnenberg <mhahnenberg>
Status: RESOLVED FIXED    
Severity: Normal CC: barraclough, benjamin, buildbot, cmarcelo, ggaren, levin+threading, mhahnenberg, ojan.autocc, philn, rniwa, sam, webkit.review.bot, xan.lopez
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch ggaren: review+

Description Geoffrey Garen 2013-01-03 15:53:37 PST
Currently, we don't have one.
Comment 1 Mark Hahnenberg 2013-02-06 15:36:01 PST
<rdar://problem/12954220>
Comment 2 Mark Hahnenberg 2013-02-06 16:18:21 PST
Created attachment 186940 [details]
Patch
Comment 3 Build Bot 2013-02-06 16:51:09 PST
Comment on attachment 186940 [details]
Patch

Attachment 186940 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16395442
Comment 4 Build Bot 2013-02-06 16:53:51 PST
Comment on attachment 186940 [details]
Patch

Attachment 186940 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16393470
Comment 5 Gavin Barraclough 2013-02-06 18:00:48 PST
Hey Mark, I don't think ScriptController::javaScriptContext() is doing the right thing – this is creating a new context (i.e. new global object) whereas this should be returning a JSContext wrapping the frame's current global object.  As such I think that rather than:
    [[JSContext alloc] initWithVirtualMachine:virtualMachineFromContextGroup(toRef(&root->globalObject()->globalData()))];
you want something more like:
    [[JSContext alloc] initWithGlobalObject:root->globalObject()];

Also, the ScriptController persists across navigation, so the reference to m_javaScriptContext needs to be cleared on page navigation.  It would probably be best to add a copy of the value to the back/forwards cache, so that the same JSContext persists until WebCore drops the global object.

contextFromGlobalContextRef and virtualMachineFromContextGroup both just return m_apiData, which means that they'll return nil for objects that weren't created via the API, but we want the API to be able to work on WebCore's global objects in WebCore's globaldata – so I think we need these methods to be able to lazily create objective-C wrappers around the existing objects, if these don't yet exist.

I think we really need to write a test case for this code.
Comment 6 Mark Hahnenberg 2013-02-19 15:13:30 PST
Created attachment 189179 [details]
Patch
Comment 7 Geoffrey Garen 2013-02-19 16:01:36 PST
Comment on attachment 189179 [details]
Patch

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

r=me

> Source/JavaScriptCore/API/JSContextInternal.h:75
> ++ (JSContext *)contextFromGlobalContextRef:(JSGlobalContextRef)globalContext;

Minor naming comment: Usually these class factory methods have the same name as their related designated initializer. In this case, that's "contextWithGlobalContextRef".

> Source/JavaScriptCore/API/JSObjCWrapperCache.mm:38
> +    DEFINE_STATIC_LOCAL(Mutex, mutex, ());

This initialization is not thread-safe. To my shock and horror, there are 17 other bugs like this in WebKit. I guess threads are indeed evil. I guess I will not make you fix this, since our whole library is built on this shaky foundation, and we'll have to fix it later some other way.

> Source/JavaScriptCore/API/JSObjCWrapperCache.mm:65
> +    return static_cast<id>(NSMapGet(wrapperCache, object));

This should autorelease to keep up the contract that our +factory methods return autoreleased values.

> Source/JavaScriptCore/API/JSVirtualMachineInternal.h:38
> +JS_EXPORT_PRIVATE JSVirtualMachine *virtualMachineFromContextGroupRef(JSContextGroupRef);

Let's use the class factory method pattern here like you did in JSContext. It's the Cocoa way.
Comment 8 Mark Hahnenberg 2013-02-19 16:41:42 PST
Comment on attachment 189179 [details]
Patch

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

>> Source/JavaScriptCore/API/JSObjCWrapperCache.mm:65
>> +    return static_cast<id>(NSMapGet(wrapperCache, object));
> 
> This should autorelease to keep up the contract that our +factory methods return autoreleased values.

I believe that NSMapTable autoreleases for us. I tried adding a call to autorelease here and it caused crashing :-(
Comment 9 Geoffrey Garen 2013-02-19 18:16:42 PST
> I believe that NSMapTable autoreleases for us. I tried adding a call to autorelease here and it caused crashing :-(

I should have been clearer: you need "[[x retain] autorelease]", not "[x autorelease]". I don't think the table autoreleases.
Comment 10 Sam Weinig 2013-02-19 22:31:32 PST
No WebKit2 :(.
Comment 11 Mark Hahnenberg 2013-02-20 14:25:13 PST
Created attachment 189378 [details]
Patch
Comment 12 Geoffrey Garen 2013-02-21 09:10:05 PST
> I should have been clearer: you need "[[x retain] autorelease]", not "[x autorelease]". I don't think the table autoreleases.

Correction: the table autoreleases.
Comment 13 Geoffrey Garen 2013-02-21 09:19:39 PST
Comment on attachment 189378 [details]
Patch

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

r=me, with some comments

> Source/JavaScriptCore/API/JSContext.mm:174
> +- (id)initWithGlobalContextRef:(JSGlobalContextRef)context

I think we should name this "initWithJSGlobalContextRef" or name the other thingy "globalContextRef". But they should be consistent.

> Source/JavaScriptCore/API/JSVirtualMachine.mm:62
> +    if (!wrapperCache)
> +        initWrapperCache();

Would be nice to have a wrapperCache() helper function that did the null check for us.

> Source/JavaScriptCore/API/JSVirtualMachine.mm:87
> +    // The extra JSContextGroupRetain is balanced here.

I think you mean that the JSContextGroupCreate is balanced here.

> Tools/TestWebKitAPI/Tests/mac/WebViewDidCreateJavaScriptContext.mm:107
> +    __block JSContext *tempContext = context;
> +    context[@"insertMyCustomProperty"] = ^(JSValue *element) {
> +        JSValue *fortyTwo = [JSValue valueWithInt32:42 inContext:tempContext];

This is a memory leak because the block retains the context, which has a property retaining the block. The right way to get a context inside a block callback is [JSContext currentContext].
Comment 14 Mark Hahnenberg 2013-02-21 12:02:20 PST
Committed r143637: <http://trac.webkit.org/changeset/143637>