Currently, we don't have one.
<rdar://problem/12954220>
Created attachment 186940 [details] Patch
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 on attachment 186940 [details] Patch Attachment 186940 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/16393470
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.
Created attachment 189179 [details] Patch
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 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 :-(
> 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.
No WebKit2 :(.
Created attachment 189378 [details] Patch
> 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 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].
Committed r143637: <http://trac.webkit.org/changeset/143637>