Make _callAsyncFunction:withArguments: work with promises
Created attachment 386888 [details] Patch
Comment on attachment 386888 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=386888&action=review r=me > Source/WTF/wtf/SharedFunction.h:36 > +class SharedFunction<Out(In...)> : public RefCounted<SharedFunction<Out(In...)>> { I think instead of this we can just use SharedTask > Source/WebCore/bindings/js/ScriptController.cpp:715 > + auto thenIdentifier = world.vm().propertyNames->builtinNames().thenPrivateName(); this should be: "world.vm().propertyNames->then". A test that would catch this is using your own "thenable" > Source/WebCore/bindings/js/ScriptController.cpp:719 > + if (!thenFunction.isFunction(world.vm())) { The right check here is: CallData ignoredCallData; if (thenFunction.isObject && asObject(thenFunction)->methodTable(vm)->getCallData(asObject(thenFunction), ignoredCallData) != CallType::None) I think a test for a callable which does not pass the isFunction() query would be something like this with a Proxy as the "thenFunction": let p = new Proxy(function() { print("this should be called"); }, { }); let thenable = {then: p}; > Source/WebCore/bindings/js/ScriptController.cpp:740 > + auto finalizeCount = makeUniqueWithoutFastMallocCheck<unsigned>(0); nit: can you change makeUnique to just check if T is a primitive? > Source/WebCore/bindings/js/ScriptController.cpp:747 > + if (heap) { these branches are not needed. > Source/WebCore/bindings/js/ScriptController.cpp:754 > + if (heap) { ditto. > Source/WebCore/bindings/js/ScriptController.cpp:756 > + finalizeGuard.get()(); instead of getting the heap pointer here, you can just do: world.vm().heap
Created attachment 386907 [details] Patch
The problem with these test runs is spamming to the console from the AsyncFunction.Promise test affecting later runs. Damn. Fixing.
Created attachment 387049 [details] Patch
(In reply to Brady Eidson from comment #4) > The problem with these test runs is spamming to the console from the > AsyncFunction.Promise test affecting later runs. > > Damn. > > Fixing. Nope that wasn't it. Accidentally applying the new async behavior to everybody. Fixed in most recent upload.
That fixed the collateral damage to other API tests but revealed that the original AsyncFunction tests are now busted. I can't quite figure out why, other than verify locally before this patch they work, and with it applied they don't. It's a problem with serialization. Before my patch, a JSC::JSValue representing "undefined" successfully serializes. Same for "null" With my patch, those exact same JSValues do not serialize.
For the "null" and "undefined" jsvalues, SerializedScriptValue::create(JSContextRef originContext, JSValueRef apiValue, JSValueRef* exception) succeeds before my patch It fails after my patch because it has an exception.
Here's the body of SerializedScriptValue::create. With my patch, an exception occurs in here. Without my patch, it doesn't. RefPtr<SerializedScriptValue> SerializedScriptValue::create(JSContextRef originContext, JSValueRef apiValue, JSValueRef* exception) { JSGlobalObject* lexicalGlobalObject = toJS(originContext); VM& vm = lexicalGlobalObject->vm(); JSLockHolder locker(vm); auto scope = DECLARE_CATCH_SCOPE(vm); JSValue value = toJS(lexicalGlobalObject, apiValue); ------> Exception happens in the above line auto serializedValue = SerializedScriptValue::create(*lexicalGlobalObject, value); if (UNLIKELY(scope.exception())) { if (exception) *exception = toRef(lexicalGlobalObject, scope.exception()->value()); scope.clearException(); return nullptr; } ASSERT(serializedValue); return serializedValue; }
(In reply to Brady Eidson from comment #9) > Here's the body of SerializedScriptValue::create. > > With my patch, an exception occurs in here. > Without my patch, it doesn't. > > RefPtr<SerializedScriptValue> SerializedScriptValue::create(JSContextRef > originContext, JSValueRef apiValue, JSValueRef* exception) > { > JSGlobalObject* lexicalGlobalObject = toJS(originContext); > VM& vm = lexicalGlobalObject->vm(); > JSLockHolder locker(vm); > auto scope = DECLARE_CATCH_SCOPE(vm); > > JSValue value = toJS(lexicalGlobalObject, apiValue); > > ------> Exception happens in the above line > > auto serializedValue = > SerializedScriptValue::create(*lexicalGlobalObject, value); > if (UNLIKELY(scope.exception())) { > if (exception) > *exception = toRef(lexicalGlobalObject, > scope.exception()->value()); > scope.clearException(); > return nullptr; > } > ASSERT(serializedValue); > return serializedValue; > } Nope, I was wrong - the exception has already occurred the moment you make the DECLARE_CATCH_SCOPE. This must be because of my patch's attempt to pull the "then" property from the result.
(In reply to Brady Eidson from comment #10) > (In reply to Brady Eidson from comment #9 > > Nope, I was wrong - the exception has already occurred the moment you make > the DECLARE_CATCH_SCOPE. > > This must be because of my patch's attempt to pull the "then" property from > the result. Confirmed. By smattering exception scope checks throughout ScriptController::executeAsynchronousUserAgentScriptInWorld I found it occurs on this line: auto thenFunction = result.value().get(&globalObject, thenIdentifier);
(In reply to Brady Eidson from comment #11) > (In reply to Brady Eidson from comment #10) > > (In reply to Brady Eidson from comment #9 > > > > Nope, I was wrong - the exception has already occurred the moment you make > > the DECLARE_CATCH_SCOPE. > > > > This must be because of my patch's attempt to pull the "then" property from > > the result. > > Confirmed. By smattering exception scope checks throughout > ScriptController::executeAsynchronousUserAgentScriptInWorld I found it > occurs on this line: > > auto thenFunction = result.value().get(&globalObject, thenIdentifier); It appears doing that on a JSNull or JSUndefined throws an exception. Will try my own fix and make sure Saam takes another look.
Created attachment 387079 [details] Patch
(In reply to Brady Eidson from comment #13) > Created attachment 387079 [details] > Patch Basically added an "isNullOrUndefined()" check on the value before we try to extract "then" from it.
Comment on attachment 387079 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387079&action=review > Source/WebCore/bindings/js/ScriptController.cpp:718 > + auto thenFunction = result.value().get(&globalObject, thenIdentifier); I think the appropriate check is just to check for exceptions after this (and maybe omit the isUndefinedOrNull, since that'll just fall out from this). (Even if result.value() isn't null or undefined, this "get" may run arbitrary JS code, and can throw. For. example, when it's a getter). I think you want to declare a catch scope, and return early (and maybe reject?) if an exception is thrown. The only other place where an exception might get thrown is your JS function call at the bottom. However, since that's the final thing in this function, you can call "release" on your exception scope before doing the JS call.
(In reply to Saam Barati from comment #15) > Comment on attachment 387079 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=387079&action=review > > > Source/WebCore/bindings/js/ScriptController.cpp:718 > > + auto thenFunction = result.value().get(&globalObject, thenIdentifier); > > I think the appropriate check is just to check for exceptions after this > (and maybe omit the isUndefinedOrNull, since that'll just fall out from > this). > (Even if result.value() isn't null or undefined, this "get" may run > arbitrary JS code, and can throw. For. example, when it's a getter). Okay, but... > I think you want to declare a catch scope, and return early (and maybe > reject?) if an exception is thrown. No, because it's 100% valid to return null or undefined and pass them back to the UI process. So if looking up "then" on a real object can run arbitrary JS code and throw an exception, we definitely should pass that back to the UI process as an error. But looking up "then" on null or undefined should silently fail, and we need to pass back null or undefined to the UI process. I'm guessing that it's impossible to detect *why* an exception was thrown in the get() call, and therefore I need to prequalify the value beforehand, like in this patch? Or is there something else I'm missing?
Note: The api-ios failures here are not due to this patch.
Pow-wow'ed with Keith - Should do an isObject check before doing the get (This is what some of the related ES spec algorithms do)
Created attachment 387116 [details] Patch
(In reply to Brady Eidson from comment #16) > (In reply to Saam Barati from comment #15) > > Comment on attachment 387079 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=387079&action=review > > > > > Source/WebCore/bindings/js/ScriptController.cpp:718 > > > + auto thenFunction = result.value().get(&globalObject, thenIdentifier); > > > > I think the appropriate check is just to check for exceptions after this > > (and maybe omit the isUndefinedOrNull, since that'll just fall out from > > this). > > (Even if result.value() isn't null or undefined, this "get" may run > > arbitrary JS code, and can throw. For. example, when it's a getter). > > Okay, but... > > > I think you want to declare a catch scope, and return early (and maybe > > reject?) if an exception is thrown. > > No, because it's 100% valid to return null or undefined and pass them back > to the UI process. I see. I didn't realize that. What Keith suggested to you seems reasonable to me too. > > So if looking up "then" on a real object can run arbitrary JS code and throw > an exception, we definitely should pass that back to the UI process as an > error. > > But looking up "then" on null or undefined should silently fail, and we need > to pass back null or undefined to the UI process. > > I'm guessing that it's impossible to detect *why* an exception was thrown in > the get() call, and therefore I need to prequalify the value beforehand, > like in this patch? > > Or is there something else I'm missing?
Comment on attachment 387116 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=387116&action=review > Source/WebCore/bindings/js/ScriptController.cpp:718 > + auto thenFunction = result.value().get(&globalObject, thenIdentifier); I think you still want an exception check after this.
(In reply to Saam Barati from comment #21) > Comment on attachment 387116 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=387116&action=review > > > Source/WebCore/bindings/js/ScriptController.cpp:718 > > + auto thenFunction = result.value().get(&globalObject, thenIdentifier); > > I think you still want an exception check after this. As mentioned, the SerializedScriptValue creator handles this. It would be nicer to catch the native exception, and I'll do it in a followup
Comment on attachment 387116 [details] Patch Clearing flags on attachment: 387116 Committed r254222: <https://trac.webkit.org/changeset/254222>
All reviewed patches have been landed. Closing bug.
<rdar://problem/58420346>
It looks like the changes in https://trac.webkit.org/changeset/254222/webkit introduced a Timing out API test TestWebKitAPI.AsyncFunction.Promise https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.AsyncFunction.Promise tracked in https://bugs.webkit.org/show_bug.cgi?id=206012
(In reply to Truitt Savell from comment #26) > It looks like the changes in https://trac.webkit.org/changeset/254222/webkit > > introduced a Timing out API test TestWebKitAPI.AsyncFunction.Promise > > https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.AsyncFunction. > Promise > > tracked in https://bugs.webkit.org/show_bug.cgi?id=206012 Yet Another Case™ of EWS not agreeing with the bots. Will take a look in that other bug.
(In reply to Brady Eidson from comment #27) > (In reply to Truitt Savell from comment #26) > > It looks like the changes in https://trac.webkit.org/changeset/254222/webkit > > > > introduced a Timing out API test TestWebKitAPI.AsyncFunction.Promise > > > > https://results.webkit.org/?suite=api-tests&test=TestWebKitAPI.AsyncFunction. > > Promise > > > > tracked in https://bugs.webkit.org/show_bug.cgi?id=206012 > > Yet Another Case™ of EWS not agreeing with the bots. > > Will take a look in that other bug. It looks like it's only on Debug builds, EWS tests Release.