1170760 - Implement Promise subclassing properly

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

If we want to do this, we need to significantly modify a bunch of our Promise stuff.  In particular, we need all the static methods to take a this value, at the very least, because the ES Promise spec is written based on using that this value in all sorts of places.

I _think_ this can be done without doing bug 1135961, but mostly doesn't become observable until that bug is done.

The big question is how much we should prioritize this...

Comment 1

[Boris Zbarsky [:bzbarsky]]

OK, so just for my information, my final try runs are at https://treeherder.mozilla.org/#/jobs?repo=try&revision=101d223d6cfb with the fast construction path enabled and https://treeherder.mozilla.org/#/jobs?repo=try&revision=e3a62a2b9b4c with it disabled.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Introduce a PromiseCapability struct

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Pass in the 'this' value to Promise static methods

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Pass in the 'this' value to Promise static methods

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add an @@species getter on Promise

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Change Promise::Constructor to run in the Xray compartment when new Promise happens over Xrays

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Implement NewPromiseCapability which can either return a PromiseCapability as in the spec, or one that has a native promise and maybe resolve/reject functions if the consumer asked for them

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Fix GetDependentPromise to deal with a situation when someone called then() and passed it the resolve/reject functions that come from a promise's constructor

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Add subclassing support to Promise::Race

Note that the web platform tests don't actually have quite the behavior they're
expected to per the spec yet.  They will get adjusted later on as we add
subclassing support to Promise.resolve and Promise.prototype.then.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Add subclassing support to Promise::All

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Stop using Promise::Resolve in the bindings for PromiseDebugging

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Add subclassing support to Promise::Resolve

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 11. Add subclassing support to Promise::Reject

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: part 12. Rip out the promise-resolved-with-promise fast path

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 13. Add subclassing support to Promise::Then/Catch

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 10 with more bind() goodness in tests

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 11 (Promise.rejec) with more bind() goodness in sync code

Comment 18

[Peter Van der Beken [:peterv]]

Comment on attachment 8684347 [details] [diff] [review]
part 3.  Add an @@species getter on Promise

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

Not putting this in NativeProperties does mean that this won't work from Xrays :-/.

Comment 19

[Andrea Marchesini [:baku]]

Comment on attachment 8684343 [details] [diff] [review]
part 1.  Introduce a PromiseCapability struct

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

::: dom/promise/Promise.cpp
@@ +333,5 @@
> +// constructor.  In that case we will just set mNativePromise in our
> +// PromiseCapability and not set mPromise/mResolve/mReject; the correct
> +// callbacks will be the standard Promise ones, and we don't really want to
> +// synthesize JSFunctions for them in that situation.
> +struct MOZ_STACK_CLASS Promise::PromiseCapability {

{ in a new line.

@@ +334,5 @@
> +// PromiseCapability and not set mPromise/mResolve/mReject; the correct
> +// callbacks will be the standard Promise ones, and we don't really want to
> +// synthesize JSFunctions for them in that situation.
> +struct MOZ_STACK_CLASS Promise::PromiseCapability {
> +  PromiseCapability(JSContext* aCx)

explicit ?

@@ +368,5 @@
> +  RefPtr<Promise> mNativePromise;
> +
> +private:
> +  // We don't want to allow creation of temporaries of this type, ever.
> +  PromiseCapability(const PromiseCapability&) = delete;

do you want to delete also the Move ctor?

@@ +384,5 @@
> +  MOZ_ASSERT(mNativePromise || !mPromise.isUndefined(),
> +             "NewPromiseCapability didn't succeed");
> +
> +  JS::Rooted<JS::Value> exn(aCx);
> +  if (!JS_GetPendingException(aCx, &exn)) {

NS_WARN_IF ?

@@ +401,5 @@
> +    return;
> +  }
> +
> +  JS::Rooted<JS::Value> ignored(aCx);
> +  if (!JS::Call(aCx, JS::UndefinedHandleValue, mReject, JS::HandleValueArray(exn),

NS_WARN_IF ?

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Not putting this in NativeProperties does mean that this won't work from Xrays :-/.

Indeed.  What this means in practice is that over Xrays we'll always end up using the actual constructor we have (for all()/race()) or the canonical Promise constructor (for then()).  But that's ok, because we always want to be working with the canonical Promise constructor there anyway.  Modulo whatever happens with AbortablePromise, I guess.  Not sure whether anyone works with that over Xrays, and in any case the behavior for it would be unchanged from what we have now.

The JS folks need to fix it so JSPropertySpec can story symbol-named stuff.  :(

Comment 21

[Andrea Marchesini [:baku]]

Comment on attachment 8684351 [details] [diff] [review]
part 5.  Implement NewPromiseCapability which can either return a PromiseCapability as in the spec, or one that has a native promise and maybe resolve/reject functions if the consumer asked for them

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

::: dom/promise/Promise.cpp
@@ +35,5 @@
>  #include "PromiseWorkerProxy.h"
>  #include "WorkerPrivate.h"
>  #include "WorkerRunnable.h"
>  #include "xpcpublic.h"
> +#include "WrapperFactory.h"

alphabetic order.

@@ +821,5 @@
>  
> +#define GET_CAPABILITIES_EXECUTOR_RESOLVE_SLOT 0
> +#define GET_CAPABILITIES_EXECUTOR_REJECT_SLOT 1
> +
> +static bool

still unclear to me if we prefer static functions or anonymous namespaces. I let you decide :)

Comment 22

[Andrea Marchesini [:baku]]

Comment on attachment 8684352 [details] [diff] [review]
part 6.  Fix GetDependentPromise to deal with a situation when someone called then() and passed it the resolve/reject functions that come from a promise's constructor

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

::: dom/promise/PromiseCallback.cpp
@@ +9,5 @@
>  #include "mozilla/dom/PromiseNativeHandler.h"
>  
>  #include "jsapi.h"
> +#include "jswrapper.h"
> +#include "jsfriendapi.h"

alphabetic order?

::: dom/promise/PromiseCallback.h
@@ +56,5 @@
>  
>    nsresult Call(JSContext* aCx,
>                  JS::Handle<JS::Value> aValue) override;
>  
> +  Promise* GetDependentPromise() override;

can we make it const?

Comment 23

[Boris Zbarsky [:bzbarsky]]

> { in a new line.

Done.

> explicit ?

Yep.  Static analysis on the try runs caught that.  I think I ended up putting it in part 5, which is wrong.  Good catch.

> do you want to delete also the Move ctor?

Good idea, done.

> NS_WARN_IF ?

I'm not convinced.  The GetPendingException case just returns whether there is an exception; not having one is semi-normal (e.g. slow script dialog).  The other one would return false any time page JS threw an exception under that call, and I'm not sure that calls for a warning...

> alphabetic order.

Done.

> still unclear to me if we prefer static functions or anonymous namespaces

Looks like this file prefers the latter.  I'll switch to that.

> can we make it const?

We probably can.  Followup ok?  It's not really related to this patch, and involves changing all the PromiseCallback impls.

Comment 24

[Andrea Marchesini [:baku]]

Comment on attachment 8684353 [details] [diff] [review]
part 7.  Add subclassing support to Promise::Race

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

::: dom/promise/Promise.cpp
@@ +1389,4 @@
>    }
>  
> +  MOZ_ASSERT(constructor.isObject(), "How did NewPromiseCapability succeed?");
> +  JS::Rooted<JSObject*> constructorObj(cx, &constructor.toObject());

This cannot fail, right?

Comment 25

[Boris Zbarsky [:bzbarsky]]

> This cannot fail, right?

Correct.

Comment 26

[Eric Faust [:efaust]]

Comment on attachment 8684343 [details] [diff] [review]
part 1.  Introduce a PromiseCapability struct

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

I can't speak to NS_WARN_IF or gecko style, but the guts look fine.

::: dom/promise/Promise.cpp
@@ +334,5 @@
> +// PromiseCapability and not set mPromise/mResolve/mReject; the correct
> +// callbacks will be the standard Promise ones, and we don't really want to
> +// synthesize JSFunctions for them in that situation.
> +struct MOZ_STACK_CLASS Promise::PromiseCapability {
> +  PromiseCapability(JSContext* aCx)

Indeed, this will have to be explicit to get past the gremlin of static analysis.

Comment 27

[Peter Van der Beken [:peterv]]

Comment on attachment 8684349 [details] [diff] [review]
part 4.  Change Promise::Constructor to run in the Xray compartment when new Promise happens over Xrays

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

This could use some more explaining in the comments about why exactly we're doing this.

Comment 28

[Andrea Marchesini [:baku]]

Comment on attachment 8684354 [details] [diff] [review]
part 8.  Add subclassing support to Promise::All

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

::: dom/promise/Promise.cpp
@@ +1450,5 @@
> +  // So the plan is as follows: Create the values array in the promise reflector
> +  // compartment.  Create the PromiseAllResolveElement function and the data
> +  // holder in our current compartment.  Store a cross-compartment wrapper to
> +  // the values array in the holder.  This should be OK because the only things
> +  // we hadn the PromiseAllResolveElement function to are the "then" calls we do

hadn ?

Comment 29

[Andrea Marchesini [:baku]]

Comment on attachment 8684355 [details] [diff] [review]
part 9.  Stop using Promise::Resolve in the bindings for PromiseDebugging

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

::: dom/promise/PromiseDebugging.cpp
@@ +85,3 @@
>  {
> +  Promise* promise = UnwrapPromise(aPromise, aRv);
> +  if (aRv.Failed()) {

NS_WARN_IF ?

@@ +142,3 @@
>  {
> +  Promise* promise = UnwrapPromise(aPromise, aRv);
> +  if (aRv.Failed()) {

same here and in the other UnwrapPromise?

::: dom/webidl/PromiseDebugging.webidl
@@ +52,5 @@
>  
>  [ChromeOnly, Exposed=(Window,System)]
>  interface PromiseDebugging {
> +  /**
> +   * The various functions on this interface all expect to take promises but don't want the WebIDL behavior of assimilating random passed-in objects into promises.  They also want to treat Promise subclass instances as promises instead of wrapping them in a vanilla Promise, which is what the IDL spec says to do.  So we list all our arguments as "object" instead of "Promise" and check for them being a Promise internally.

can you indent this comment in multiple lines?

Comment 30

[Andrea Marchesini [:baku]]

Comment on attachment 8684363 [details] [diff] [review]
part 12.  Rip out the promise-resolved-with-promise fast path

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

::: dom/promise/Promise.cpp
@@ +2034,5 @@
> +      //    provides some callback functions to call as arguments to its
> +      //    argument.
> +      //
> +      // Ensuring that stuff while not inside SpiderMonkey is painful, so let's
> +      // drop the fast path for now.

do you want to file a follow up? Otherwise it seems a comment that will stay here for a while..

Comment 31

[Boris Zbarsky [:bzbarsky]]

> This could use some more explaining in the comments about why exactly we're doing this.

Will do.

> hadn ?

"hand", good catch.

> NS_WARN_IF ?

Will do.

> can you indent this comment in multiple lines?

Yep.

> Otherwise it seems a comment that will stay here for a while..

Till is actively working on implementing Promise in SpiderMonkey.  I hope we'll have it done in a few months.  In any case, the followup is bug 911216, effectively.

Comment 32

[Andrea Marchesini [:baku]]

Comment on attachment 8684364 [details] [diff] [review]
part 13.  Add subclassing support to Promise::Then/Catch

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

::: dom/bindings/Codegen.py
@@ +7138,5 @@
>          needsCx = needCx(returnType, arguments, self.extendedAttributes,
>                           not descriptor.interface.isJSImplemented(), static)
>          if needsCx:
>              argsPre.append("cx")
> +            

extra spaces.

::: dom/promise/Promise.cpp
@@ +1117,5 @@
>    promise->MaybeRejectInternal(aCx, aValue);
>    return promise.forget();
>  }
>  
> +static void

In this file we already have an anonymous namespace (actually 2).
Can you put this function there, removing 'static' ?

@@ +1285,5 @@
> +    if (!newPromiseObj) {
> +      // Just throw something.
> +      aRv.ThrowTypeError<MSG_ILLEGAL_PROMISE_CONSTRUCTOR>();
> +      return;
> +    }      

extra spaces.

::: dom/promise/PromiseCallback.cpp
@@ +444,5 @@
> +        mRejectFunc->Call(typeError, &ignored, rejectRv);
> +        // This reported any JS exceptions; we just have a pointless exception
> +        // on there now.
> +        rejectRv.SuppressException();
> +      }        

extra spaces here.

@@ +465,5 @@
> +    // This reported any JS exceptions; we just have a pointless exception
> +    // on there now.
> +    resolveRv.SuppressException();
> +  }    
> +    

and here too.

Comment 33

[Andrea Marchesini [:baku]]

Comment on attachment 8684906 [details] [diff] [review]
Part 10 with more bind() goodness in tests

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

::: dom/bindings/test/TestBindingHeader.h
@@ +1370,5 @@
>  
>    virtual nsISupports* GetParentObject();
>  };
>  
> +class TestInterfaceWithPromiseConstructorArg : public nsISupports, public nsWrapperCache

final

Comment 34

[Boris Zbarsky [:bzbarsky]]

>final

Done, and all the bits from comment 32.

Comment 35

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Add subclassing support to Promise::All

Part 8 but without the weird issues with exception reporting when the index into the array overflows

Comment 36

[Eric Faust [:efaust]]

Comment on attachment 8684351 [details] [diff] [review]
part 5.  Implement NewPromiseCapability which can either return a PromiseCapability as in the spec, or one that has a native promise and maybe resolve/reject functions if the consumer asked for them

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

r=me with comments addressed.

::: dom/promise/Promise.cpp
@@ +821,5 @@
>  
> +#define GET_CAPABILITIES_EXECUTOR_RESOLVE_SLOT 0
> +#define GET_CAPABILITIES_EXECUTOR_REJECT_SLOT 1
> +
> +static bool

I mildly prefer static functions, but maybe I'm just old fashioned.

@@ +832,5 @@
> +  // us.
> +  JS::CallArgs args = CallArgsFromVp(aArgc, aVp);
> +
> +  // Step 1 is an assert.
> +  // Step 2 doesn't need to be done.

My first question when reading this was "Why?". Can we annotate this better? It will not be clear later whether it's a redundant move, like in this case, or some invariant that may or may not still be true. Even though it's no worse than "step 5", it feels worse, somehow.

@@ +889,5 @@
> +    // be in, though; we'll need it later.
> +    JS::Rooted<JSObject*> callerGlobal(aCx, JS::CurrentGlobalOrNull(aCx));
> +    JSAutoCompartment ac(aCx, global);
> +
> +    // Now wrap aConstructor into our compartment, so comparing it to

Be more explicit about which is ours, here. It's the global parameter's compartment, not the one we're currently running in.

@@ +918,5 @@
> +        // compartment comes from xpc::XrayAwareCalleeGlobal.  So we really just
> +        // want to create these functions in the callerGlobal compartment.
> +        MOZ_ASSERT(xpc::WrapperFactory::IsXrayWrapper(&aConstructor.toObject()) ||
> +                   callerGlobal == global);
> +        JSAutoCompartment ac2(aCx, callerGlobal);

mmm, sweet explicit compartment nesting.

@@ +943,5 @@
> +    }
> +  }
> +
> +  // Step 4.
> +  // We can create our get-capabilities function in our current compartment.  It

I would prefer "the invoking compartment" or "the calling compartment" or something similar.

@@ +960,5 @@
> +
> +  JS::Rooted<JSObject*> getCapabilitiesObj(aCx);
> +  getCapabilitiesObj = JS_GetFunctionObject(getCapabilitiesFunc);
> +
> +  // Step 5 doesn't need to be done.

Again, "why"

@@ +981,5 @@
> +  if (!v.isObject() || !JS::IsCallable(&v.toObject())) {
> +    aRv.ThrowTypeError<MSG_PROMISE_RESOLVE_FUNCTION_NOT_CALLABLE>();
> +    return;
> +  }
> +  if (!MaybeWrapObjectValue(aCx, &v)) {

Why do we need to wrap here? If aConstructor->compartment() == aCx->compartment(), then it should be OK as is.

Comment 37

[Boris Zbarsky [:bzbarsky]]

> Can we annotate this better?

Yes.  This now says:

  // Step 2 doesn't need to be done, because it's just giving a name to the
  // PromiseCapability record which is supposed to be stored in an internal
  // slot.  But we don't store that at all, per the comment above; we just
  // directly store its [[Resolve]] and [[Reject]] members.

> Be more explicit about which is ours, here.

    // Now wrap aConstructor into the compartment of aGlobal, so comparing it to
    // the canonical Promise for that compartment actually makes sense.

> I would prefer "the invoking compartment" or "the calling compartment"

Done, "calling compartment".

> Again, "why"

  // Step 5 doesn't need to be done, since we're not actually storing a
  // PromiseCapability in the executor; see the comments in
  // GetCapabilitiesExecutor above.

> Why do we need to wrap here? 

We don't, good catch.  Removed.

Comment 38

[Eric Faust [:efaust]]

Comment on attachment 8684353 [details] [diff] [review]
part 7.  Add subclassing support to Promise::Race

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

r=me

::: dom/promise/Promise.cpp
@@ +1394,5 @@
> +  // After this point we have a useful promise value in "capability", so just go
> +  // ahead and put it in our retval now.  Every single return path below would
> +  // want to do that anyway.
> +  aRetval.set(capability.PromiseValue());
> +  if (!MaybeWrapValue(cx, aRetval)) {

no change required: this will wrap iff capability.mNativePromise, right?

::: testing/web-platform/tests/js/builtins/Promise-subclassing.html
@@ +1,1 @@
> +<!doctype html>

This is gonna burn if they uplift without allowing classes everywhere. Let's hope that doesn't happen :).

Comment 39

[Boris Zbarsky [:bzbarsky]]

> no change required: this will wrap iff capability.mNativePromise, right?

Yes.

> Let's hope that doesn't happen :).

Indeed.  If they do, we'll just mark it as failing on that beta branch at uplift time.

Comment 40

[Eric Faust [:efaust]]

Comment on attachment 8684364 [details] [diff] [review]
part 13.  Add subclassing support to Promise::Then/Catch

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

No test that actually includes a class?

::: dom/promise/Promise.cpp
@@ +1150,5 @@
> +    aRv.ThrowTypeError<MSG_ILLEGAL_PROMISE_CONSTRUCTOR>();
> +    return;
> +  }
> +
> +  // Step 6.

Can't we use the helper from before? Maybe that's less clear, since we have more ministrations to do

@@ +1181,5 @@
> +void
> +Promise::Then(JSContext* aCx, JS::Handle<JSObject*> aCalleeGlobal,
> +              AnyCallback* aResolveCallback, AnyCallback* aRejectCallback,
> +              JS::MutableHandle<JS::Value> aRetval, ErrorResult& aRv)
> +{

Is it invariant here that aCalleeGlobal == GetWrapper()->global()? This seems possible from callers?

@@ +1283,5 @@
> +    // We want to store the reflector itself.
> +    newPromiseObj = js::CheckedUnwrap(newPromiseObj);
> +    if (!newPromiseObj) {
> +      // Just throw something.
> +      aRv.ThrowTypeError<MSG_ILLEGAL_PROMISE_CONSTRUCTOR>();

That error message seems odd...Not some kind of permission denied? What is the exact failure case here?

::: dom/promise/PromiseCallback.cpp
@@ +220,5 @@
> +
> +  ErrorResult rv;
> +  JS::Rooted<JS::Value> ignored(aCx);
> +  mPromiseFunc->Call(value, &ignored, rv);
> +  // Useful exceptions already got reported.

MOZ_STUMBLE_ALONG ;)

Comment 41

[Eric Faust [:efaust]]

Comment on attachment 8684906 [details] [diff] [review]
Part 10 with more bind() goodness in tests

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

r=me with the changed global behavior below.

::: dom/bindings/Codegen.py
@@ +5121,5 @@
> +            #    really doesn't define behavior here because it doesn't define
> +            #    what Realm we're in after the callback returns, which is when
> +            #    the argument conversion happens.  We will use the current
> +            #    compartment, which is the compartment of the callable (which
> +            #    may itself be a cross-compartment wrapper itself), which makes

that seems by far the sanest option.

@@ +5136,5 @@
> +            #    Promise.resolve with our current compartment Promise, because
> +            #    that will wrap it up in a chrome-side Promise, which is
> +            #    decidedly _not_ what's desired here.  So in that case we should
> +            #    really unwrap the return value and use the global of the
> +            #    result.  CheckedUnwrap should be good enough for that; if it

everything after "if it fails" to me makes little sense, as discussed on IRC. the system prinicpal should fail unchecked unwrap for any reason that we need to work around. We can just error out. This needs a corresponding piece of this beefy comment, though.

::: dom/promise/Promise.cpp
@@ +1044,5 @@
> +        aRv.NoteJSContextException();
> +        return;
> +      }
> +
> +      // Cheat instead of calling JS_SameValue, since we know one's an object.

man, that feels dirty. OK. I agree this will work.

Comment 42

[Eric Faust [:efaust]]

Comment on attachment 8684907 [details] [diff] [review]
Part 11 (Promise.rejec) with more bind() goodness in sync code

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

LGTM. Just more of the same from the last patch I reviewed.

Comment 43

[Boris Zbarsky [:bzbarsky]]

> No test that actually includes a class?

As discussed on IRC, the changes to testing/web-platform/tests/js/builtins/Promise-subclassing.html cover this.

> Can't we use the helper from before?

Again as discussed on IRC, SpeciesConstructor has the following behavior for the @@species value:

* null-or-undefined: use default ctor (which may not match what we're getting @@species
  from).
* Anything else that's not a constructible object: throw.
* Use the @@species value

while the helper from before has the following behavior:

* null-or-undefined: use the thing we're getting @@species from
* Anything else that's not a constructible object: throw.
* Use the @@species value

So mainly the null-or-undefined handling is different, I guess, though the handling of a non-object "this" is also different: for SpeciesConstructor it means default constructor, while for the helper it means "throw".

> Is it invariant here that aCalleeGlobal == GetWrapper()->global()?

No, this invariant totally does not hold, even if we ignore Xrays.  If you have:

  window1.Promise.prototype.then.call(window2.Promise.resolve(), stuff)

then aCalleeGlobal is window1 and GetWrappe()->global() is window2.

> What is the exact failure case here?

The failure case is that Promise[Symbol.species] is a Promise constructor from a different origin.  So for example an extension set "contentWindow.Promise.wrappedJSObject[Symbol.species] = Promise" or some such insanity.  Or perhaps a similar thing across two content windows that start same-origin and then set document.domain, though I suspect we would deny calls to the function in that case, whereas we allow calling a chrome function from content, I think.

We could change the exact exception message, I guess.  It's really hard to come up with anything sane to say here other than "STOP THAT".

> MOZ_STUMBLE_ALONG ;)

The point is, any actual exceptions got reported.  "rv" just has a generic NS_ERROR_FAILURE on it if something under there failed.  I could make this more explicit.

Comment 44

[Eric Faust [:efaust]]

Comment on attachment 8691522 [details] [diff] [review]
part 8.  Add subclassing support to Promise::All

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

Looks good to me.

::: dom/promise/Promise.cpp
@@ +1436,5 @@
> +  //
> +  // We have to be very careful about which compartments we create things in
> +  // here.  In particular, we have to maintain the invariant that anything
> +  // stored in a reserved slot is same-compartment with the object whose
> +  // reserved slot it's in.  But we want to create the values array in the

This last sentence that begins "but we want" doesn't parse for me after several attempts. Can we rephrase it in some way?

@@ +1529,5 @@
> +                            JS::UndefinedHandleValue, JSPROP_ENUMERATE)) {
> +        // Have to go back into the caller compartment before we try to touch
> +        // capability.mReject.  Luckily, capability.mReject is guaranteed to be
> +        // an object in the right compartment here.
> +        JSAutoCompartment ac2(cx, &capability.mReject.toObject());

I tried to leave a comment being like "please use a bool instead of an AutoCompartment, and check the bool after leaving ac's scope for this error case", but it's ugly. Plus changing compartments is really cheap, so this is fine.

Comment 45

[Boris Zbarsky [:bzbarsky]]

> everything after "if it fails" to me makes little sens

Yep.  Updated comment:

            #    it fails, then we're failing unwrap while in a
            #    system-privileged compartment, so presumably we have a dead
            #    object wrapper.  Just error out.  Do NOT fall back to using
            #    the current compartment instead: that will return a
            #    system-privileged rejected (because getting .then inside
            #    resolve() failed) Promise to the caller, which they won't be
            #    able to touch.  That's not helpful.  If we error out, on the
            #    other hand, they will get a content-side rejected promise.
            #    Same thing if the value returned is not even an object.

and the corresponding code:

                # Case 4 above.  Note that globalObj defaults to the current
                # compartment global.  Note that we don't use $*{exceptionCode}
                # here because that will try to aRv.Throw(NS_ERROR_FAILURE)
                # which we don't really want here.
                assert exceptionCode == "aRv.Throw(NS_ERROR_UNEXPECTED);\nreturn nullptr;\n"
                getPromiseGlobal = fill(
                    """
                    if (!$${val}.isObject()) {
                      aRv.ThrowTypeError<MSG_NOT_OBJECT>("${sourceDescription}");
                      return nullptr;
                    }
                    JSObject* unwrappedVal = js::CheckedUnwrap(&$${val}.toObject());
                    if (!unwrappedVal) {
                      // A slight lie, but not much of one, for a dead object wrapper.
                      aRv.ThrowTypeError<MSG_NOT_OBJECT>("${sourceDescription}");
                      return nullptr;
                    }
                      
                    globalObj = js::GetGlobalForObjectCrossCompartment(unwrappedVal);
                    """,
                    sourceDescription=sourceDescription)

Comment 46

[Boris Zbarsky [:bzbarsky]]

>Can we rephrase it in some way?

Good catch.  New phrasing:

  // But we want to create the values array in the
  // Promise reflector compartment, because that array can get exposed to code
  // that has access to the Promise reflector (in particular code from that
  // compartment), and that should work, even if the Promise reflector
  // compartment is less-privileged than our caller compartment.

Comment 47

[Andrea Marchesini [:baku]]

Comment on attachment 8684907 [details] [diff] [review]
Part 11 (Promise.rejec) with more bind() goodness in sync code

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

::: dom/promise/Promise.cpp
@@ +1121,5 @@
> +  // Step 3.
> +  PromiseCapability capability(cx);
> +  NewPromiseCapability(cx, global, aThisv, false, capability, aRv);
> +  // Step 4.
> +  if (aRv.Failed()) {

NS_WARN_IF ?

Comment 48

[Boris Zbarsky [:bzbarsky]]

Attachment: Update to Promise.race to not consider @@species

This updates part 7, per https://github.com/tc39/ecma262/pull/211

Comment 49

[Boris Zbarsky [:bzbarsky]]

Attachment: Er, Promise.race without @@species with test changes too

Comment 50

[Boris Zbarsky [:bzbarsky]]

Attachment: Similar update to Promise.all for no @@species

Comment 51

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d79cce3630a
https://hg.mozilla.org/integration/mozilla-inbound/rev/39e4f5b1ba40
https://hg.mozilla.org/integration/mozilla-inbound/rev/cb6aba9b8497
https://hg.mozilla.org/integration/mozilla-inbound/rev/b312a08fbab5
https://hg.mozilla.org/integration/mozilla-inbound/rev/dc2a7f5dc5d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c2c1acc34ee
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee514a256922
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5b7dfab882
https://hg.mozilla.org/integration/mozilla-inbound/rev/541831dc6b57
https://hg.mozilla.org/integration/mozilla-inbound/rev/f380faddfdd8
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dfb229da01d
https://hg.mozilla.org/integration/mozilla-inbound/rev/6104fe33d5f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d84599a8846

Comment 52

[Phil Ringnalda (:philor)]

That was a very nice shrubbery. Now, we want a Gu-fix (https://treeherder.mozilla.org/logviewer.html#?job_id=17842969&repo=mozilla-inbound or https://treeherder.mozilla.org/logviewer.html#?job_id=17842429&repo=mozilla-inbound if you prefer it Mulet-flavored), or we shall say "Backout" again.

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/08f527d65282

Comment 53

[Boris Zbarsky [:bzbarsky]]

Looks like someone added some more broken gaia stuff since the last time I did a try run including Gu (which was green at the time).

Comment 54

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/86e73c5414d6
https://hg.mozilla.org/integration/mozilla-inbound/rev/b8bdcbadd22b
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fbe06bfeba5
https://hg.mozilla.org/integration/mozilla-inbound/rev/2b32666a5279
https://hg.mozilla.org/integration/mozilla-inbound/rev/57a6ec978c79
https://hg.mozilla.org/integration/mozilla-inbound/rev/0eeec5eed51b
https://hg.mozilla.org/integration/mozilla-inbound/rev/694d3b2752a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/fed3e6ac1aff
https://hg.mozilla.org/integration/mozilla-inbound/rev/d282d800811e
https://hg.mozilla.org/integration/mozilla-inbound/rev/f540f2da98a5
https://hg.mozilla.org/integration/mozilla-inbound/rev/28e2db073b65
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e144edfd1d8
https://hg.mozilla.org/integration/mozilla-inbound/rev/79478e472e28

Comment 55

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/86e73c5414d6
https://hg.mozilla.org/mozilla-central/rev/b8bdcbadd22b
https://hg.mozilla.org/mozilla-central/rev/6fbe06bfeba5
https://hg.mozilla.org/mozilla-central/rev/2b32666a5279
https://hg.mozilla.org/mozilla-central/rev/57a6ec978c79
https://hg.mozilla.org/mozilla-central/rev/0eeec5eed51b
https://hg.mozilla.org/mozilla-central/rev/694d3b2752a7
https://hg.mozilla.org/mozilla-central/rev/fed3e6ac1aff
https://hg.mozilla.org/mozilla-central/rev/d282d800811e
https://hg.mozilla.org/mozilla-central/rev/f540f2da98a5
https://hg.mozilla.org/mozilla-central/rev/28e2db073b65
https://hg.mozilla.org/mozilla-central/rev/1e144edfd1d8
https://hg.mozilla.org/mozilla-central/rev/79478e472e28

Comment 56

[Eric Faust [:efaust]]

Comment on attachment 8692116 [details] [diff] [review]
Er, Promise.race without @@species with test changes too

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

Silly, but true.

Comment 57

[Eric Faust [:efaust]]

Comment on attachment 8692119 [details] [diff] [review]
Similar update to Promise.all for no @@species

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

"For completeness"