Summary: | WKWebView evaluateJavaScript:completionHandler: should reuse its JSContext instance | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mihai Parparita <mihaip> | ||||||||
Component: | WebKit2 | Assignee: | 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
Mihai Parparita
2015-06-29 12:01:24 PDT
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 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. |