Implement Promise subclassing properly

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Tracking

Trunk
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 affected, firefox45 fixed)

Details

Attachments

(15 attachments, 5 obsolete attachments)

5.34 KB, patch
efaust
: review+
baku
: review+
Details | Diff | Splinter Review
10.83 KB, patch
peterv
: review+
Details | Diff | Splinter Review
5.64 KB, patch
peterv
: review+
Details | Diff | Splinter Review
4.33 KB, patch
peterv
: review+
Details | Diff | Splinter Review
11.79 KB, patch
baku
: review+
efaust
: review+
Details | Diff | Splinter Review
7.80 KB, patch
baku
: review+
Details | Diff | Splinter Review
20.82 KB, patch
baku
: review+
efaust
: review+
Details | Diff | Splinter Review
15.78 KB, patch
baku
: review+
Details | Diff | Splinter Review
10.25 KB, patch
baku
: review+
Details | Diff | Splinter Review
41.25 KB, patch
baku
: review+
efaust
: review+
Details | Diff | Splinter Review
24.11 KB, patch
baku
: review+
efaust
: review+
Details | Diff | Splinter Review
11.34 KB, patch
baku
: review+
efaust
: review+
Details | Diff | Splinter Review
19.64 KB, patch
efaust
: review+
Details | Diff | Splinter Review
4.06 KB, patch
efaust
: feedback+
Details | Diff | Splinter Review
2.73 KB, patch
efaust
: feedback+
Details | Diff | Splinter Review
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...
(Assignee)

Updated

3 years ago
Flags: needinfo?(bzbarsky)
(Assignee)

Updated

3 years ago
Depends on: 1219749
(Assignee)

Comment 1

3 years ago
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.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 2

3 years ago
Created attachment 8684343 [details] [diff] [review]
part 1.  Introduce a PromiseCapability struct
Attachment #8684343 - Flags: review?(efaustbmo)
Attachment #8684343 - Flags: review?(amarchesini)
(Assignee)

Updated

3 years ago
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
(Assignee)

Comment 3

3 years ago
Created attachment 8684345 [details] [diff] [review]
part 2.  Pass in the 'this' value to Promise static methods
Attachment #8684345 - Flags: review?(peterv)
(Assignee)

Comment 4

3 years ago
Created attachment 8684346 [details] [diff] [review]
part 2.  Pass in the 'this' value to Promise static methods
Attachment #8684346 - Flags: review?(peterv)
(Assignee)

Updated

3 years ago
Attachment #8684345 - Attachment is obsolete: true
Attachment #8684345 - Flags: review?(peterv)
(Assignee)

Comment 5

3 years ago
Created attachment 8684347 [details] [diff] [review]
part 3.  Add an @@species getter on Promise
Attachment #8684347 - Flags: review?(peterv)
(Assignee)

Comment 6

3 years ago
Created attachment 8684349 [details] [diff] [review]
part 4.  Change Promise::Constructor to run in the Xray compartment when new Promise happens over Xrays
Attachment #8684349 - Flags: review?(peterv)
(Assignee)

Comment 7

3 years ago
Created 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
Attachment #8684351 - Flags: review?(efaustbmo)
Attachment #8684351 - Flags: review?(amarchesini)
(Assignee)

Comment 8

3 years ago
Created 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
Attachment #8684352 - Flags: review?(amarchesini)
(Assignee)

Comment 9

3 years ago
Created attachment 8684353 [details] [diff] [review]
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.
Attachment #8684353 - Flags: review?(efaustbmo)
Attachment #8684353 - Flags: review?(amarchesini)
Created attachment 8684354 [details] [diff] [review]
part 8.  Add subclassing support to Promise::All
Attachment #8684354 - Flags: review?(efaustbmo)
Attachment #8684354 - Flags: review?(amarchesini)
Created attachment 8684355 [details] [diff] [review]
part 9.  Stop using Promise::Resolve in the bindings for PromiseDebugging
Attachment #8684355 - Flags: review?(amarchesini)
Created attachment 8684356 [details] [diff] [review]
part 10.  Add subclassing support to Promise::Resolve
Attachment #8684356 - Flags: review?(efaustbmo)
Attachment #8684356 - Flags: review?(amarchesini)
Created attachment 8684359 [details] [diff] [review]
part 11.  Add subclassing support to Promise::Reject
Attachment #8684359 - Flags: review?(efaustbmo)
Attachment #8684359 - Flags: review?(amarchesini)
Created attachment 8684363 [details] [diff] [review]
part 12.  Rip out the promise-resolved-with-promise fast path
Attachment #8684363 - Flags: review?(amarchesini)
Created attachment 8684364 [details] [diff] [review]
part 13.  Add subclassing support to Promise::Then/Catch
Attachment #8684364 - Flags: review?(efaustbmo)
Attachment #8684364 - Flags: review?(amarchesini)
Created attachment 8684906 [details] [diff] [review]
Part 10 with more bind() goodness in tests
Attachment #8684906 - Flags: review?(efaustbmo)
Attachment #8684906 - Flags: review?(amarchesini)
(Assignee)

Updated

3 years ago
Attachment #8684356 - Attachment is obsolete: true
Attachment #8684356 - Flags: review?(efaustbmo)
Attachment #8684356 - Flags: review?(amarchesini)
Created attachment 8684907 [details] [diff] [review]
Part 11 (Promise.rejec) with more bind() goodness in sync code
Attachment #8684907 - Flags: review?(efaustbmo)
Attachment #8684907 - Flags: review?(amarchesini)
(Assignee)

Updated

3 years ago
Attachment #8684359 - Attachment is obsolete: true
Attachment #8684359 - Flags: review?(efaustbmo)
Attachment #8684359 - Flags: review?(amarchesini)
(Assignee)

Updated

3 years ago
Depends on: 1223004
Attachment #8684346 - Flags: review?(peterv) → review+
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 :-/.
Attachment #8684347 - Flags: review?(peterv) → review+
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 ?
Attachment #8684343 - Flags: review?(amarchesini) → review+
> 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 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 :)
Attachment #8684351 - Flags: review?(amarchesini) → review+
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?
Attachment #8684352 - Flags: review?(amarchesini) → review+
> { 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 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?
Attachment #8684353 - Flags: review?(amarchesini) → review+
> This cannot fail, right?

Correct.
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.
Attachment #8684343 - Flags: review?(efaustbmo) → review+
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.
Attachment #8684349 - Flags: review?(peterv) → review+
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 ?
Attachment #8684354 - Flags: review?(efaustbmo) → review+
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?
Attachment #8684355 - Flags: review?(amarchesini) → review+
Attachment #8684354 - Flags: review?(amarchesini) → review?(efaustbmo)
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..
Attachment #8684363 - Flags: review?(amarchesini) → review+
> 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 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.
Attachment #8684364 - Flags: review?(amarchesini) → review+
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
Attachment #8684906 - Flags: review?(amarchesini) → review+
>final

Done, and all the bits from comment 32.
Created attachment 8691522 [details] [diff] [review]
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
Attachment #8691522 - Flags: review?(efaustbmo)
(Assignee)

Updated

3 years ago
Attachment #8684354 - Attachment is obsolete: true
Attachment #8684354 - Flags: review?(efaustbmo)
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.
Attachment #8684351 - Flags: review?(efaustbmo) → review+
> 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 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 :).
Attachment #8684353 - Flags: review?(efaustbmo) → review+
> 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 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 ;)
Attachment #8684364 - Flags: review?(efaustbmo) → review+
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.
Attachment #8684906 - Flags: review?(efaustbmo) → review+
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.
Attachment #8684907 - Flags: review?(efaustbmo) → review+
> 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 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.
Attachment #8691522 - Flags: review?(efaustbmo) → review+
> 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)
>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 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 ?
Attachment #8684907 - Flags: review?(amarchesini) → review+
(Assignee)

Updated

3 years ago
Depends on: 1228053
Created attachment 8692115 [details] [diff] [review]
Update to Promise.race to not consider @@species

This updates part 7, per https://github.com/tc39/ecma262/pull/211
Attachment #8692115 - Flags: feedback?(efaustbmo)
Created attachment 8692116 [details] [diff] [review]
Er, Promise.race without @@species with test changes too
Attachment #8692115 - Attachment is obsolete: true
Attachment #8692115 - Flags: feedback?(efaustbmo)
Attachment #8692116 - Flags: feedback?(efaustbmo)
Created attachment 8692119 [details] [diff] [review]
Similar update to Promise.all for no @@species
Attachment #8692119 - Flags: feedback?(efaustbmo)
(Assignee)

Updated

3 years ago
Depends on: 1228239
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 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.
Attachment #8692116 - Flags: feedback?(efaustbmo) → feedback+
Comment on attachment 8692119 [details] [diff] [review]
Similar update to Promise.all for no @@species

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

"For completeness"
Attachment #8692119 - Flags: feedback?(efaustbmo) → feedback+
(Assignee)

Updated

3 years ago
Duplicate of this bug: 1245936
(Assignee)

Updated

2 years ago
Blocks: 1185964
You need to log in before you can comment on or make changes to this bug.