Implement WebIDL spec provisions for Promise arguments

RESOLVED FIXED in Firefox 42

Status

()

defect
RESOLVED FIXED
5 years ago
2 months ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

(Blocks 2 bugs)

unspecified
mozilla42
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(3 attachments)

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)
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...
Blocks: 885333
We need this for service workers eventually so FetchEvent.respondWith(5) properly triggers an async error instead of throwing in the event handler.
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.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Oh, I guess that only does part 2.  I should do part 1 as well... Patch for that coming up.
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.
Attachment #8638790 - Flags: feedback?(bkelly) → feedback+
I verified this succeeds with your patches, but fails with the old code.
Attachment #8641121 - Flags: review?(bzbarsky)
Comment on attachment 8641121 [details] [diff] [review]
Add a test for FetchEvent.respondWith(5). r=bz

r=me
Attachment #8641121 - Flags: review?(bzbarsky) → review+
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>?
Attachment #8638753 - Flags: review?(peterv) → review+
Attachment #8638790 - Flags: review?(peterv) → review+
> 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.
https://hg.mozilla.org/mozilla-central/rev/565c77fa874c
https://hg.mozilla.org/mozilla-central/rev/27db61bad678
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
NI myself to remember to land the test patch here.
Flags: needinfo?(bkelly)
Sorry for the delay in pushing this patch.
Flags: needinfo?(bkelly)
Thanks.  Sorry for the late push.  I probably should have opened a separate bug.
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.