983300 - Implement the WebIDL spec provisions for Promise return values

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

As far as I can tell those are:

1) Legacy callers may not have a Promise return type
2) If an operation has a promise return type, any exceptions are converted to
   calls to the canonical %Promise%.reject.

There is no mention of attribute getters of Promise type.  Presumably those don't do anything magic.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Oh, I'm not going to introduce a new Promise type for now, by the way.  Will keep it an interface type.

Comment 2

[Boris Zbarsky [:bzbarsky]]

What should happen for "Promise?" return values?  Should those even be allowed?

Comment 3

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky [:bz] from comment #0)
> As far as I can tell those are:
> 
> 1) Legacy callers may not have a Promise return type

Right.

> 2) If an operation has a promise return type, any exceptions are converted to
>    calls to the canonical %Promise%.reject.

Yes.  And that's the case for callbacks and operations/attributes on callback interfaces too.

> There is no mention of attribute getters of Promise type.  Presumably those
> don't do anything magic.

I think I forgot to consider attributes properly.  They're not defined to do anything magic at the moment, but perhaps they should convert exceptions into rejected Promise objects too?

(In reply to Boris Zbarsky [:bz] from comment #2)
> What should happen for "Promise?" return values?  Should those even be
> allowed?

Hmm.  I think they should be disallowed everywhere, not just as return values.

Comment 4

[Cameron McCormack (:heycam)]

Filed these two bugs on the spec:

https://www.w3.org/Bugs/Public/show_bug.cgi?id=25048
https://www.w3.org/Bugs/Public/show_bug.cgi?id=25049

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Disallow Promise as a return type for legacy callers.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Introduce a GenericPromiseReturningBindingMethod for methods that return Promise return value.

Bobby, could you make sure the callee setup here makes sense?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Convert exceptions in specialized methods returning promises into rejected promises. efaust

I'm not a huge fan of all the identical-ish methods this produces, but they're pretty small, and I had nowhere to stick them: we're already using the jitinfo for the specialized method.  I _did_ consider just turning off the Ion fast path for methods returning Promise, but wasn't sure that would be better.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Convert exceptions in static methods returning promises into rejected promises.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Try run: https://tbpl.mozilla.org/?tree=Try&rev=904593e2b36e

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8390985 [details] [diff] [review]
part 2.  Introduce a GenericPromiseReturningBindingMethod for methods that return Promise return value.

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

Yeah, putting the promise in the compartment of the callee (modulo Xrays) makes sense. r=me on that part.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 2 updated per discussion with efaust to eliminate one level of call indirection at the cost of a bit of extra copy/paste

Comment 12

[Eric Faust [:efaust]]

Comment on attachment 8391003 [details] [diff] [review]
part 4.  Convert exceptions in static methods returning promises into rejected promises.

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

Looks good to me. r=efaust

::: js/src/jsfriendapi.h
@@ +1671,5 @@
>      {
>          return type() == ParallelNative;
>      }
>  
>      bool isDOMJitInfo() const

nit: Since this function is used in the ICs to signal that it's OK with caching getter functions with outer window hooks. Since this thing is a static function that ignores the |this| value anyway, it seems weird to add this here.

It seems silly to care about the machanics at at a getter site, but at least the name is then misleading, I guess, since static functions totally are DOM related.

Comment 13

[Eric Faust [:efaust]]

Comment on attachment 8390988 [details] [diff] [review]
part 3.  Convert exceptions in specialized methods returning promises into rejected promises.   efaust

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

Good. r=me

::: js/src/jsfriendapi.h
@@ +1577,5 @@
>          return Base::hasDefined(i);
>      }
>  
> +    JSObject &callee() const {
> +        // We can't use Base::callee() becaue that will try to poke at

nit: because

Comment 14

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8390984 [details] [diff] [review]
part 1.  Disallow Promise as a return type for legacy callers.

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

Please add the bit about overloads to your commit message here.

::: dom/bindings/parser/tests/test_promise.py
@@ +61,5 @@
> +    harness.ok(True,
> +               "Should allow overloads which only have Promise and return "
> +               "types.")
> +
> +    

extra \n / whitespace at EOF

Comment 15

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8393767 [details] [diff] [review]
Part 2 updated per discussion with efaust to eliminate one level of call indirection at the cost of a bit of extra copy/paste

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

I convinced myself that this is correct.

::: dom/bindings/BindingUtils.cpp
@@ +2288,5 @@
> +  const JSJitInfo *info = FUNCTION_VALUE_TO_JITINFO(args.calleev());
> +  prototypes::ID protoID = static_cast<prototypes::ID>(info->protoID);
> +  if (!args.thisv().isObject()) {
> +    unused <<
> +      ThrowInvalidThis(cx, args,

Is this unused really adding anything?

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Is this unused really adding anything?

Well, it's making it clear that we're ignoring the normally-used boolean return value...  I could just nix it; I agree it's pretty ugly.  ;)

Comment 17

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

I think it's a waste of time for anything that doesn't have MOZ_WARN_UNUSED_RESULT.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> but at least the name is then misleading, I guess

That's fair.  How about reversing the sense of the boolean and calling it needsOuterizedThisObject()?  That makes it _very_ clear what's really being tested.  I'd still prefer to return true from needsOuterizedThisObject() for the static methods, since that's generally safer, imo, in case someone does change them to use "this" for some odd reason.

Comment 19

[Eric Faust [:efaust]]

(In reply to Boris Zbarsky [:bz] from comment #18)
> > but at least the name is then misleading, I guess
> 
> That's fair.  How about reversing the sense of the boolean and calling it
> needsOuterizedThisObject()?  That makes it _very_ clear what's really being
> tested.  I'd still prefer to return true from needsOuterizedThisObject() for
> the static methods, since that's generally safer, imo, in case someone does
> change them to use "this" for some odd reason.

That sounds great. At least then everyone understands what's going on. It will also simplify the other use of isDOMJitInfo() for calling the thisObject hook.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Applied the review comments, and:

   https://hg.mozilla.org/integration/mozilla-inbound/rev/bb16317f5d24
   https://hg.mozilla.org/integration/mozilla-inbound/rev/4289ebd5725c
   https://hg.mozilla.org/integration/mozilla-inbound/rev/26940e1fb960
   https://hg.mozilla.org/integration/mozilla-inbound/rev/85285b291ab4

Hard to write tests, since we don't have any existing promise-returning APIs that I can rely on to try throwing in a test.  :(

Comment 21

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

https://hg.mozilla.org/mozilla-central/rev/bb16317f5d24
https://hg.mozilla.org/mozilla-central/rev/4289ebd5725c
https://hg.mozilla.org/mozilla-central/rev/26940e1fb960
https://hg.mozilla.org/mozilla-central/rev/85285b291ab4