More ScriptController refactoring in preparation for async functions - Bundle everything needed for script evaluation into a "JavaScriptExecutionParameters" object - Instead of returning a value, make evaluateInWorld take a completion handler to callback with the result (For this patch, the completion handler will always be called synchronously)
Created attachment 385725 [details] Patch
Going to be going through a series of EWS runs to find failures on the other platforms
Created attachment 385726 [details] Patch
Created attachment 385736 [details] Patch
Created attachment 385737 [details] Patch
Created attachment 385738 [details] Patch
Created attachment 385789 [details] Patch
Created attachment 385797 [details] Patch
Created attachment 385800 [details] Patch
Created attachment 385808 [details] Patch
The changes further required to make this a no-behavior change are becoming problematic. Resetting a bit.
Created attachment 385835 [details] WIP to take home might not even build.
Created attachment 385852 [details] Patch
Comment on attachment 385852 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=385852&action=review I don't think this accomplishes anything. It just makes a Function that must be called immediately. The whole point of Functions is that they don't need to be called immediately. Do you have plans to push this asynchrony lower? If not, then your intended caller could just take a Function, do the work synchronously, then call the Function with the result. > Source/WebCore/bindings/js/ExceptionDetails.h:36 > - String sourceURL; > + String sourceURL { }; This does nothing because String has a default constructor. > Source/WebCore/bindings/js/ScriptController.h:93 > + using ResolveFunction = WTF::Function<void(ValueOrException)>; CompletionHandler?
(In reply to Alex Christensen from comment #14) > Comment on attachment 385852 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=385852&action=review > > I don't think this accomplishes anything. It just makes a Function that > must be called immediately. The whole point of Functions is that they don't > need to be called immediately. Do you have plans to push this asynchrony > lower? If not, then your intended caller could just take a Function, do the > work synchronously, then call the Function with the result. > > > Source/WebCore/bindings/js/ExceptionDetails.h:36 > > - String sourceURL; > > + String sourceURL { }; > > This does nothing because String has a default constructor. Actually it does! On older compilers (such as High Sierra which the bots run), you can't do a partial struct initialization when some members have default initializers but others don't. See the build failure from the penultimate patch that this ultimate patch fixes (https://ews-build.webkit.org/#/builders/7/builds/15941) > > > Source/WebCore/bindings/js/ScriptController.h:93 > > + using ResolveFunction = WTF::Function<void(ValueOrException)>; > > CompletionHandler? ResolveFunction is a carryover from a future patch - as the asynchrnocity will be waiting for a promise to resolve. If you insist it be called CompletionHandler now I can change it, but I'll be changing it back two patches from now.
Discussed IRL - This was broken out from a completed larger patch, but the discussion inspired me to reanalyze the completed larger patch.
Repurposing
Created attachment 386182 [details] Patch
Patch not quite for review - Trying the EWS builds, and also getting feedback from JS folks
Comment on attachment 386182 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386182&action=review > Source/WebCore/bindings/js/ExceptionDetails.h:36 > + String sourceURL { }; Does this do anything? String has a default initializer. > Source/WebCore/bindings/js/RunJavaScriptParameters.h:32 > +enum class RunAsAsyncFunction { I think this can be `enum class RunAsAsyncFunction : uint8_t`? But maybe EnumTraits will do this for you when XPCing? > Source/WebCore/bindings/js/RunJavaScriptParameters.h:62 > + String source; > + RunAsAsyncFunction runAsAsyncFunction; > + Optional<WebCore::ScriptController::ArgumentWireBytesMap> arguments; > + ForceUserGesture forceUserGesture; Isn't it standard WebKit practice to put the members of a struct/class at the bottom of the struct/class? > Source/WebCore/bindings/js/RunJavaScriptParameters.h:66 > + encoder << source << runAsAsyncFunction << arguments << forceUserGesture; Out of curiosity, why does encoder use << and not a variadic template call? > Source/WebCore/bindings/js/ScriptController.cpp:643 > + JSValue functionObject = JSExecState::profiledEvaluate(&globalObject, JSC::ProfilingReason::Other, jsSourceCode, &proxy, evaluationException); You probably to check for an exception after this call. Then you don't need to check for !functionObject below. > Source/WebCore/bindings/js/ScriptController.cpp:647 > + // FIXME: JS guys say there's a more correct way to do this > + if (!functionObject || !functionObject.isFunction(world.vm())) > + break; I leave this to Saam. I'm not exactly sure which method he was thinking of right now... maybe isCallable? > Source/WebCore/bindings/js/ScriptController.cpp:656 > + returnValue = JSExecState::profiledCall(&globalObject, JSC::ProfilingReason::Other, functionObject, callType, callData, &proxy, markedArguments, evaluationException); Ugh, getting the CallData/CallType shouldn't be required to make a call into JS... Unfortunately, we don't have a profiling version of call for places where we haven't already entered the VM. Can you file a bug against JSC to add this and FIXME link to it here?
Comment on attachment 386182 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386182&action=review >> Source/WebCore/bindings/js/ExceptionDetails.h:36 >> + String sourceURL { }; > > Does this do anything? String has a default initializer. https://bugs.webkit.org/show_bug.cgi?id=205239#c15 >> Source/WebCore/bindings/js/RunJavaScriptParameters.h:32 >> +enum class RunAsAsyncFunction { > > I think this can be `enum class RunAsAsyncFunction : uint8_t`? But maybe EnumTraits will do this for you when XPCing? These should use bool as their storage type. Then you don't need EnumTraits at all because our IPC code knows bool enum types only have two possible values. > Source/WebCore/bindings/js/RunJavaScriptParameters.h:77 > + RunAsAsyncFunction runAsAsyncFunction; > + if (!decoder.decode(runAsAsyncFunction)) > + return WTF::nullopt; Could you use Decoder's operator>> into an Optional<RunAsAsyncFunction>? That's what we're trying to push our decoders towards. > Source/WebCore/bindings/js/ScriptController.cpp:599 > + StringBuilder functionStringBuilder; There's got to be a more efficient way of doing this.
Comment on attachment 386182 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386182&action=review > Source/WebCore/bindings/js/ScriptController.cpp:619 > + functionStringBuilder.append(","); Can append a character instead of a one-character string literal. Slightly more efficient. > Source/WebCore/bindings/js/ScriptController.cpp:627 > + functionStringBuilder.append("){"); > + functionStringBuilder.append(parameters.source); > + functionStringBuilder.append("})"); Can do all three of these with a single call to append and three arguments. Slightly more efficient.
(In reply to Alex Christensen from comment #21) > > > Source/WebCore/bindings/js/RunJavaScriptParameters.h:77 > > + RunAsAsyncFunction runAsAsyncFunction; > > + if (!decoder.decode(runAsAsyncFunction)) > > + return WTF::nullopt; > > Could you use Decoder's operator>> into an Optional<RunAsAsyncFunction>? > That's what we're trying to push our decoders towards. For pods like an enum, isn't that more code and more storage space? > > > Source/WebCore/bindings/js/ScriptController.cpp:599 > > + StringBuilder functionStringBuilder; > > There's got to be a more efficient way of doing this. I'll implement Darin's suggestions.
(In reply to Keith Miller from comment #20) > Comment on attachment 386182 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=386182&action=review > > > Source/WebCore/bindings/js/ExceptionDetails.h:36 > > + String sourceURL { }; > > Does this do anything? String has a default initializer. Alex already covered this by linking to a past comment. But for those following along that don't click his link: When using partial initialion lists with struct in older compilers they require explicit default initialization of all members, even if they are a class that has a default constructor. > > > Source/WebCore/bindings/js/RunJavaScriptParameters.h:62 > > + String source; > > + RunAsAsyncFunction runAsAsyncFunction; > > + Optional<WebCore::ScriptController::ArgumentWireBytesMap> arguments; > > + ForceUserGesture forceUserGesture; > > Isn't it standard WebKit practice to put the members of a struct/class at > the bottom of the struct/class? I don't think we have any such guideline. I've definitely seen it all ways. Maybe more structs lean one way than the other and we should formalize the style. > > Source/WebCore/bindings/js/RunJavaScriptParameters.h:66 > > + encoder << source << runAsAsyncFunction << arguments << forceUserGesture; > > Out of curiosity, why does encoder use << and not a variadic template call? We started on WK2 before C++11 was formalized and available everywhere, and IPC encoder/decoder were kind of a core feature. So the answer is "appeal to ancient wisdom" > > Source/WebCore/bindings/js/ScriptController.cpp:643 > > + JSValue functionObject = JSExecState::profiledEvaluate(&globalObject, JSC::ProfilingReason::Other, jsSourceCode, &proxy, evaluationException); > > You probably to check for an exception after this call. Then you don't need > to check for !functionObject below. Okay. > > > Source/WebCore/bindings/js/ScriptController.cpp:647 > > + // FIXME: JS guys say there's a more correct way to do this > > + if (!functionObject || !functionObject.isFunction(world.vm())) > > + break; > > I leave this to Saam. I'm not exactly sure which method he was thinking of > right now... maybe isCallable? I guess I'll go with isCallable. > > > Source/WebCore/bindings/js/ScriptController.cpp:656 > > + returnValue = JSExecState::profiledCall(&globalObject, JSC::ProfilingReason::Other, functionObject, callType, callData, &proxy, markedArguments, evaluationException); > > Ugh, getting the CallData/CallType shouldn't be required to make a call into > JS... Unfortunately, we don't have a profiling version of call for places > where we haven't already entered the VM. Can you file a bug against JSC to > add this and FIXME link to it here? Can do. Thanks!
(In reply to Brady Eidson from comment #24) > (In reply to Keith Miller from comment #20) > > > > > Source/WebCore/bindings/js/ScriptController.cpp:647 > > > + // FIXME: JS guys say there's a more correct way to do this > > > + if (!functionObject || !functionObject.isFunction(world.vm())) > > > + break; > > > > I leave this to Saam. I'm not exactly sure which method he was thinking of > > right now... maybe isCallable? > > I guess I'll go with isCallable. Actually it looks like isCallable requires the CallData stuff, which you later suggest shouldn't really be required. So I'll leave it as isFunction for now.
(In reply to Keith Miller from comment #20) > Comment on attachment 386182 [details] > > > Source/WebCore/bindings/js/ScriptController.cpp:656 > > + returnValue = JSExecState::profiledCall(&globalObject, JSC::ProfilingReason::Other, functionObject, callType, callData, &proxy, markedArguments, evaluationException); > > Ugh, getting the CallData/CallType shouldn't be required to make a call into > JS... Unfortunately, we don't have a profiling version of call for places > where we haven't already entered the VM. Can you file a bug against JSC to > add this and FIXME link to it here? Added to the patch, and: https://bugs.webkit.org/show_bug.cgi?id=205562
Created attachment 386345 [details] Patch
Created attachment 386362 [details] Patch
Created attachment 386365 [details] Patch
The build failures on the bots: Undefined symbols for architecture arm64: "JSC::getVM(JSC::JSGlobalObject*)", referenced from: API::SerializedScriptValue::wireBytesFromNSObject(objc_object*) in UnifiedSource19-mm.o Unfortunately, wireBytesFromNSObject() doesn't directly call getVM, *and* both debug and release are building great for me locally. Will explore in a bit.
Created attachment 386373 [details] Patch Commenting out the suspicious GetVM.h to try on EWS
(In reply to Brady Eidson from comment #31) > Created attachment 386373 [details] > Patch > > Commenting out the suspicious GetVM.h to try on EWS So that clearly didn't work. GetVM.h declares the existence of getVM(), but there is no actual definition anywhere but inline in JSGlobalObjectInlines.h Which is fine... Except the bizarre linker error says it can't be found.
Created attachment 386374 [details] Patch
The linux build failures are now similar to an earlier much different patch in this series: In file included from DerivedSources/WebCore/unified-sources/UnifiedSource-42f7b70e-2.cpp:6: WebCore/xml/XPathGrammar.y: In function ‘int xpathyyparse(WebCore::XPath::Parser&)’: WebCore/xml/XPathGrammar.y:404:34: error: expected type-specifier before ‘Path’ In file included from DerivedSources/WebCore/unified-sources/UnifiedSource-42f7b70e-2.cpp:6: WebCore/xml/XPathGrammar.y:411:34: error: expected type-specifier before ‘Path’ I'm... really not sure how to proceed resolving that.
Created attachment 386380 [details] 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
That patch fixes the XPath complaints for me locally, and then fixes the rest of the subsequent GTK build errors (Headers.cmake, WebKitWebView, etc.)
(In reply to Tim Horton from comment #37) > That patch fixes the XPath complaints for me locally, and then fixes the > rest of the subsequent GTK build errors (Headers.cmake, WebKitWebView, etc.) Thanks for your help!
Created attachment 386393 [details] Patch
Comment on attachment 386393 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386393&action=review Did not review the entire patch, but one thought. > Source/WebCore/bindings/js/ExceptionDetails.h:36 > - String sourceURL; > + String sourceURL { }; Should not make this change. This doesn’t make a difference ... sourceURL will be a null string with or without this new set of braces. Also note that the data member above named "message" behaves the same and we are not changing it. > Source/WebCore/bindings/js/RunJavaScriptParameters.h:48 > + RunJavaScriptParameters(const String& source, RunAsAsyncFunction runAsAsyncFunction, Optional<ArgumentWireBytesMap> arguments, ForceUserGesture forceUserGesture) Need an overload with String&& and Optional<ArgumentWireBytesMap>&&. That can be used in the decode function template below. Without the && on the arguments, the WTFMove does not good. Without the && on source, then WTFMove can’t be done to save a little reference count churn. > Source/WebCore/bindings/js/RunJavaScriptParameters.h:84 > + return { { source, runAsAsyncFunction, WTFMove(arguments), forceUserGesture } }; Should add a WTFMove around source.
(In reply to Darin Adler from comment #40) > Comment on attachment 386393 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=386393&action=review > > Did not review the entire patch, but one thought. > > > Source/WebCore/bindings/js/ExceptionDetails.h:36 > > - String sourceURL; > > + String sourceURL { }; > > Should not make this change. This doesn’t make a difference ... sourceURL > will be a null string with or without this new set of braces. Also note that > the data member above named "message" behaves the same and we are not > changing it. This is the third time I've had to reply to this very same comment. This change is required by older compilers (e.g. High Sierra on the bots) because they improperly handle initialization lists without it. Here is a direct link to an error caused by omitting this change: https://ews-build.webkit.org/#/builders/7/builds/15941/steps/7/logs/errors > > > Source/WebCore/bindings/js/RunJavaScriptParameters.h:48 > > + RunJavaScriptParameters(const String& source, RunAsAsyncFunction runAsAsyncFunction, Optional<ArgumentWireBytesMap> arguments, ForceUserGesture forceUserGesture) > > Need an overload with String&& and Optional<ArgumentWireBytesMap>&&. That > can be used in the decode function template below. Without the && on the > arguments, the WTFMove does not good. Without the && on source, then WTFMove > can’t be done to save a little reference count churn. > > > Source/WebCore/bindings/js/RunJavaScriptParameters.h:84 > > + return { { source, runAsAsyncFunction, WTFMove(arguments), forceUserGesture } }; > > Should add a WTFMove around source. Will do both of these.
Created attachment 386432 [details] Patch
Created attachment 386433 [details] Patch
(In reply to Brady Eidson from comment #43) > Created attachment 386433 [details] > Patch Confirmed with this patch that removing the sourceURL intializer definitely still breaks things on the older compilers :P
Created attachment 386434 [details] Patch
Comment on attachment 386393 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386393&action=review >>> Source/WebCore/bindings/js/ExceptionDetails.h:36 >>> + String sourceURL { }; >> >> Should not make this change. This doesn’t make a difference ... sourceURL will be a null string with or without this new set of braces. Also note that the data member above named "message" behaves the same and we are not changing it. > > This is the third time I've had to reply to this very same comment. > > This change is required by older compilers (e.g. High Sierra on the bots) because they improperly handle initialization lists without it. > > Here is a direct link to an error caused by omitting this change: > https://ews-build.webkit.org/#/builders/7/builds/15941/steps/7/logs/errors Given your experience having to respond to this comment three times, I think a brief comment on that line of source code is called for. That way someone won’t remove this in the future and then learn about why it’s there only when a High Sierra build fails.
Created attachment 386461 [details] Patch
(In reply to Darin Adler from comment #46) > Comment on attachment 386393 [details] > > Given your experience having to respond to this comment three times, I think > a brief comment on that line of source code is called for. That way someone > won’t remove this in the future and then learn about why it’s there only > when a High Sierra build fails. Indeed! Done. Perhaps we can all band together for a small Christmas miracle and review the parts of this patch not contained in ExceptionDetails.h :)
Comment on attachment 386461 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386461&action=review r=me with lots of nits. > Source/WebCore/bindings/js/RunJavaScriptParameters.h:40 > + RunJavaScriptParameters(const String& source, bool runAsAsyncFunction, Optional<ArgumentWireBytesMap> arguments, bool forceUserGesture) I think if we remove both of these constructors and just use WTFMove in the decoder, it should do everything Darin hoped. > Source/WebCore/bindings/js/ScriptController.cpp:569 > + auto result = executeScriptInWorld(world, { script, false, WTF::nullopt, forceUserGesture }); It would probably be worth putting RunJavaScriptParameters here so we find it easier when we add more members to RunJavaScriptParameters that have initializers and this will continue to successfully compile. > Source/WebCore/bindings/js/ScriptController.cpp:589 > + default: > + RELEASE_ASSERT_NOT_REACHED(); I think it would be better to not have a default here. > Source/WebCore/bindings/js/ScriptController.cpp:614 > + errorMessage = "Unable to deserialize argument to execute asynchronous JavaScript function"; > + break; Should we return the exception here? > Source/WebCore/bindings/js/ScriptController.h:99 > + using ResolveFunction = WTF::Function<void(ValueOrException)>; Could this be a CompletionHandler, because it needs to be called once? WTF:: unnecessary in either case. > Source/WebCore/xml/XPathGrammar.y:404 > + $$ = new XPath::Path(std::unique_ptr<Expression>($1), std::unique_ptr<LocationPath>($3)); This could use a comment in the ChangeLog saying it's unrelated but necessary to continue compiling for some reason. > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:904 > + _page->runJavaScriptInMainFrame({ javaScriptString, (bool)asAsyncFunction, WTFMove(argumentsMap), (bool)forceUserGesture }, [handler](API::SerializedScriptValue* serializedScriptValue, Optional<WebCore::ExceptionDetails> details, WebKit::ScriptValueCallback::Error errorCode) { I think we prefer !! or static_cast<bool> over c-style casting to bool. Also, we should move handler into the capture list. > Source/WebKit/UIProcess/API/Cocoa/WKWebViewPrivate.h:319 > +- (void)_callAsyncFunction:(NSString *)javaScriptString withArguments:(NSDictionary *)arguments completionHandler:(void (^)(id, NSError *error))completionHandler; Could this be NSDictionary<NSString *, id> *? It seems the keys are the variable names. > Source/WebKit/WebProcess/WebPage/WebPage.cpp:3382 > + send(Messages::WebPageProxy::ScriptValueCallback(dataReference, details, callbackID)); Maybe as a followup we could use sendWithAsyncReply instead of two messages and manually generating and keeping track of this identifier. > Tools/TestWebKitAPI/Tests/WebKitCocoa/AsyncFunction.mm:92 > + // Values can only be NSString, NSNumber, NSDate, NSArray, NSDictionary, and may only contain those 5 types. The return value seems to be able to be NSNull. Can a parameter? Either way, I think that would be a great test case. Also, what about NSMutableDictionary? Also, what about custom classes that inherit from these types?
(In reply to Alex Christensen from comment #49) > Comment on attachment 386461 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=386461&action=review > > r=me with lots of nits. Accepting all of the nits except for where I comment: > > > Source/WebCore/bindings/js/ScriptController.cpp:589 > > + default: > > + RELEASE_ASSERT_NOT_REACHED(); > > I think it would be better to not have a default here. This was to placate one of the windows or linux compilers that complained about lack of return statement. *sigh* > > Source/WebCore/bindings/js/ScriptController.cpp:614 > > + errorMessage = "Unable to deserialize argument to execute asynchronous JavaScript function"; > > + break; > > Should we return the exception here? I waffled on this. Certainly not just the raw exception, as it would appear as if the function executed with a bizarre error. So I decided to skip it because - since we validate the arguments up front in the UI process - I don't think we'll ever actually hit this. > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/AsyncFunction.mm:92 > > + // Values can only be NSString, NSNumber, NSDate, NSArray, NSDictionary, and may only contain those 5 types. > > The return value seems to be able to be NSNull. Can a parameter? Totally. Dunno how I missed that since I literally copied the comment from WKScriptMessage.h > Also, what about NSMutableDictionary? Since it's a subclass of NSDictionary, yes. I'll add a test. > Also, what about custom classes that inherit from these types? For string, number, and date. subclasses will work fine. The base class accessors will be used to serialize the argument to the javascript type. Custom subclassing of NSArray and NSDictionary is a nonstarter for outside devs because of class clustering, etc etc. I'll update the comment.
(In reply to Brady Eidson from comment #50) > (In reply to Alex Christensen from comment #49) > > > Also, what about custom classes that inherit from these types? > > For string, number, and date. subclasses will work fine. The base class > accessors will be used to serialize the argument to the javascript type. > > Custom subclassing of NSArray and NSDictionary is a nonstarter for outside > devs because of class clustering, etc etc. > > I'll update the comment. I was totally wrong; NSString, NSNumber, and NSDate are also all class clusters that are difficult to subclass. I'm gonna leave it as-is (besides adding NSNull)
(In reply to Alex Christensen from comment #49) > > > Source/WebCore/bindings/js/RunJavaScriptParameters.h:40 > > + RunJavaScriptParameters(const String& source, bool runAsAsyncFunction, Optional<ArgumentWireBytesMap> arguments, bool forceUserGesture) > > I think if we remove both of these constructors and just use WTFMove in the > decoder, it should do everything Darin hoped. More followup: The key to having these at all was the constructor that took the bools (which the API layers speak) and converted them to the enums (which IPC and WebCore now speak) By adding the bool form, you lose the implicit constructor, so I needed to add that one too. I've left them and added all the WTFMoves
Created attachment 386509 [details] Patch
Created attachment 386535 [details] Patch
Created attachment 386536 [details] Patch
Comment on attachment 386536 [details] Patch Clearing flags on attachment: 386536 Committed r253950: <https://trac.webkit.org/changeset/253950>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58245873>
Comment on attachment 386536 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386536&action=review > Source/WebCore/bindings/js/ScriptController.cpp:649 > + if (!functionObject || !functionObject.isFunction(world.vm())) { This check is ok. I was mistaken. This isn't the correct way to check if an arbitrary value is "callable". However, the value here isn't arbitrary. You're using a function in your text that you're evaluating. So isFunction is sufficient. Sorry for getting confused.
*** Bug 134330 has been marked as a duplicate of this bug. ***