Similar to the fix made in bug 146358, evaluateJavaScript:completionHandler: in WKWebView creates a new JSContext every time it parses the return value (see http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm#L640). This is inefficient and could benefit from the same caching added by http://trac.webkit.org/changeset/186010 I reported this via Radar in rdar://17956460 during the iOS 8 beta period.
Mark, any chance you could look at this, since it's very similar to the work you did for bug 146358?
(In reply to comment #1) > Mark, any chance you could look at this, since it's very similar to the work > you did for bug 146358? Will take a look.
Created attachment 255949 [details] the patch.
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
<rdar://problem/17956460>
Created attachment 255952 [details] patch 2: fixed missing semi-colon
Comment on attachment 255952 [details] patch 2: fixed missing semi-colon View in context: https://bugs.webkit.org/attachment.cgi?id=255952&action=review > Source/WebKit2/UIProcess/API/Cocoa/APISerializedScriptValueCocoa.mm:38 > + static JSContext* context = [[JSContext alloc] init]; I don't think it's OK to leak this context forever. Instead, you should use a timer to cache it for a short amount of time and then destroy it. > Source/WebKit2/UIProcess/API/Cocoa/APISerializedScriptValueCocoa.mm:42 > + if (exception && *exception) > + return nil; What happens in the case where our caller pass a null exception out parameter? In that case, we have no way to check for exceptions, which seems bad. Can we check valueRef instead, or use some other strategy that allow us to check for exceptions even if exception is null?
Created attachment 255982 [details] patch 3: applied Geoff's feedback
(In reply to comment #8) > Created attachment 255982 [details] > patch 3: applied Geoff's feedback For the record, the Win EWS failure is unrelated to this patch.
Comment on attachment 255982 [details] patch 3: applied Geoff's feedback View in context: https://bugs.webkit.org/attachment.cgi?id=255982&action=review > Source/WebKit2/UIProcess/API/Cocoa/APISerializedScriptValueCocoa.mm:65 > + ASSERT(isUIThread()); I think asserting RunLoop::isMain() is better here.
(In reply to comment #10) > > Source/WebKit2/UIProcess/API/Cocoa/APISerializedScriptValueCocoa.mm:65 > > + ASSERT(isUIThread()); > > I think asserting RunLoop::isMain() is better here. I've applied this change, and re-tested. Landed in r186229: <http://trac.webkit.org/r186229>.
(In reply to comment #11) > (In reply to comment #10) > > > Source/WebKit2/UIProcess/API/Cocoa/APISerializedScriptValueCocoa.mm:65 > > > + ASSERT(isUIThread()); > > > > I think asserting RunLoop::isMain() is better here. > > I've applied this change, and re-tested. Landed in r186229: > <http://trac.webkit.org/r186229>. Broke the build: https://build.webkit.org/builders/Apple%20Yosemite%20Release%20%2832-bit%20Build%29/builds/4205/steps/compile-webkit/logs/stdio
Comment on attachment 255982 [details] patch 3: applied Geoff's feedback View in context: https://bugs.webkit.org/attachment.cgi?id=255982&action=review > Source/WebKit2/UIProcess/API/Cocoa/APISerializedScriptValueCocoa.mm:44 > + JSContext* ensureContext() Isn't JSContext in JSC namespace? > Source/WebKit2/UIProcess/API/Cocoa/APISerializedScriptValueCocoa.mm:73 > + JSValue *value = [JSValue valueWithJSValueRef:valueRef inContext:context]; ditto for JSValue.
(In reply to comment #13) > Comment on attachment 255982 [details] > patch 3: applied Geoff's feedback > > View in context: > https://bugs.webkit.org/attachment.cgi?id=255982&action=review > > > Source/WebKit2/UIProcess/API/Cocoa/APISerializedScriptValueCocoa.mm:44 > > + JSContext* ensureContext() > > Isn't JSContext in JSC namespace? > > > Source/WebKit2/UIProcess/API/Cocoa/APISerializedScriptValueCocoa.mm:73 > > + JSValue *value = [JSValue valueWithJSValueRef:valueRef inContext:context]; > > ditto for JSValue. No, these are the Objective-C types.
Fix for 32-bit build landed in r186243: <http://trac.webkit.org/r186243>.
I've added bug 210920 to request some further improvements here - the shared JSContext used for deserialization is only cached for 1 second from creation, which is non-optimal for heavier users of these APIs and causes some annoyances for debugging.