1243001 - Implement DOM support for moving Promise into SpiderMonkey

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

This is the DOM side of bug 911216.

The general plan of attack is the following.  We're going to introduce a SPIDERMONKEY_PROMISE compile-time define which we can use to turn on SpiderMonkey promises.  We can also use it to turn them off (e.g. on Aurora/Beta) if it turns out there are problems.  For a bit we'll support both modes of operation, by conditioning parts of our Promise impl on this define.  Once we've successfully shipped SpiderMonkey promises, we will remove the dead #ifndef SPIDERMONKEY_PROMISE code and probably do a bit more simplification of the Promise bits after that.

An alternative would be to have a totally separate Promise.h/Promise.cpp if SPIDERMONKEY_PROMISE is defined instead of doing ifdefs in a single file.  I could do that if people prefer; in the end it turned out that the two Promise versions don't share _that_ much with each other....  There might be a bit more complication with vcs blame this way, of course.  Just let me know.

Anyway, the new setup generally works like this: a dom::Promise just points to a SpiderMonkey Promise and delegates everything to it.  Creation of a dom::Promise either requires passing in an existing SpiderMonkey Promise or cross-compartment wrapper for one, or will go ahead and create a SpiderMonkey Promise using the nsIGlobalObject dom::Promise is created with.  dom::Promise is not wrappercached (because a SpiderMonkey Promise doesn't know how to notify it when it gets moved); it just uses a Heap<> to store the SpiderMonkey Promise.  The C++-facing API that people actually use remains, so that changes outside of dom/bindings and dom/promise there are not really needed.

On the Web IDL side, we keep representing Promise as an interface, but make it NoInterfaceObject.  When passing Promise arguments, we do the Promise::Resolve thing the spec calls for, which gives us a dom::Promise wrapping stuff up.  When doing Promise return values, we take the non-wrappercached object codepath and just hack dom::Promise::WrapObject to return its already-existing SpiderMonkey Promise.

Note that this means that dom::Promise object identity is NOT preserved by a roundtrip through JS.  That seems OK to me, honestly; that's basically how IDL callbacks work.

The goal of this bug is to land all the relevant DOM code but all behind SPIDERMONKEY_PROMISE ifdefs.  There should therefore be no behavior changes.  Once the SpiderMonkey parts of this are also done we'll take a look to see whether we have any try failures.

Till, I assume the relevant SpiderMonkey APIs will end up with clear documentation as to whether they can take references to cross-compartment wrappers to SpiderMonkey Promise objects and how they handle dead object wrappers and so forth.  For now I'm assuming they just do sane things: accept cross-compartment wrappers, CheckedUnwrap, if that fails throw.  If the result of the unwrap is not a SpiderMonkey Promise, I'm not sure whether we should throw or just assert...  If that assumption turns out to be false, I can update these patches as needed.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Remove the dead WrappedWorkerRunnable class from Promise code

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Make Promise an empty [NoInterfaceObject] interface when SPIDERMONKEY_PROMISE is defined

The idea is to not define a "Promise" property on the global and not generate
any of the methods, since SpiderMonkey will implement all of those, but to keep
some of the conversion to/from JS logic and the IDL parser validation bits that
we have right now.  Once we can assume SPIDERMONKEY_PROMISE we can probably
change how the "Promise" identifier is handled by the IDL parser and how the
resulting type is handled by codegen, but for now we're aiming for minimal
changes.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Turn off the IDL bits of PromiseDebugging when SPIDERMONKEY_PROMISE is defined

These are basically the minimal changes needed to make PromiseDebugging compile
in the new world.  It will NOT function correctly (see the XXX comments); Till
will be fixing it up more in bug 911216 as he transitions the relevant bits in
our devtools to work on top of SpiderMonkey promises.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Switch to using MaybeResolve/MaybeReject instead of ResolveInternal/RejectInternal for PromiseWorkerProxy

This is the one part of this set of patches that is actually a substantive change
even without SPIDERMONKEY_PROMISE defined.  This is being done because I don't
want to create ResolveInternal/RejectInternal methods on dom::Promise in the
new world.

In practice, the difference between MaybeResolve/Reject and
ResolveInternal/RejectInternal is that the former will do nothing if the
Promise is "resolved" in terms of spec terminology: either settled or locked in
to track another Promise.  A resolved but still pending Promise can still get
fulfilled (what we call ResolveInternal) or rejected when the promise it's
tracking settles.

So the difference only matters if PromiseWorkerProxy can be working with a
"resolved" Promise (in which case what it's doing now would settle it, while
what I'm switching to would not).  But I don't believe PromiseWorkerProxy ever
points to a "resolved" Promise.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Get rid of most of the dom::Promise methods when SPIDERMONKEY_PROMISE is defined, and reimplement the rest in terms of SpiderMonkey Promise

This is the bulk of the new dom::Promise implementation.  It's using the APIs
that bug 911216 part 1 adds.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Implement Promise::AppendNativeHandler in the SPIDERMONKEY_PROMISE world

This patch introduces a kinda fake IDL interface just to get the benefits of
cycle collection for the JS-to-C++ link we now need for PromiseNativeHandler
(because the SpiderMonkey Promise somehow needs to point to the
PromiseNativeHandler).  Now in practice a bunch of our PromiseNativeHandlers
are not cycle collected.  That kinda freaks me out, but spot-checking a few
suggests they do not in fact leak (either because they don't form cycles or
because the Promise they're observing always settles and then releases them).
Either way, that's a problem that exists with or without this patch...

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Stop wrappercaching dom::Promise when SPIDERMONKEY_PROMISE is defined

nsWrapperCache expects the object it stores to have an ObjectMoved op that will
notify the wrapper cache when the object is moved.  SpiderMonkey promises don't
have a way to do this.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Tell SpiderMonkey to put its promise jobs into the CycleCollectedJSRuntime job queue

This will run the SpiderMonkey promise jobs more or less the same way that we
run Promise jobs right now, including using a Web IDL callback for the actual
invocation.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8712210 [details] [diff] [review]
part 7.  Stop wrappercaching dom::Promise when SPIDERMONKEY_PROMISE is defined

Oh, I just realized that to make this work we'll also need to fix all the IDL bits that return Promise via nsISupports.  Right now that's relying on XPConnect QIing to nsWrapperCache and calling the virtual WrapObject(), but that won't work anymore...

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 7 with a bit more XPCConvert hackery to keep Promise-as-nsISupports working

Comment 11

[Boris Zbarsky [:bzbarsky]]

I guess one open question is whether a dom::Promise should always store a JS Promise directly or whether it's OK for it to store a cross-compartment wrapper for one.  This only matters for Promise::Resolve/Reject called over Xrays, I believe; in all other cases we wouldn't have a cross-compartment wrapper to start with...  So we can sort that out once we have the Xray setup in place and I see how it works.

The other thing that needs a bit of clarification is whether the JSContext and JSValue passed to CallOriginalPromiseResolve/Reject are meant to be same-compartment or not.  Again, this matters only for the Xray case.

Comment 12

[Peter Van der Beken [:peterv]]

Comment on attachment 8712193 [details] [diff] [review]
part 3.  Turn off the IDL bits of PromiseDebugging when SPIDERMONKEY_PROMISE is defined

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

Hmm, this seems a bit useless without the bits from bug 911216.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> Hmm, this seems a bit useless without the bits from bug 911216.

Sure.  Everything in this bug is useless without those bits; the later patches don't even compile without them.

That's why we're not going to flip the define until this is all landed and bug 911216 is landed.  We could try to do them all in one bug, but it's a bit simpler to just get this stuff in behind the ifdef and then we can do try runs with the patches from bug 911216...

Comment 14

[Boris Zbarsky [:bzbarsky]]

> the later patches don't even compile without them

I mean if SPIDERMONKEY_PROMISE is actually defined.  Without that they compile, of course.  :)

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 5 with the right API used on the AutoObjectVector to reserve space

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 7 with a ToJSValue overload for Promise so we don't try to xpconnect-wrap it on worker threads

Comment 17

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8712725 [details] [diff] [review]
Part 5 with the right API used on the AutoObjectVector to reserve space

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

::: dom/bindings/Codegen.py
@@ +5194,5 @@
> +                  if (!global) {
> +                    promiseRv.Throw(NS_ERROR_UNEXPECTED);
> +                    promiseRv.MaybeSetPendingException(cx);
> +                    $*{exceptionCode}
> +                  }                  

ws

Comment 18

[Boris Zbarsky [:bzbarsky]]

> ws

Fixed.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 6 without the extra object

Comment 20

[Boris Zbarsky [:bzbarsky]]

Attachment: Part 7 with rookie compartment mismatch mistake fixed

Comment 21

[Peter Van der Beken [:peterv]]

Comment on attachment 8712725 [details] [diff] [review]
Part 5 with the right API used on the AutoObjectVector to reserve space

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

::: dom/promise/Promise.cpp
@@ +565,5 @@
> +    aRv.NoteJSContextException();
> +    return nullptr;
> +  }
> +
> +  for (auto promise : aPromiseList) {

I think I'd prefer auto& here.

Comment 22

[Peter Van der Beken [:peterv]]

Comment on attachment 8715963 [details] [diff] [review]
Part 6 without the extra object

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

::: dom/promise/Promise.cpp
@@ +792,5 @@
> +  if (NS_WARN_IF(!rejectFunc)) {
> +    jsapi.ClearException();
> +    return;
> +  }
> +  

Whitespace.

Comment 23

[Boris Zbarsky [:bzbarsky]]

> I think I'd prefer auto& here.

Done.

> Whitespace.

Fixed.

Comment 24

[Till Schneidereit [:till]]

Attachment: Part 9: Fix Promise usage in nsDOMWindowUtils::GetSupportsHardwareH264Decoding

This was added in bug 1212323.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8717473 [details] [diff] [review]
Part 9: Fix Promise usage in nsDOMWindowUtils::GetSupportsHardwareH264Decoding

r=me but I already have that rolled into part 7 locally anyway.  I guess that hasn't been reviewed yet, so I should just post an updated version....

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Stop wrappercaching dom::Promise when SPIDERMONKEY_PROMISE is defined

nsWrapperCache expects the object it stores to have an ObjectMoved op that will
notify the wrapper cache when the object is moved.  SpiderMonkey promises don't
have a way to do this.

The XPCConvert changes are needed to allow code that passes around Promise
objects as nsISupports to continue working instead of ending up with
double-wrapped nsISupports (XPCWrappedNative for an nsISupports XPCWrappedJS)
around the SpiderMonkey Promise.

Comment 27

[Peter Van der Beken [:peterv]]

Comment on attachment 8717486 [details] [diff] [review]
part 7.  Stop wrappercaching dom::Promise when SPIDERMONKEY_PROMISE is defined

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

The need for the XPCConvert changes is unfortunate :-(.

::: dom/bindings/BindingUtils.h
@@ +1151,5 @@
>    return WrapNewBindingNonWrapperCachedObject(cx, scope, value.get(), rval,
>                                                givenProto);
>  }
>  
> +// Helper for object references (as opposed to pointers.

... pointers).

Comment 28

[Boris Zbarsky [:bzbarsky]]

> The need for the XPCConvert changes is unfortunate :-(.

We can try to follow up with making Promise not implement nsISupports and hence catching at least the C++ places that rely on those codepaths... but it's a pretty risky project.  :(

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/350b09f4cc2b
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb8f560ba9fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/c945faa259ff
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb43bd69d282
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8cfb5df18c3
https://hg.mozilla.org/integration/mozilla-inbound/rev/863f5c3d64a0
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a52f75bbf6b
https://hg.mozilla.org/integration/mozilla-inbound/rev/dae2ed7930f4

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/350b09f4cc2b
https://hg.mozilla.org/mozilla-central/rev/eb8f560ba9fa
https://hg.mozilla.org/mozilla-central/rev/c945faa259ff
https://hg.mozilla.org/mozilla-central/rev/bb43bd69d282
https://hg.mozilla.org/mozilla-central/rev/d8cfb5df18c3
https://hg.mozilla.org/mozilla-central/rev/863f5c3d64a0
https://hg.mozilla.org/mozilla-central/rev/5a52f75bbf6b
https://hg.mozilla.org/mozilla-central/rev/dae2ed7930f4