Bug 106059 – Objective-C API: Need a way to use the Objective-C JavaScript API with WebKit

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Geoffrey Garen]

Currently, we don't have one.

[Mark Hahnenberg]

<rdar://problem/12954220>

[Mark Hahnenberg]

Created attachment 186940 [details]
Patch

[Build Bot]

Comment on attachment 186940 [details]
Patch

Attachment 186940 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16395442

[Build Bot]

Comment on attachment 186940 [details]
Patch

Attachment 186940 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16393470

[Gavin Barraclough]

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.

[Mark Hahnenberg]

Created attachment 189179 [details]
Patch

[Geoffrey Garen]

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.

[Mark Hahnenberg]

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

[Geoffrey Garen]

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

[Sam Weinig]

No WebKit2 :(.

[Mark Hahnenberg]

Created attachment 189378 [details]
Patch

[Geoffrey Garen]

> I should have been clearer: you need "[[x retain] autorelease]", not "[x autorelease]". I don't think the table autoreleases.

Correction: the table autoreleases.

[Geoffrey Garen]

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

[Mark Hahnenberg]

Committed r143637: <http://trac.webkit.org/changeset/143637>