Bug 146416

Summary: WKWebView evaluateJavaScript:completionHandler: should reuse its JSContext instance
Product: WebKit Reporter: Mihai Parparita <mihaip>
Component: WebKit2Assignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, berto, cdumez, cgarcia, commit-queue, gustavo, mark.lam, mcatanzaro, mrobinson, simontaylor1, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=210920
Attachments:
Description Flags
the patch.
none
patch 2: fixed missing semi-colon
none
patch 3: applied Geoff's feedback andersca: review+

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>.
Comment 16 Simon Taylor 2020-04-23 10:20:09 PDT
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.