Bug 146416 - WKWebView evaluateJavaScript:completionHandler: should reuse its JSContext instance
Summary: WKWebView evaluateJavaScript:completionHandler: should reuse its JSContext in...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2015-06-29 12:01 PDT by Mihai Parparita
Modified: 2015-07-02 16:34 PDT (History)
10 users (show)

See Also:


Attachments
the patch. (11.41 KB, patch)
2015-07-01 13:39 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
patch 2: fixed missing semi-colon (11.41 KB, patch)
2015-07-01 14:17 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
patch 3: applied Geoff's feedback (12.20 KB, patch)
2015-07-01 19:17 PDT, Mark Lam
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mihai Parparita 2015-06-29 12:01:24 PDT
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.
Comment 1 Mihai Parparita 2015-06-29 12:02:27 PDT
Mark, any chance you could look at this, since it's very similar to the work you did for bug 146358?
Comment 2 Mark Lam 2015-06-29 12:09:16 PDT
(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.
Comment 3 Mark Lam 2015-07-01 13:39:04 PDT
Created attachment 255949 [details]
the patch.
Comment 4 WebKit Commit Bot 2015-07-01 13:41:27 PDT
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
Comment 5 Mark Lam 2015-07-01 13:45:03 PDT
<rdar://problem/17956460>
Comment 6 Mark Lam 2015-07-01 14:17:28 PDT
Created attachment 255952 [details]
patch 2: fixed missing semi-colon
Comment 7 Geoffrey Garen 2015-07-01 14:41:39 PDT
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?
Comment 8 Mark Lam 2015-07-01 19:17:03 PDT
Created attachment 255982 [details]
patch 3: applied Geoff's feedback
Comment 9 Mark Lam 2015-07-02 10:39:32 PDT
(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 10 Anders Carlsson 2015-07-02 10:48:36 PDT
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.
Comment 11 Mark Lam 2015-07-02 11:40:30 PDT
(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>.
Comment 12 Chris Dumez 2015-07-02 15:01:26 PDT
(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 13 Chris Dumez 2015-07-02 15:10:57 PDT
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.
Comment 14 Anders Carlsson 2015-07-02 15:20:17 PDT
(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.
Comment 15 Mark Lam 2015-07-02 16:34:06 PDT
Fix for 32-bit build landed in r186243: <http://trac.webkit.org/r186243>.