Bug 146416 – WKWebView evaluateJavaScript:completionHandler: should reuse its JSContext instance

• Component: WebKit2
• Status: RESOLVED FIXED

[Mihai Parparita]

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.

[Mihai Parparita]

Mark, any chance you could look at this, since it's very similar to the work you did for bug 146358?

[Mark Lam]

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

[Mark Lam]

Created attachment 255949 [details]
the patch.

[WebKit Commit Bot]

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

[Mark Lam]

<rdar://problem/17956460>

[Mark Lam]

Created attachment 255952 [details]
patch 2: fixed missing semi-colon

[Geoffrey Garen]

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?

[Mark Lam]

Created attachment 255982 [details]
patch 3: applied Geoff's feedback

[Mark Lam]

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

[Anders Carlsson]

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.

[Mark Lam]

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

[Chris Dumez]

(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

[Chris Dumez]

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.

[Anders Carlsson]

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

[Mark Lam]

Fix for 32-bit build landed in r186243: <http://trac.webkit.org/r186243>.

[Simon Taylor]

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.