Bug 205654 – Make _callAsyncFunction:withArguments: work with promises

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Brady Eidson]

Make _callAsyncFunction:withArguments: work with promises

[Brady Eidson]

Created attachment 386888 [details]
Patch

[Saam Barati]

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

[Brady Eidson]

Created attachment 386907 [details]
Patch

[Brady Eidson]

The problem with these test runs is spamming to the console from the AsyncFunction.Promise test affecting later runs.

Damn.

Fixing.

[Brady Eidson]

Created attachment 387049 [details]
Patch

[Brady Eidson]

(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.

[Brady Eidson]

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.

[Brady Eidson]

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.

[Brady Eidson]

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;
}

[Brady Eidson]

(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.

[Brady Eidson]

(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);

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 387079 [details]
Patch

[Brady Eidson]

(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.

[Saam Barati]

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.

[Brady Eidson]

(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?

[Brady Eidson]

Note: The api-ios failures here are not due to this patch.

[Brady Eidson]

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)

[Brady Eidson]

Created attachment 387116 [details]
Patch

[Saam Barati]

(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?

[Saam Barati]

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.

[Brady Eidson]

(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

[WebKit Commit Bot]

Comment on attachment 387116 [details]
Patch

Clearing flags on attachment: 387116

Committed r254222: <https://trac.webkit.org/changeset/254222>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/58420346>

[Truitt Savell]

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

[Brady Eidson]

(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.

[Jonathan Bedard]

(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.