983301 - Implement WebIDL spec provisions for Promise arguments

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

As far as I can tell these are:

1)  Promises are not distinguishable from anything else.
2)  Converting a JS value to V a Promise is done via calling canonical
    %Promise%.resolve(V)

Comment 1

[Boris Zbarsky [:bzbarsky]]

As in bug 983300, we can do this without adding a new Promise syntactic type for now.

Also, we currently have no Promise args, so we could just make them fail codegen pending this bug being fixed...

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

We need this for service workers eventually so FetchEvent.respondWith(5) properly triggers an async error instead of throwing in the event handler.

Comment 3

[Boris Zbarsky [:bzbarsky]]

So looks like FetchEvent.respondWith and ExtendableEvent.waitUntil now use Promise arguments.  Also Navigator.mozSetMessageHandlerPromise.

We also end up hitting this case for the RTCIdentityProvider callback interface, which has Promise return values.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: Handle JS-to-native conversion for Promise values via calling Promise::Resolve on whatever value was passed in (except if the type is nullable and the passed-in value is null or undefined)

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, I guess that only does part 2.  I should do part 1 as well... Patch for that coming up.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Change Promise to not be distinguishable from any other type

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8638790 [details] [diff] [review]
part 2.  Change Promise to not be distinguishable from any other type

Looks right.  These tests in dom/workers/test/serviceworkers pass locally, although a lot of stuff is leaked afterwards.  It seems unlikely thats related to this bug and may just be a difference with me switching to gtk3 recently.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Add a test for FetchEvent.respondWith(5). r=bz

I verified this succeeds with your patches, but fails with the old code.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8641121 [details] [diff] [review]
Add a test for FetchEvent.respondWith(5). r=bz

r=me

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 8638753 [details] [diff] [review]
Handle JS-to-native conversion for Promise values via calling Promise::Resolve on whatever value was passed in (except if the type is nullable and the passed-in value is null or undefined)

Review of attachment 8638753 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/bindings/Codegen.py
@@ -4939,5 @@
>                          isCallbackReturnValue)
>  
> -        # Sequences and non-worker callbacks have to hold a strong ref to the
> -        # thing being passed down.  Union return values must hold a strong ref
> -        # because they may be returning an addrefed pointer.

Does isMember not apply to union members anymore? Because the rewritten comments don't mention them anymore.

::: dom/bindings/test/TestBindingHeader.h
@@ +716,5 @@
> +  void PassOptionalNullablePromiseWithDefaultValue(Promise*);
> +  void PassPromiseSequence(const Sequence<OwningNonNull<Promise>>&);
> +  void PassPromiseMozMap(const MozMap<nsRefPtr<Promise>>&);
> +  void PassNullablePromiseSequence(const Sequence<nsRefPtr<Promise>> &);
> +  Promise* ReceivePromise();

Maybe add one returning an already_AddRefed<Promise>?

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Does isMember not apply to union members anymore?

It does, for owning unions.  I was thinking that talking about "union return values" in the js-to-native conversion didn't make sense, but I guess it does for JS-implemented interfaces.

Updated that comment to say:

        # Sequence and dictionary members, as well as owning unions (which can
        # appear here as return values in JS-implemented interfaces) have to
        # hold a strong ref to the thing being passed down.  Those all set
        # isMember.

> Maybe add one returning an already_AddRefed<Promise>?

Done.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/565c77fa874c
https://hg.mozilla.org/integration/mozilla-inbound/rev/27db61bad678

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/565c77fa874c
https://hg.mozilla.org/mozilla-central/rev/27db61bad678

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

NI myself to remember to land the test patch here.

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Sorry for the delay in pushing this patch.

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd9983f167c2

Comment 17

[Guilherme Lima]

This landed yesterday morning - https://hg.mozilla.org/mozilla-central/rev/bd9983f167c2

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Thanks.  Sorry for the late push.  I probably should have opened a separate bug.