Implement promises in the JavaScript engine

RESOLVED FIXED in Firefox 50

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
a year ago

People

(Reporter: annevk, Assigned: till)

Tracking

(Blocks: 3 bugs)

Trunk
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(44 attachments, 40 obsolete attachments)

2.63 KB, patch
Details | Diff | Splinter Review
4.04 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.70 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
1.27 KB, patch
efaust
: review+
Details | Diff | Splinter Review
5.80 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.86 KB, patch
efaust
: review+
Details | Diff | Splinter Review
104.19 KB, patch
efaust
: review+
Details | Diff | Splinter Review
8.89 KB, patch
shu
: review+
Details | Diff | Splinter Review
9.00 KB, patch
efaust
: review+
Details | Diff | Splinter Review
9.29 KB, patch
till
: review+
Details | Diff | Splinter Review
50.78 KB, patch
till
: review+
Details | Diff | Splinter Review
33.24 KB, patch
efaust
: review+
Details | Diff | Splinter Review
7.56 KB, patch
Details | Diff | Splinter Review
5.18 KB, patch
Details | Diff | Splinter Review
2.44 KB, patch
Details | Diff | Splinter Review
3.44 KB, patch
Details | Diff | Splinter Review
7.12 KB, patch
Details | Diff | Splinter Review
880 bytes, patch
Details | Diff | Splinter Review
3.06 KB, patch
Details | Diff | Splinter Review
1.67 KB, patch
Details | Diff | Splinter Review
4.52 KB, patch
tromey
: review+
Details | Diff | Splinter Review
2.29 KB, patch
shu
: review+
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
chmanchester
: review+
Details | Review
7.76 KB, patch
efaust
: review+
Details | Diff | Splinter Review
16.53 KB, patch
shu
: review+
Details | Diff | Splinter Review
5.18 KB, patch
efaust
: review+
Details | Diff | Splinter Review
10.08 KB, patch
efaust
: review+
Details | Diff | Splinter Review
9.74 KB, patch
Details | Diff | Splinter Review
15.52 KB, patch
efaust
: review+
Details | Diff | Splinter Review
5.52 KB, patch
bholley
: review+
Details | Diff | Splinter Review
21.73 KB, patch
Details | Diff | Splinter Review
4.28 KB, patch
shu
: review+
Details | Diff | Splinter Review
2.39 KB, patch
efaust
: review+
Details | Diff | Splinter Review
1.02 KB, text/plain
Details
1.13 KB, text/plain
Details
3.03 KB, patch
Yoric
: feedback+
Details | Diff | Splinter Review
1.16 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
5.97 KB, patch
efaust
: review+
Details | Diff | Splinter Review
1.87 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
38.48 KB, patch
efaust
: review+
Details | Diff | Splinter Review
2.38 KB, patch
efaust
: review+
Details | Diff | Splinter Review
10.05 KB, patch
ejpbruel
: review+
Details | Diff | Splinter Review
1.13 KB, patch
till
: review+
Details | Diff | Splinter Review
36.27 KB, patch
till
: review+
Details | Diff | Splinter Review
(Reporter)

Description

5 years ago
https://github.com/domenic/promises-unwrapping/blob/master/README.md is the upcoming replacement for the DOM specification on promises. It's highly likely it'll be declared the way to do promises two and a half week from now.

We should consider implementing it in the JavaScript engine directly, as that's where promises need to be.
(Reporter)

Updated

5 years ago
Duplicate of this bug: 879245
Keywords: dev-doc-needed
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
Created attachment 801245 [details] [diff] [review]
WIP AP2 style promises.

Initial implementation. This self-hosting stuff is cool!

I haven't comprehensively tested this, just played around with simple scripts.

One glaring problem: I have no idea how to queue a task in pure Spidermonkey. Right now it is called synchronously, which is wrong.
Assignee: nobody → nsm.nikhil
Created attachment 801252 [details] [diff] [review]
WIP AP2 style promises.

Added most assertions.
Comment on attachment 801252 [details] [diff] [review]
WIP AP2 style promises.

Nicolas,

I'd appreciate a cursory glance over the self hosting and if I hoist any more stuff to JS.

Anne,

I've followed the spec algorithmically, but the semantics of UpdateDerived() and co. are a little tricky, so if you could go over that.
Attachment #801252 - Flags: feedback?(nmatsakis)
Attachment #801252 - Flags: feedback?(annevk)

Comment 5

5 years ago
Nikhil: if you come across anything that's unclear or could be simplified in the spec, feel free to open a GitHub issue. I admit it did come out less simple than I was hoping. Spec writing isn't as easy as Allen and Anne make it look!

Also, you guys may find the existing sample implementation and test suite to be useful:

Implementation: https://github.com/domenic/promises-unwrapping/blob/master/testable-implementation.js

Test suite: https://github.com/domenic/promises-unwrapping/blob/master/run-tests.js, which is additive to https://github.com/promises-aplus/promises-tests/tree/spec-1.1

I am happy to spend some time (perhaps after JSConf EU though) converting the test suite into a more SpiderMonkey-friendly or even test262-friendly format.

Comment 6

5 years ago
Not sure if I am reading this correctly, but the WIP patch seems to use normal user-readable properties, not internal properties, for all of the specified-internal properties. Users should not be able to read or modify the internal properties.
(In reply to Domenic Denicola from comment #6)
> Not sure if I am reading this correctly, but the WIP patch seems to use
> normal user-readable properties, not internal properties, for all of the
> specified-internal properties. Users should not be able to read or modify
> the internal properties.

I was hoping the self hosted JS would automatically hide these from 'web' JS, but it doesn't seem to be the case. I'll update the patch soon.
Created attachment 801347 [details] [diff] [review]
WIP AP2 style promises.

Set internal properties in slots.
Attachment #801252 - Attachment is obsolete: true
Attachment #801252 - Flags: feedback?(nmatsakis)
Attachment #801252 - Flags: feedback?(annevk)
Attachment #801347 - Flags: feedback?(nmatsakis)
Attachment #801347 - Flags: feedback?(domenic)
(In reply to Domenic Denicola from comment #5)

> I am happy to spend some time (perhaps after JSConf EU though) converting
> the test suite into a more SpiderMonkey-friendly or even test262-friendly
> format.

That would be nice!
If this is done, what is the path forward for DOM APIs that need to return a Promise?  I see nothing in this patch that allows such an API to be implemented.

Furthermore, what's the story on Xrays here?  Seems like without that, any DOM API returning promises must not be used from extensions or chrome...
For that matter, what's the story here for DOM APIs that _take_ a Promise?

It sounds like we'll need to add some hackarounds similar to what we did for typed arrays, but that requires APIs for testing whether an object is a Promise and APIs for manipulating them.
(In reply to Boris Zbarsky [:bz] from comment #10)
> If this is done, what is the path forward for DOM APIs that need to return a
> Promise?  I see nothing in this patch that allows such an API to be
> implemented.
> 
> Furthermore, what's the story on Xrays here?  Seems like without that, any
> DOM API returning promises must not be used from extensions or chrome...

I don't know anything about Xray implementation and how it would fit into this, pointers are appreciated.
(In reply to Boris Zbarsky [:bz] from comment #11)
> For that matter, what's the story here for DOM APIs that _take_ a Promise?
> 
> It sounds like we'll need to add some hackarounds similar to what we did for
> typed arrays, but that requires APIs for testing whether an object is a
> Promise and APIs for manipulating them.

Adding C++ APIs should be trivial to do. I'll come up with something in a few days. Meanwhile, if anyone wants to hack on this, go ahead.
The Xray question was for bholley.
Flags: needinfo?(bobbyholley+bmo)
If we implemented promises in the JS engine, then we'll almost certainly need some sort of Xray traits for them. There are a number of other JS-y things that we have no good Xray story for right now - typed arrays and global constructors, to name a couple.

The problem is that, for this stuff to sanely pass through the bindings in all the relevant situations, we're going to need a sane C++ representation of a Promise that isn't just a JSObject*.

(In reply to Anne (:annevk) from comment #0)
> We should consider implementing it in the JavaScript engine directly, as
> that's where promises need to be.

Can you elaborate on that?
Flags: needinfo?(bobbyholley+bmo)

Comment 16

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #11)
> For that matter, what's the story here for DOM APIs that _take_ a Promise?

Only tangentially related, since I realize you were asking about technical implementation aspects and not the general story, but https://github.com/domenic/promises-unwrapping/blob/master/writing-specifications-with-promises.md#guidance explains how such promise-taking or promise-returning APIs *should* work. Maybe that will inform implementation decisions.
(Reporter)

Comment 17

5 years ago
The plan has always been for promises to end up as a JavaScript type and become an integral part of the language. E.g. for dedicated support of asynchronous iterators (iirc).
(In reply to Anne (:annevk) from comment #17)
> The plan has always been for promises to end up as a JavaScript type and
> become an integral part of the language. E.g. for dedicated support of
> asynchronous iterators (iirc).

Ok. So if we do that, we're going to need two things:

(1) A C++ representation of the promise that we can pass around to anyone who wants to manipulate such objects, so that they don't have to use manual JSAPI, much like we have for TypedArrays.

(2) Some sort of Xray traits to handle them appropriately.

Both of these are probably going to want some sort of hooks into the more stable "C++ guts" of the objects, bypassing expandos and whatnot. We may want to build some kind of general mechanism into SM for these sorts of things. Does that sound right, Boris? If so, I can do some brainstorming with the spidermonks.
Flags: needinfo?(bzbarsky)
Yep, that sounds right.
Flags: needinfo?(bzbarsky)
I just spoke to bill about for a while about this. We brainstormed how to do Xrays for JS objects, but he also pointed out that our JSM-implemented promises make use of the event loop (to prevent .then().then() from ever hitting recursion limits), which would make it difficult to implement in the JS engine, since ES has no notion of an event loop.

Do DOM Promises do the same thing?
Flags: needinfo?(annevk)
https://github.com/domenic/promises-unwrapping talks about microtasks.
(not clear to me why we'd want microtasks in this case)
Ok, then it sounds like this is not ES-compatible.
(Reporter)

Comment 24

5 years ago
Yes, interaction with the event loop is needed, see also https://www.w3.org/Bugs/Public/show_bug.cgi?id=22296 for which it'll be microtasks if you're interested.

JavaScript will have to get some way to interact with the event loop. Object.observe() needs it, modules will need it, some other proposed ES7 features will need it.
Flags: needinfo?(annevk)

Comment 25

5 years ago
Hey Nikhil, what are your plans about reconciling this implementation with the one done by Andrea in dom/promise?
Flags: needinfo?(nsm.nikhil)
I'm not sure, since without a 'event loop' this implementation is quite useless right now. It seems to me that our dom/promise implementation is relatively spec compliant with the AP2 style, apart from the |function(PromiseResolver) {}| vs |function(resolve, reject) {}| and using |Promise(...)| rather than |new Promise(...)| to create a promise. Am I right Anne?

I propose we modify Andrea's implementation to support these two changes while the number of users in the codebase is still small. We keep this implementation around until the JS engine microtasks are implemented, or the JS implementation interacts with the Gecko event loop. That way we can remove dom/promise in the future without breaking everything. Does this sound acceptable?
Flags: needinfo?(nsm.nikhil) → needinfo?(annevk)
I meant that JS code in the tree won't have to change. C++ code using Promises may need changes depending on the C++ API for this Spidermonkey promise implementation.
> apart from the |function(PromiseResolver) {}| vs |function(resolve, reject) {}|

That's bug 911213 (with patch, note).

> and using |Promise(...)| rather than |new Promise(...)| to create a promise.

With the DOM promise implementation, both work to create a Promise, like for every other DOM constructor in Gecko.  See https://www.w3.org/Bugs/Public/show_bug.cgi?id=22808
(Reporter)

Comment 29

5 years ago
Promise() actually ought to throw per the specification.

If it's too hard to get the event loop going we should update Andrea's implementation. It's not just API changes however. Resolving happens on the output side now, not the input side. A bunch of small details have been tweaked and thenables need to be supported.
Flags: needinfo?(annevk)
> Promise() actually ought to throw per the specification.

Which specification?  Per WebIDL, for a WebIDL interface with a constructor, it should not.  That's what the discussion in https://www.w3.org/Bugs/Public/show_bug.cgi?id=22808 is about.

In any case, the Promise vs new Promise thing is pretty minor....
(In reply to Nikhil Marathe [:nsm] from comment #26)
> I propose we modify Andrea's implementation to support these two changes
> while the number of users in the codebase is still small. We keep this
> implementation around until the JS engine microtasks are implemented, or the
> JS implementation interacts with the Gecko event loop. That way we can
> remove dom/promise in the future without breaking everything. Does this
> sound acceptable?

That sounds good to me. It also lends a bit less urgency to the Xray-to-JS stuff, though that's still probably work that needs to happen eventually.
(In reply to Anne (:annevk) from comment #29)
> Promise() actually ought to throw per the specification.
> 

Huh? the AP2 spec makes that the only way to get a Promise, the |new Promise()| style gives a 'initialized' promise which doesn't seem to be useful outside of the implementation itself.
Seems like DOM Promise semantics were removed from the live spec [1]
Anne, Is there a place I can see an older version? I want to know what you mean by resolved at input side vs. output side.

[1]: http://dom.spec.whatwg.org/
(Reporter)

Comment 34

5 years ago
bz, https://github.com/domenic/promises-unwrapping/blob/master/README.md is not defined in terms of IDL. And given how JavaScript wants to do subclassing, what IDL does now is harmful. :/

nsm, Promise() will throw a TypeError because this.[[IsPromise]] is not set. It's kinda cryptic how that works though, I recommend talking to someone who can read and explain ECMAScript language better than I can. As for the older DOM specification, https://github.com/slightlyoff/Promises/ has a polyfill of it. You can also go through GitHub and check out an older version.
> what IDL does now is harmful.

The problem is it also matches what Gecko has been shipping for over a decade...  So we have this problem of making sure sites don't depend on it.  We should probably take this to a different bug, though.
Created attachment 803129 [details] [diff] [review]
WIP AP2 style promises.

As discussed with Anne, in ES6, |Promise(...)| should throw and |new Promise(...)| is the right way of using Promises from user-space. The current behaviour of other similar types though (Map, Set) is to allow both variants. Updated patch to allow both styles right now.
Attachment #801347 - Attachment is obsolete: true
Attachment #801347 - Flags: feedback?(nmatsakis)
Attachment #801347 - Flags: feedback?(domenic)
Comment on attachment 803129 [details] [diff] [review]
WIP AP2 style promises.

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

Sorry this took me a while. I didn't review the *Promise* logic at all, just the safety and general style of the self-hosted code. Here are a few suggestions for improvement.

::: js/src/builtin/Promise.js
@@ +18,5 @@
> + # See builtin/Promise.cpp for slot descriptions.                            #
> + #                                                                           #
> + # Don't directly touch a Promise's slots from anywhere else!                #
> + ############################################################################*/
> +function Promise_GetResolvedValue(p) {

I suggest you use macros here (we run the self-hosted code through cpp, so you can just write #define)

Also, define the constants for the slot numbers in a header file and #include it both from here and in the c++ code.

See bug 898362 for an example where I use this approach.

@@ +103,5 @@
> +
> +  // Step 2
> +  if (IsPromise(x)) {
> +    // 2.1
> +    if (Object.is(p, x)) {

Accessing globals like `Object` is dangerous in self-hosted code. Add a binding in Utilities.js, if one doesn't exist already, that captures the initial value, then call that.

@@ +324,5 @@
> +  iterable.forEach(function(nextValue) {
> +    let currentIndex = index;
> +    let nextPromise = Promise_ToPromise(nextValue);
> +    let onFulfilled = function(v) {
> +      values[currentIndex] = v;

You have to be wary of array assignments; if the user has modified Array.prototype, it may not do as you expect, since you are starting with empty array.

You may want to use the intrinsic NewDenseArray to start out with an array that is pre-allocated to a particular size, or else to use some of the other intrinsics (`UnsafePutElement`, perhaps) to guarantee the semantics you want.

Comment 38

5 years ago
(In reply to Bobby Holley (:bholley) from comment #20)
> I just spoke to bill about for a while about this. We brainstormed how to do
> Xrays for JS objects, but he also pointed out that our JSM-implemented
> promises make use of the event loop (to prevent .then().then() from ever
> hitting recursion limits)
Why? To prevent misuses, the only known alternative to "recursion limit" or equivalent is to add a clamping delay. Not sure which one we want more. I would prefer recursion limit. I heard lots other devs too.
David, clamping has nothing to do with recursion limits.  The "recursion limit" is a hardware (or rather C/C++ language) limitation: when you run out of stack, you crash.  So you can't allow sync-invoking a deep callstack.  But with promises you can just break it up, which is what comment 20 says.
Can't we do something similar to tail-call optimization to prevent stack overflow, even when the Promise is integrated into the language?
Rather, I thought it was the benefit of integrating Promise into the language. I used to reluctant to convert my loops to Promise because I would have been imposed some delay for every iteration.

Comment 42

5 years ago
(In reply to Nikhil Marathe [:nsm] from comment #26)
> We keep this
> implementation around until the JS engine microtasks are implemented, or the
> JS implementation interacts with the Gecko event loop. That way we can
> remove dom/promise in the future without breaking everything. Does this
> sound acceptable?

Do you know if somebody is working on the js microtasks implementation?  If not, it seems to me like working on this patch for now is not a good use of time since things may change significantly by the time we can consider implementing this inside SpiderMonkey.
I'm not actively working on it.
Assignee: nsm.nikhil → nobody
(Reporter)

Updated

5 years ago
Assignee: nobody → general
Component: DOM → JavaScript Engine
(Reporter)

Comment 44

5 years ago
I filed bug 918806 to make our DOM implementation match AP2-style as close as possible and enable that by default. Once the SpiderMonkey team has figured out its event loop story we can migrate it.
(Reporter)

Updated

4 years ago
Summary: Implement AP2-style promises → Implement promises in the JavaScript engine
Assignee: general → nobody

Comment 45

3 years ago
Some implementation discussion in the logs here:

  http://logs.glob.uno/?c=content#c289609
Created attachment 8689625 [details] [diff] [review]
Implement ES6 Promises in the JavaScript engine

This implements all of the ES6 spec, but has undergone only very cursory testing.

It works in the shell, and allows creating Promise instances in JS and through a JSAPI call.

Next up: integration with Gecko, testing.

Relies on bug 1226261, because the constructor is self-hosted, too.
Assignee: nobody → till
Status: NEW → ASSIGNED
Created attachment 8693575 [details] [diff] [review]
Implement ES6 Promises in the JavaScript engine, rebased
Created attachment 8702113 [details] [diff] [review]
Part 1: Implement ES6 Promises in the JavaScript engine. v3

Now without the self-hosted ctor, so doesn't depend on bug 1226261.
Attachment #8693575 - Attachment is obsolete: true
Attachment #8689625 - Attachment is obsolete: true
Attachment #803129 - Attachment is obsolete: true
Created attachment 8702114 [details] [diff] [review]
Part 2: Support debugger hooks for creation and settling of promises

This adds support for calling the debugger hooks for creation and settling of promises. The hooks stay unchanged, as they work just fine as before.
Attachment #8702114 - Flags: review?(shu)
Created attachment 8702115 [details] [diff] [review]
Part 1: Implement ES6 Promises in the JavaScript engine. v4

Now with less assert triggering
Attachment #8702113 - Attachment is obsolete: true
Created attachment 8702116 [details] [diff] [review]
Part 3: Implement all Promise inspection functionality as Debugger getters

This implements all the Promise inspection stuff currently available via PromiseDebugging as getters on Debugger.Object. Shu, is this about right, are the names way off, or am I going into the wrong direction entirely?

Also, this doesn't yet do anything about the remote protocol, I haven't started looking into that, since it seems to make more sense to do that once I have promises integrated into Gecko.
Attachment #8702116 - Flags: review?(shu)
(In reply to Till Schneidereit [:till] from comment #51)
> Created attachment 8702116 [details] [diff] [review]
> Part 3: Implement all Promise inspection functionality as Debugger getters
> 
> This implements all the Promise inspection stuff currently available via
> PromiseDebugging as getters on Debugger.Object. Shu, is this about right,
> are the names way off, or am I going into the wrong direction entirely?

This sounds correct to me.

> Also, this doesn't yet do anything about the remote protocol, I haven't
> started looking into that, since it seems to make more sense to do that once
> I have promises integrated into Gecko.

The protocol request/response methods are all on ObjectActor, so we were papering over the fact that PromiseDebugging was some random webidl thing and not part of the Debugger API anyways. We already wrap PromiseDebugging with functions that take Debugger.Object objects for the most part as well: https://dxr.mozilla.org/mozilla-central/source/devtools/server/actors/object.js#1655-1680
Comment on attachment 8702116 [details] [diff] [review]
Part 3: Implement all Promise inspection functionality as Debugger getters

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

Overall this LGTM, but you should also make sure to add documentation for these new methods to js/src/doc/Debugger/Debugger.Object.md in the final patch.

::: js/src/vm/Debugger.cpp
@@ +7277,5 @@
> +        return true;
> +    }
> +
> +    Rooted<PromiseObject*> promise(cx, &refobj->as<PromiseObject>());
> +    args.rval().setInt32(promise->state());

int 32?
Attachment #8702116 - Flags: feedback+

Comment 54

2 years ago
Comment on attachment 8702114 [details] [diff] [review]
Part 2: Support debugger hooks for creation and settling of promises

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

Looks like swapping out old fake promises with real promises. I don't know much about the actual Promise implementation but this LGTM.

::: js/src/builtin/Promise.cpp
@@ +6,5 @@
>  
>  #include "builtin/Promise.h"
>  #include "builtin/SelfHostingDefines.h"
>  #include "gc/Heap.h"
> +#include "vm/Debugger.h"

JS::dbg::onNewPromise is in "js/Debug.h"

::: js/src/builtin/TestingFunctions.cpp
@@ +1202,2 @@
>          return false;
> +    if (!args[0].isObject() || args[0].toObject().getClass() != &PromiseObject::class_) {

Use JSObject::is<T>
Attachment #8702114 - Flags: review?(shu) → review+

Comment 55

2 years ago
Comment on attachment 8702116 [details] [diff] [review]
Part 3: Implement all Promise inspection functionality as Debugger getters

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

The API needs more careful design and documentation in doc/Debugger/Debugger.Object.md

I also do not understand why dependentPromises only looks at the reject reactions.

::: js/src/builtin/Promise.cpp
@@ +76,5 @@
> +PromiseObject::getID()
> +{
> +    Value idVal(getReservedSlot(PROMISE_ID_SLOT));
> +    if (idVal.isUndefined()) {
> +        idVal.setDouble(++gIDGenerator);

Are we worried about the heat death of the universe?

@@ +85,5 @@
> +
> +ArrayObject*
> +PromiseObject::dependentPromises(JSContext* cx)
> +{
> +    RootedValue rejectReactionsVal(cx, getReservedSlot(PROMISE_REJECT_REACTIONS_SLOT));

Why are only reject reactions iterated over here? Are fulfill reactions not considered dependent?

@@ +95,5 @@
> +    if (!GetPropertyKeys(cx, rejectReactions, JSITER_OWNONLY, &keys))
> +        return nullptr;
> +
> +    size_t entries = keys.length();
> +    if (entries == 0)

Inline keys.length() for entries here.

@@ +111,5 @@
> +    for (size_t i = 0; i < keys.length(); i++) {
> +        MutableHandleValue val = values[i];
> +        if (!GetProperty(cx, rejectReactions, rejectReactions, keys[i], val))
> +            return nullptr;
> +        RootedObject reaction(cx, &val.toObject());

How are these guaranteed to be objects?

@@ +114,5 @@
> +            return nullptr;
> +        RootedObject reaction(cx, &val.toObject());
> +        if (!GetProperty(cx, reaction, reaction, capabilitiesId, val))
> +            return nullptr;
> +        RootedObject capabilities(cx, &val.toObject());

Ditt: how are these guaranteed to be objects?

@@ +119,5 @@
> +        if (!GetProperty(cx, capabilities, capabilities, cx->runtime()->commonNames->promise, val))
> +            return nullptr;
> +    }
> +    return NewDenseCopiedArray(cx, values.length(), values[0].address());
> +

Nit: extra newline

::: js/src/vm/Debugger.cpp
@@ +7272,5 @@
> +{
> +    THIS_DEBUGOBJECT_REFERENT(cx, argc, vp, "get promiseState", args, refobj);
> +
> +    if (!refobj->is<PromiseObject>()) {
> +        args.rval().setUndefined();

Debugger.Object has precedence for methods and accessors throwing when accessed on the wrong kind of thing, e.g., executeInGlobal. I would throw here and below.

@@ +7277,5 @@
> +        return true;
> +    }
> +
> +    Rooted<PromiseObject*> promise(cx, &refobj->as<PromiseObject>());
> +    args.rval().setInt32(promise->state());

Yeah, this should be an enum class internally and, if you want to expose states as a raw number, at least provide constants in the Debugger API. Usually, though, enums are just reflected as string values in Debugger.

@@ +7334,5 @@
> +
> +static bool
> +DebuggerObject_getPromiseAllocationStack(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    THIS_DEBUGOBJECT_REFERENT(cx, argc, vp, "get promiseTimeToResolution", args, refobj);

Copy/paste typo

@@ +7344,5 @@
> +
> +    Rooted<PromiseObject*> promise(cx, &refobj->as<PromiseObject>());
> +    RootedObject stack(cx, promise->allocationStack());
> +    if (!cx->compartment()->wrap(cx, &stack))
> +        return false;

Hm... this is a plain CCW. What is the intended API here? Do you want it plainly accessible as a D.O? Do you want it as a debuggee (in that compartment) value? For the former, you should use wrapDebuggeeObject.

@@ +7352,5 @@
> +
> +static bool
> +DebuggerObject_getPromiseResolutionStack(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    THIS_DEBUGOBJECT_REFERENT(cx, argc, vp, "get promiseTimeToResolution", args, refobj);

Copy/paste typo

@@ +7367,5 @@
> +    }
> +
> +    RootedObject stack(cx, promise->resolutionStack());
> +    if (!cx->compartment()->wrap(cx, &stack))
> +        return false;

Ditto here on plain CCW.

@@ +7375,5 @@
> +
> +static bool
> +DebuggerObject_getPromiseID(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    THIS_DEBUGOBJECT_REFERENT(cx, argc, vp, "get promiseTimeToResolution", args, refobj);

Copy/paste typo

@@ +7406,5 @@
> +        if (!dependentPromises)
> +            return false;
> +    }
> +    if (!cx->compartment()->wrap(cx, &dependentPromises))
> +        return false;

Unlike the stack cases above, I think it's clear that dependentPromises should return an array in the debugger compartment of Debugger.Objects corresponding to the dependent promises.
Attachment #8702116 - Flags: review?(shu)
What's the plan for the pending rejection flushing bits of PromiseDebugging?

Does the Debugger setup here correctly handle the case when Promise.prototype.then() is passed the resolve/reject functions that are arguments to a Promise constructor?  See the somewhat lengthy comment at the beginning of WrapperPromiseCallback::GetDependentPromise in PromiseCallback.cpp.

Updated

2 years ago
Blocks: 1128959

Updated

2 years ago
Depends on: 1237762
Created attachment 8709007 [details] [diff] [review]
Part 1: Implement ES6 Promises in the JavaScript engine

This should be pretty much done, but needs more tests. Asking for feedback as a sanity check on the implementation.
Attachment #8709007 - Flags: feedback?(efaustbmo)
Attachment #8702115 - Attachment is obsolete: true
Created attachment 8709010 [details] [diff] [review]
Part 2: Support debugger hooks for creation and settling of promises
Attachment #8702114 - Attachment is obsolete: true
Created attachment 8709013 [details] [diff] [review]
Part 3: Implement all Promise inspection functionality as Debugger getters. v2

I think this addresses all comments, except for bz's comment 56, which I'll address separately.

(In reply to Shu-yu Guo [:shu] (PTO until 1/18) from comment #55)
> Comment on attachment 8702116 [details] [diff] [review]
> Part 3: Implement all Promise inspection functionality as Debugger getters
> 
> Review of attachment 8702116 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The API needs more careful design and documentation in
> doc/Debugger/Debugger.Object.md

You're right, of course. I added documentation both to the md doc and inline where things are less than obvious.

> I also do not understand why dependentPromises only looks at the reject
> reactions.

Because those contain all the required information: in most cases dependent promises are stored in both the fulfill and reject reactions list. Sometimes, though, only a reject reaction exists, so that's the list we want to use.

> > +        idVal.setDouble(++gIDGenerator);
> 
> Are we worried about the heat death of the universe?

This was taken from the DOM implementation, but you're right and I changed it to uint32_t.

> > +    RootedValue rejectReactionsVal(cx, getReservedSlot(PROMISE_REJECT_REACTIONS_SLOT));
> 
> Why are only reject reactions iterated over here? Are fulfill reactions not
> considered dependent?

See above. I added a comment explaining this to the code, too.

> > +        RootedObject reaction(cx, &val.toObject());
> 
> How are these guaranteed to be objects?

By the code creating and using them: they're internal objects not accessible by content.

> Debugger.Object has precedence for methods and accessors throwing when
> accessed on the wrong kind of thing, e.g., executeInGlobal. I would throw
> here and below.

Done. I only saw the precedents for returning undefined, but yes, this is much better.

> > +    args.rval().setInt32(promise->state());
> 
> Yeah, this should be an enum class internally and, if you want to expose
> states as a raw number, at least provide constants in the Debugger API.
> Usually, though, enums are just reflected as string values in Debugger.

Right, this was just wrong. Fixed to use #DEFINEs internally (because self-hosted code needs to use it, too) and return useful values at the API level.

> > +    Rooted<PromiseObject*> promise(cx, &refobj->as<PromiseObject>());
> > +    RootedObject stack(cx, promise->allocationStack());
> > +    if (!cx->compartment()->wrap(cx, &stack))
> > +        return false;
> 
> Hm... this is a plain CCW. What is the intended API here? Do you want it
> plainly accessible as a D.O? Do you want it as a debuggee (in that
> compartment) value? For the former, you should use wrapDebuggeeObject.

Thanks for the catch, this again was just wrong.

> > +        if (!dependentPromises)
> > +            return false;
> > +    }
> > +    if (!cx->compartment()->wrap(cx, &dependentPromises))
> > +        return false;
> 
> Unlike the stack cases above, I think it's clear that dependentPromises
> should return an array in the debugger compartment of Debugger.Objects
> corresponding to the dependent promises.

Indeed! Fixed to create objects in the proper compartment both for the list and its entries.
Attachment #8702116 - Attachment is obsolete: true
Attachment #8709013 - Flags: review?(shu)
(In reply to Boris Zbarsky [:bz] from comment #56)
> What's the plan for the pending rejection flushing bits of PromiseDebugging?

I'll implement the HostPromiseRejectionTracker thing that's part of the ES7 draft[1] and change what we have right now to use that.

> Does the Debugger setup here correctly handle the case when
> Promise.prototype.then() is passed the resolve/reject functions that are
> arguments to a Promise constructor?  See the somewhat lengthy comment at the
> beginning of WrapperPromiseCallback::GetDependentPromise in
> PromiseCallback.cpp.

Thanks, I hadn't considered that. The new patch fixes this in a somewhat different way to the current implementation, and also avoids creating the temporary promise if we know that it can't have leaked to content.


[1] https://tc39.github.io/ecma262/#sec-host-promise-rejection-tracker
Created attachment 8709016 [details] [diff] [review]
Part 4: Support JS engine-implemented Promises in webidl

I'm afraid I'll have to give up on the webidl stuff; I've already spent way more time on it than I had anticipated, and didn't really get anywhere.

bz, I've attached what I have now in case it helps at all - it's still the invasive approach that changes the webidl parser instead of keeping that as-is and just making Promise [NoInterfaceObject].

Let me know if I can do anything more to help with this part. Otherwise, I'll happily take over again once the basic integration works and something compiles in a way that keeps the SpiderMonkey Promise active in global objects.
Flags: needinfo?(bzbarsky)
> Are we worried about the heat death of the universe?

Overflowing a 32-bit counter takes on the order of a second or two on modern hardware.  A few minutes if each counter increment is counting a "really slow" (hundreds of instructions, for the case of things we're caring about here) operation.  So if you want to guarantee anything resembling uniqueness in actual operation you need to use a 64-bit counter.
Flags: needinfo?(till)
Oh, and just to check: we were talking about a build-time flag for SpiderMonkey Promises.  That's not done so far, right?  I'll just assume a SPIDERMONKEY_PROMISE define that gets set appropriately for the IDL bits.

Comment 64

2 years ago
I'm adding unhandled DOM Promise rejection reporting to test suites in bug 989960.

If this implementation is compatible with the PromiseDebugging API, there is nothing to do there. If the observing mechanism is different then we may have to update the PromiseTestUtils module.

Methods used there include onLeftUncaught, getPromiseID, getState, getRejectionStack.

Comment 65

2 years ago
Async stacks for "then" callbacks is another feature we should keep in the new implementation.
(In reply to :Paolo Amadini from comment #64)
> I'm adding unhandled DOM Promise rejection reporting to test suites in bug
> 989960.
> 
> If this implementation is compatible with the PromiseDebugging API, there is
> nothing to do there. If the observing mechanism is different then we may
> have to update the PromiseTestUtils module.
> 
> Methods used there include onLeftUncaught, getPromiseID, getState,
> getRejectionStack.
> 
> Async stacks for "then" callbacks is another feature we should keep in the
> new implementation.

The implementation contains equivalents for all the PromiseDebugging stuff[1], including the creation and settling stacks. See part 3 for details if you're interested.


[1] Well, right now the tracking of uncaught rejections isn't in there, but I'll add that before landing.(In reply to Boris Zbarsky [:bz] from comment #62)
> > Are we worried about the heat death of the universe?
> 
> Overflowing a 32-bit counter takes on the order of a second or two on modern
> hardware.  A few minutes if each counter increment is counting a "really
> slow" (hundreds of instructions, for the case of things we're caring about
> here) operation.  So if you want to guarantee anything resembling uniqueness
> in actual operation you need to use a 64-bit counter.

I wonder if we really would get into a situation where we have that many promises, ever. However, it can certainly not be ruled out, and the costs of having this be a uint64_t instead are imperceptible, so I'll change it back.
Flags: needinfo?(till)
Created attachment 8709978 [details] [diff] [review]
Part 1: Implement ES6 Promises in the JavaScript engine

Let's actually make this a review. This version adds an --enable-promises configure flag that governs whether the Promise constructor is installed on globals. Everything else gets build always, so I don't have to keep rebasing this.
Attachment #8709978 - Flags: review?(efaustbmo)
Attachment #8709007 - Attachment is obsolete: true
Attachment #8709007 - Flags: feedback?(efaustbmo)
Is there a design doc or some sort of plan for how this will integrate with the event loop that I can read?
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #68)
> Is there a design doc or some sort of plan for how this will integrate with
> the event loop that I can read?

The design is exceedingly simple: the patch adds a JS_SetEnqueuePromiseJobCallback JSAPI function[1] that can be used to set a callback to use when a promise job, in the form of a JSFunction, should be enqueued. The embedding can then schedule that job however it wants. As a simple example, the shell just pushes the jobs on a queue which it drains after executing a script[2].


[1] https://bugzilla.mozilla.org/attachment.cgi?id=8709978&action=diff#a/js/src/jsapi.h_sec3
[2] https://bugzilla.mozilla.org/attachment.cgi?id=8709978&action=diff#a/js/src/shell/js.cpp_sec3
Comment hidden (obsolete)
Created attachment 8710094 [details] [diff] [review]
Part 1: Implement ES6 Promises in the JavaScript engine

Now with JS_GetPromiseConstructor and JS_GetPromisePrototype JSAPI functions. Sorry for the bugspam.
Attachment #8710094 - Flags: review?(efaustbmo)
Attachment #8709978 - Attachment is obsolete: true
Attachment #8709978 - Flags: review?(efaustbmo)

Comment 72

2 years ago
Comment on attachment 8709013 [details] [diff] [review]
Part 3: Implement all Promise inspection functionality as Debugger getters. v2

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

Someone else needs to review the Promise.js changes. I don't know enough about the implementation or spec to do it.

Looks pretty good. Tests? :)

::: js/src/builtin/Promise.cpp
@@ +73,5 @@
>  }
>  
> +namespace {
> +// Generator used by PromiseObject::getID.
> +mozilla::Atomic<uint32_t> gIDGenerator(0);

Seeing how we don't have uint64s in JS yet, and bz suggested uint64s, I guess make this a double?

::: js/src/doc/Debugger/Debugger.Object.md
@@ +214,5 @@
> +        * `"rejected"`, if the [`Promise`][promise] has been rejected.
> +
> +    `value`
> +    :   If the [`Promise`][promise] has been *fulfilled*, this is the value it
> +        it was fulfilled with.

Add ", `undefined` otherwise."

@@ +218,5 @@
> +        it was fulfilled with.
> +
> +    `reason`
> +    :   If the [`Promise`][promise] has been *rejected*, this is the value it
> +        it was rejected with.

Add ", `undefined` otherwise."

@@ +225,5 @@
> +    exception.
> +
> +`promiseAllocationStack`
> +:   If the referent is a [`Promise`][promise], this is the stack to the
> +    promise's allocation point. This can return null if the promise was not

What kind of object are the stacks? How do I use them?

@@ +273,5 @@
> +:   If the referent is a [`Promise`][promise], this is the number of
> +    milliseconds elapsed between when the [`Promise`][promise] was created and
> +    when it was resolved. If the referent hasn't been resolved or is not a
> +    [`Promise`][promise], throw a `TypeError` exception.
> +

Great docs.

::: js/src/vm/Debugger.cpp
@@ +7286,5 @@
> +DebuggerObject_getIsPromise(JSContext* cx, unsigned argc, Value* vp)
> +{
> +    THIS_DEBUGOBJECT_REFERENT(cx, argc, vp, "get isPromise", args, refobj);
> +
> +    refobj = CheckedUnwrap(refobj);

I may be missing something, but I don't think you should be unwrapping here and below. If the referent promise is already a CCW, it's not up to you to unwrap it here.

@@ +7308,5 @@
> +                             "Debugger", "Promise", refobj->getClass()->name);
> +        return false;
> +    }
> +
> +    Rooted<PromiseObject*> promise(cx, &refobj->as<PromiseObject>());

The PromiseObject checking here and below should be refactored out, perhaps via a new macro THIS_DEBUGOBJECT_OWNER_PROMISE_REFERENT.

@@ +7333,5 @@
> +
> +    if (!dbg->wrapDebuggeeValue(cx, &result))
> +        return false;
> +    if (!dbg->wrapDebuggeeValue(cx, &reason))
> +        return false;

No need to wrap these, since these aren't debuggee values, they're debugger values. Besides, wrapDebuggeeValue does nothing on non-objects.

@@ +7335,5 @@
> +        return false;
> +    if (!dbg->wrapDebuggeeValue(cx, &reason))
> +        return false;
> +
> +    if (!SetProperty(cx, obj, cx->names().state.get(), state))

DefineProperty

@@ +7337,5 @@
> +        return false;
> +
> +    if (!SetProperty(cx, obj, cx->names().state.get(), state))
> +        return false;
> +    if (!SetProperty(cx, obj, cx->names().value.get(), result))

DefineProperty

@@ +7339,5 @@
> +    if (!SetProperty(cx, obj, cx->names().state.get(), state))
> +        return false;
> +    if (!SetProperty(cx, obj, cx->names().value.get(), result))
> +        return false;
> +    if (!SetProperty(cx, obj, cx->names().reason.get(), reason))

DefineProperty
Attachment #8709013 - Flags: review?(shu) → review+
Comment on attachment 8709013 [details] [diff] [review]
Part 3: Implement all Promise inspection functionality as Debugger getters. v2

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

Drive by comments, hope you don't mind!

::: js/src/doc/Debugger/Debugger.Object.md
@@ +214,5 @@
> +        * `"rejected"`, if the [`Promise`][promise] has been rejected.
> +
> +    `value`
> +    :   If the [`Promise`][promise] has been *fulfilled*, this is the value it
> +        it was fulfilled with.

Is this a Debugger.Object referring to the fulfilled value or the fulfilled value itself? The docs should make this explicit.

I would expect the former; it certainly has less footguns.

@@ +225,5 @@
> +    exception.
> +
> +`promiseAllocationStack`
> +:   If the referent is a [`Promise`][promise], this is the stack to the
> +    promise's allocation point. This can return null if the promise was not

I assume these are SavedFrame stacks? There are docs on them you can link to in js/src/doc/SavedFrame/SavedFrame.md. Unlike the debuggee objects returned by the getPromiseState, these don't need to be wrapped because they (a) aren't really debuggee objects and (b) are always frozen.

@@ +243,5 @@
> +    [`Promise`][promise], throw a `TypeError` exception.
> +
> +`promiseDependentPromises`
> +:   If the referent is a [`Promise`][promise], this is an `Array` of the
> +    promises directly depending on the referent [`Promise`][promise]. These

array of the raw promises or array of Debugger.Objects referring to the promises? Again, the docs should make it clear, and again I think it should be Debugger.Objects.

::: js/src/vm/Debugger.cpp
@@ +7330,5 @@
> +        reason = promise->result();
> +        break;
> +    }
> +
> +    if (!dbg->wrapDebuggeeValue(cx, &result))

So yes, the results are wrapped in Debugger.Objects? Nice.

@@ +7407,5 @@
> +    }
> +
> +    Rooted<PromiseObject*> promise(cx, &refobj->as<PromiseObject>());
> +    RootedValue stack(cx, ObjectValue(*promise->allocationStack()));
> +    if (!dbg->wrapDebuggeeValue(cx, &stack))

Again, I don't think any of the stacks need be wrapped: they aren't debuggee values and are frozen anyways.

Comment 74

2 years ago
(In reply to Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8] from comment #73)
 
> @@ +7407,5 @@
> > +    }
> > +
> > +    Rooted<PromiseObject*> promise(cx, &refobj->as<PromiseObject>());
> > +    RootedValue stack(cx, ObjectValue(*promise->allocationStack()));
> > +    if (!dbg->wrapDebuggeeValue(cx, &stack))
> 
> Again, I don't think any of the stacks need be wrapped: they aren't debuggee
> values and are frozen anyways.

What compartment are they in, the stacks?
When I said "wrapped" in that comment, I meant "wrapped in a Debugger.Object". They may still need to be wrapped by a CCW.

I'm not sure what compartment the promise-related stacks are captured in, but likely the debuggee's compartment, as well. For allocation stacks, we capture them in the debuggee compartment, and return them as CCWs not Debugger.Objects: https://dxr.mozilla.org/mozilla-central/source/js/src/vm/Debugger.cpp#7326
Created attachment 8711055 [details] [diff] [review]
Part 1: Implement ES6 Promises in the JavaScript engine

This version fixes a few bugs uncovered by running test262, and adds various JSAPI functions needed for Gecko integration. Some of these functions are stubs, but the patch compiles, at least.
Attachment #8711055 - Flags: feedback?(bzbarsky)
Attachment #8710094 - Attachment is obsolete: true
Attachment #8710094 - Flags: review?(efaustbmo)
In the toplevel configure.in, the new bits need to come before the MOZ_CREATE_CONFIG_STATUS() line.
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bzbarsky)
Comment on attachment 8711055 [details] [diff] [review]
Part 1: Implement ES6 Promises in the JavaScript engine

So in addition to the configure issue, a few other problems:

1)  The docstring in the shell changes still talks about JS_NewPromiseObject.

2)  NewPromiseObject needs to take a HandleObject executor, not HandleFunction.  It's perfectly valid to pass in some non-JSFunction callable (most obviously, a callable proxy) as the executor.

3)  Same thing for the two callable arguments to CallOriginalPromiseThen.  And these can actually be null, right?  The impl needs to handle that.

4)  It might be nice to do the same for the promise job callback, since the consumer will need to use a JSObject* there anyway in the case of Gecko.

5)  It looks like most runtime callbacks take a void* that can be used to pass around some closure from the thing setting the callback (e.g. see JS::SetOutOfMemoryCallback or JS_SetObjectsTenuredCallback).  Might be nice for the promise job callback too.
Attachment #8711055 - Flags: feedback?(bzbarsky) → feedback+
So I have a browser up with SpiderMonkey promises with just a few rough edges left (e.g. PromiseNativeHandler stuff).

I tried this:

  Promise.prototype.then.call(null)

and the exception it throws (as it should) is a bit weird:

  TypeError: CallPromiseMethodIfWrapped method called on incompatible null

Also, in the console the stack shown in this case includes a line saying:

  then() self-hosted:5589

which is rather unexpected; I would have expected self-hosted frames to get filtered out from the stack...  Might need a separate bug on this, because this may be a general problem not really related to Promise.
Oh, and since I can do it now, I ran the js/builtins/Promise-subclassing.html test.  It's failing for race() and all() because it looks like those are still looking at Symbol.species, when they shouldn't.
Created attachment 8711343 [details] [diff] [review]
Part 1: Implement ES6 Promises in the JavaScript engine

This fixes a few additional bugs and contains implementations of all but two of the new JSAPI functions. JS::ResolvePromise and JS::RejectPromise are somewhat annoying to implement, so they take a bit longer.

> In the toplevel configure.in, the new bits need to come before the
> MOZ_CREATE_CONFIG_STATUS() line.

Thanks! I also simplified the flag processing a bit, reducing the amount of cargo-culted cruft.

> 1)  The docstring in the shell changes still talks about JS_NewPromiseObject.

Fixed.

> 2)  NewPromiseObject needs to take a HandleObject executor, not
> HandleFunction.  It's perfectly valid to pass in some non-JSFunction
> callable (most obviously, a callable proxy) as the executor.

Fixed.

> 3)  Same thing for the two callable arguments to CallOriginalPromiseThen. 
> And these can actually be null, right?  The impl needs to handle that.

Fixed, and yes, they can be null.

> 4)  It might be nice to do the same for the promise job callback, since the
> consumer will need to use a JSObject* there anyway in the case of Gecko.

Do you mean changing changing JSEnqueuePromiseJobCallback to take a HandleObject? If so, does that matter much? HandleFunction is guaranteed to be correct, and should be usable wherever HandleObject is expected, no?

> 5)  It looks like most runtime callbacks take a void* that can be used to
> pass around some closure from the thing setting the callback (e.g. see
> JS::SetOutOfMemoryCallback or JS_SetObjectsTenuredCallback).  Might be nice
> for the promise job callback too.

I added `void* data` and removed the getter for the current callback, as that doesn't really seem useful, and not all callback setters have an equivalent.

> Oh, and since I can do it now, I ran the
> js/builtins/Promise-subclassing.html test.  It's failing for race() and
> all() because it looks like those are still looking at Symbol.species, when
> they shouldn't.

Thanks, fixed by updating to the current ES7 draft. Fixed a couple of test262 failures, too.
Attachment #8711055 - Attachment is obsolete: true
Comment on attachment 8711343 [details] [diff] [review]
Part 1: Implement ES6 Promises in the JavaScript engine

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

::: js/src/builtin/Promise.cpp
@@ +45,5 @@
> +        return nullptr;
> +
> +    // Steps 5-10.
> +    RootedValue ctorFun(cx);
> +    if (!GlobalObject::getIntrinsicValue(cx, cx->global(), cx->runtime()->commonNames->Promise,

cx->names().Promise

@@ +103,5 @@
> +
> +static JSObject*
> +CreatePromisePrototype(JSContext* cx, JSProtoKey key)
> +{
> +    return cx->global()->createBlankPrototype(cx, &PromiseObject::protoClass_);

I think just using NewBuiltinClassInstance<PlainObject>(cx) would be appropriate.

::: js/src/builtin/Promise.js
@@ +302,5 @@
> +// ES6, 25.4.4.1.1.
> +function PerformPromiseAll(iteratorRecord, constructor, resultCapability) {
> +    // Step 1.
> +    assert(IsConstructor(constructor), "PerformPromiseAll called with non-constructor");
> +    

trailing space

@@ +649,5 @@
> + */
> +function EnqueuePromiseReactions(promise, dependentPromise, onFulfilled, onRejected) {
> +    assert(IsPromise(promise),
> +           "EnqueuePromiseReactions must be provided with an unwrapped promise");
> +    

space
> Do you mean changing changing JSEnqueuePromiseJobCallback to take a HandleObject?

Yes.

> HandleFunction is guaranteed to be correct, and should be usable wherever
> HandleObject is expected

Not outside SpiderMonkey.  JSFunction is an opaque type outside SpiderMonkey; the only way to go from a JSFunction* to a JSObject* is by explicitly calling JS_GetFunctionObject().  Which is just extra noise, not to mention an extra out-of-line function call or two...
Created attachment 8711458 [details] [diff] [review]
Part 1: Implement ES6 Promises in the JavaScript engine

This also contains implementations of JS::ResolvePromise and JS::RejectPromise. In addition, I added JS::IsPromiseObject because that seems useful and was required for somewhat reasonable error checking in the shell test functions.

I also fixed the nits evilpie mentioned and changed the promise job callback signature to take a HandleObject instead of HandleFunction, as requested.
Attachment #8711343 - Attachment is obsolete: true
Created attachment 8711484 [details] [diff] [review]
Part 3: Implement all Promise inspection functionality as Debugger getters

Requesting a quick re-review as I did change quite a few thinks. Sorry for the churn :(

> Someone else needs to review the Promise.js changes. I don't know enough
> about the implementation or spec to do it.

Sure, I should've made that clear in my request.
Thanks for the review.

> Looks pretty good. Tests? :)

I'll port tests over once Gecko integration has progressed far enough that I can test how this works with PromiseDebugging. I want to remove that interface completely and adapt the tests, but I want to do it when I can actually try things out in the browser, too.
> 
> ::: js/src/builtin/Promise.cpp
> @@ +73,5 @@
> >  }
> >  
> > +namespace {
> > +// Generator used by PromiseObject::getID.
> > +mozilla::Atomic<uint32_t> gIDGenerator(0);
> 
> Seeing how we don't have uint64s in JS yet, and bz suggested uint64s, I
> guess make this a double?

mozilla::Atomic doesn't like double, so I changed it back to uint64_t. Since we're not in fact worried about the heat death of the universe, that should be fine.

> > +`promiseAllocationStack`
> > +:   If the referent is a [`Promise`][promise], this is the stack to the
> > +    promise's allocation point. This can return null if the promise was not
> 
> What kind of object are the stacks? How do I use them?

I linked to the SavedFrame docs as fitzgen suggested.

> @@ +273,5 @@
> > +:   If the referent is a [`Promise`][promise], this is the number of
> > +    milliseconds elapsed between when the [`Promise`][promise] was created and
> > +    when it was resolved. If the referent hasn't been resolved or is not a
> > +    [`Promise`][promise], throw a `TypeError` exception.
> > +
> 
> Great docs.

\o/

> ::: js/src/vm/Debugger.cpp
> @@ +7286,5 @@
> > +DebuggerObject_getIsPromise(JSContext* cx, unsigned argc, Value* vp)
> > +{
> > +    THIS_DEBUGOBJECT_REFERENT(cx, argc, vp, "get isPromise", args, refobj);
> > +
> > +    refobj = CheckedUnwrap(refobj);
> 
> I may be missing something, but I don't think you should be unwrapping here
> and below. If the referent promise is already a CCW, it's not up to you to
> unwrap it here.

Hmm, probably. Removed from all the promise-related functions.

> The PromiseObject checking here and below should be refactored out, perhaps
> via a new macro THIS_DEBUGOBJECT_OWNER_PROMISE_REFERENT.

Done, and I also refactored the existing macros a bit to reduce duplication. Hope that's ok.

> No need to wrap these, since these aren't debuggee values, they're debugger
> values. Besides, wrapDebuggeeValue does nothing on non-objects.

They're debuggee values, and can be objects. These are just the values that were passed by content when calling the `resolve` or `reject` hooks like e.g. in `new Promise(function(resolve, reject) { resolve({}); })`. I'm clearly missing documentation here, but I don't know where that should be added.

I also addressed fitzgens feedback, many thanks for giving it!
Attachment #8711484 - Flags: review?(shu)
Attachment #8711484 - Flags: review?(nfitzgerald)
Comment on attachment 8711484 [details] [diff] [review]
Part 3: Implement all Promise inspection functionality as Debugger getters

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

Looks great, thank you! Docs are great!

::: js/src/doc/Debugger/Debugger.Object.md
@@ +241,5 @@
> +    script. If the referent is not a [`Promise`][promise], throw a `TypeError`
> +    exception.
> +
> +`promiseID`
> +:   If the referent is a [`Promise`][promise], this is a runtime-unique

A lot of firefox/devtools frontend devs may not understand that runtime-unique is about equal to "worker-unique". Maybe worth noting that it is possible for ids to be reused across workers?
Attachment #8711484 - Flags: review?(nfitzgerald) → review+

Comment 87

2 years ago
Comment on attachment 8711484 [details] [diff] [review]
Part 3: Implement all Promise inspection functionality as Debugger getters

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

Looks good to me.

::: js/src/vm/Debugger.cpp
@@ +7315,5 @@
> +        result = promise->result();
> +        break;
> +      case JS::PromiseState::Rejected:
> +        state.setString(cx->names().rejected);
> +        reason = promise->result();

Typo? |promise->reason()| I imagine.
Attachment #8711484 - Flags: review?(shu) → review+
Created attachment 8712138 [details] [diff] [review]
Part 1: Implement ES6 Promises in the JavaScript engine

Fixes various issues uncovered by bz's try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c0605407883.
Attachment #8711458 - Attachment is obsolete: true
Comment on attachment 8712138 [details] [diff] [review]
Part 1: Implement ES6 Promises in the JavaScript engine

In CallOriginalPromiseThen, you have to allow onResolve/onReject to be null.  Right now the code assumes they're not (uses setObject(*onResolve) instead of setObjectOrNull(onResolve))...
Flags: needinfo?(bzbarsky)
Created attachment 8712903 [details] [diff] [review]
Part 1: Implement ES6 Promises in the JavaScript engine

Fixes a bunch of issues bz and/or tests uncovered, and adds Xray support.
Attachment #8712138 - Attachment is obsolete: true
(In reply to Shu-yu Guo [:shu] from comment #87)
> Typo? |promise->reason()| I imagine.

Actually no: since a Promise can only ever have one of them, I overloaded the same slot for `reason` and `result`. I can see how that's confusing, though, so I added `reason()` as a method on Promise - no need to expose this implementation detail.
Attachment #8709013 - Attachment is obsolete: true
Created attachment 8713245 [details] [diff] [review]
Part 1: Implement ES6 Promises in the JavaScript engine

This actually contains the Xray changes - I accidentally added those to a later patch before.
Attachment #8712903 - Attachment is obsolete: true
Created attachment 8715835 [details] [diff] [review]
Part 1: Add tests directly testing content Promise constructor resolved with chrome Promise
Attachment #8715835 - Flags: review?(bzbarsky)
Created attachment 8715845 [details] [diff] [review]
Part 2: Add support for running JS builtins' constructors over Xray wrappers without unwrapping the newTarget

DOM Xrays already have this ability, and with Promise moving to SpiderMonkey, we need it there, too. Part 6 will contain the code that uses this.
Attachment #8715845 - Flags: review?(bobbyholley)
Attachment #8715845 - Flags: feedback?(bzbarsky)
Created attachment 8715846 [details] [diff] [review]
Part 3: Allow wrapped self-hosted functions and intrinsics in the callFunction debug check

Useful so I can use callFunction instead of callContentFunction for wrapped self-hosted functions, and don't have to give up even more safety. Used in part 6.
Attachment #8715846 - Flags: review?(efaustbmo)
Created attachment 8715848 [details] [diff] [review]
Part 4: Add self-hosting intrinsic for calling wrapped functions without wrapper security checks

This intrinsic enables calling a wrapped function from a higher-privileged realm from code from a lower-privileged one with objects as arguments, even if the function is wrapped in an Xray wrapper.

Note that this is restricted to self-hosted functions or self-hosting intrinsics, so it doesn't enable calling arbitrary code without the security checks.

Used in part 6.
Attachment #8715848 - Flags: review?(efaustbmo)
Attachment #8715848 - Flags: feedback?(bzbarsky)
Attachment #8715848 - Flags: feedback?(bobbyholley)
Created attachment 8715850 [details] [diff] [review]
Part 5: Add self-hosting intrinsic for creating arrays in other compartments

Enters the compartment that the passed-in wrapped object originates from, creates an array, wraps it for the calling compartment, returns it. Simple.
Attachment #8715850 - Flags: review?(efaustbmo)
Created attachment 8715853 [details] [diff] [review]
Part 6: Implement ES6 Promises in the JavaScript engine

This finally passes all Xray tests. I had to add quite a bit of machinery to make that work, both in this patch and in the 5 newly-introduced patches it depends on.
Attachment #8715853 - Flags: feedback?(bzbarsky)
Attachment #8713245 - Attachment is obsolete: true
Comment on attachment 8715835 [details] [diff] [review]
Part 1: Add tests directly testing content Promise constructor resolved with chrome Promise

>+  testResolve1,

Why the reordering of the array here?  I was kinda keeping it in the actual order the tests are in...  Absent a strong reason, please keep it that way (and just add your new tests right before testCatch1 in the list, I guess).

r=me.  Thank you for adding this!
Attachment #8715835 - Flags: review?(bzbarsky) → review+
Attachment #8715845 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 8715848 [details] [diff] [review]
Part 4: Add self-hosting intrinsic for calling wrapped functions without wrapper security checks

Would be good to document what the arguments to this intrinsic are.  Based on reading the code sounds like the function to call, the this value to use, and the args, but it would be nice to have it clearly commented.

>+    MOZ_ASSERT(fun->as<JSFunction>().isSelfHostedBuiltin());

Will that assert is<JSFunction>?  If not, please do so.
Attachment #8715848 - Flags: feedback?(bzbarsky) → feedback+
(In reply to Boris Zbarsky [:bz] from comment #100)
> Will that assert is<JSFunction>?  If not, please do so.

It does assert that, yes.
Comment on attachment 8715853 [details] [diff] [review]
Part 6: Implement ES6 Promises in the JavaScript engine

Some definite problems I've run into so far:

1)  In PromiseObject::create we need to null-check proto before doing IsWrapper(proto).  Otherwise we crash anytime null comes through.

2)  GetWaitForAllPromise needs to deal with the situation when the promises in the list are not all from the same global (so some will be cross-compartment wrappers).  This is exercised by layout/style/test/test_font_loading_api.html, effectively, because that's taking font faces from one window and putting them into the font face set of another window, then doing an operation that corresponds to all() on the promises representing all the font faces in the font face set.  In practice I think it always has a list that's all from the same window, but that window may not match the window we want the return value of all() created in.

3)  We have code that depends on cross-global "instanceof Promise" working.  Specifically, at least dom/browser-element/BrowserElementChildPreload.js.  I'm really not sure how we want to handle this; it used to work because we implemented Promise via Web IDL, but I guess per spec it shouldn't work...  Maybe we just need to carefully audit all 35 instances of "instanceof Promise" in the tree.  I did check that changing BrowserElementChildPreload.js to check for "instanceof sandbox.Promise" makes the relevant tests pass.

4)  There are various failures in dom/bindings/test/test_promise_rejections_from_jsimplemented.html.  Some of these are due to better async stacks.  But some are due to breakage.  For example, test 5 is failing because it ends up propagating a chrome-side exception to content as a promise rejection reason.  The existing Promise code is very careful to not allow that, by, for example, passing the compartment of the promise to the Then() call and hence triggering the logic in CallbackObject::CallSetup::ShouldRethrowException and its caller in ~CallSetup which causes the chrome-side exception to be reported (to console, etc) and a sanitized exception to be thrown to content instead.  Test 3 in this file is the same issue.  Similar for calling the PromiseInit function and exceptions from it; that's caught by test 5 in this file.
Oh, and this is still failing check_spidermonkey_style.py.  ;)

Updated

2 years ago
Blocks: 1245936
Comment on attachment 8715845 [details] [diff] [review]
Part 2: Add support for running JS builtins' constructors over Xray wrappers without unwrapping the newTarget

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

::: js/public/Class.h
@@ +665,5 @@
>                                                  // SetNewObjectMetadata itself
>  #define JSCLASS_PRIVATE_IS_NSISUPPORTS  (1<<3)  // private is (nsISupports*)
>  #define JSCLASS_IS_DOMJSCLASS           (1<<4)  // objects are DOM
> +#define JSCLASS_HAS_XRAYED_CONSTRUCTOR  (1<<5)  // constructor is called
> +                                                // without unwrapping.

This needs a lot more documentation. Crawling through the patches I found a nice long comment in PromiseConstructor. I'd reference that here, and explain more specifically that this means that XrayWrappers, instead of getting the "real" constructor in the other compartment and invoking it with the unwrapped value, will instead get the constructor from the caller's compartment, and invoke it with the wrapped constructor as an argument, which is only really useful if the constructor implementation is aware of this handshake and uses it to set things up properly across the compartment boundary.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +939,5 @@
> +    JS::RootedObject holder(cx, self.ensureHolder(cx, wrapper));
> +    if (self.getProtoKey(holder) == JSProto_Function) {
> +        JSProtoKey standardConstructor = constructorFor(holder);
> +        const js::Class* clasp = js::ProtoKeyToClass(standardConstructor);
> +        MOZ_ASSERT(clasp);

constructorFor will return JSProto_Null in the case of a regular function that is not a standard constructor. This is not a valid input to js::ProtoKeyToClass, and certainly won't provide you with a clasp you can MOZ_ASSERT here.

Please add a testcase that triggers this assertion, add an additional assertion to ProtoKeyToClass against JSProto_Null, and fix this code such that all the business with the clasp is conditional on constructorFor returning something meaningful.
Attachment #8715845 - Flags: review?(bobbyholley) → review-
> Please add a testcase that triggers this assertion

Fwiw, try says that testing/mochitest/tests/Harness_sanity/test_SpecialPowersExtension.html triggers it.  But yes, having an explicit test would be nice.
Comment on attachment 8715848 [details] [diff] [review]
Part 4: Add self-hosting intrinsic for calling wrapped functions without wrapper security checks

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

In general, it seems like we should fine a way to avoid reimplementing CrossCompartmentWrapper::call. Can you just call that directly?

I'm also not wild about the JS engine deciding to ignore the security policy here, rather than leaving that up to the policy itself. But I guess it's kind of hard to communicate the scenario to AccessCheck::checkPassToPrivilegedCode...
Created attachment 8716771 [details] [diff] [review]
Part 4: Add self-hosting intrinsic for calling wrapped functions without wrapper security checks

(In reply to Bobby Holley (busy) from comment #106)
> In general, it seems like we should fine a way to avoid reimplementing
> CrossCompartmentWrapper::call. Can you just call that directly?

I didn't realize that I could enter a different security policy. With that, I can use CrossCompartmentWrapper::call as suggested, yes.

> I'm also not wild about the JS engine deciding to ignore the security policy
> here, rather than leaving that up to the policy itself. But I guess it's
> kind of hard to communicate the scenario to
> AccessCheck::checkPassToPrivilegedCode...

Right, I couldn't think of anything else, and think this is about as minimal as possible - especially as it only allows other self-hosted functions, not arbitrary code.
Attachment #8716771 - Flags: review?(efaustbmo)
Attachment #8716771 - Flags: review?(bobbyholley)
Attachment #8715848 - Attachment is obsolete: true
Attachment #8715848 - Flags: review?(efaustbmo)
Attachment #8715848 - Flags: feedback?(bobbyholley)
https://hg.mozilla.org/integration/mozilla-inbound/rev/857f110128f756387c3a07c933e81127217d5bdb
Bug 911216 - Part 1: Add tests directly testing content Promise constructor resolved with chrome Promise. r=bz
Whiteboard: [leave open]
Comment on attachment 8716771 [details] [diff] [review]
Part 4: Add self-hosting intrinsic for calling wrapped functions without wrapper security checks

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

::: js/src/vm/SelfHosting.cpp
@@ +556,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    MOZ_ASSERT(args.length() >= 2);
> +    MOZ_ASSERT(IsCallable(args[0]));
> +    MOZ_ASSERT(IsWrapper(&args[0].toObject()));
> +    MOZ_ASSERT(args[1].isObject() || args[1].isUndefined());

What is args[1]? It isn't documented, and doesn't seem to be used anywhere...

@@ +562,5 @@
> +    RootedObject wrappedFun(cx, &args[0].toObject());
> +#ifdef DEBUG
> +    RootedObject fun(cx, UncheckedUnwrap(wrappedFun));
> +#endif
> +    MOZ_ASSERT(fun->as<JSFunction>().isSelfHostedBuiltin());

Unless performance is a serious concern, I think we should MOZ_RELEASE_ASSERT this, along with any assumptions about the argument needed to get here.

@@ +575,5 @@
> +        args2[i].set(args[i + 2]);
> +
> +    const BaseProxyHandler* handler = wrappedFun->as<ProxyObject>().handler();
> +    AutoEnterPolicy policy(cx, handler, wrappedFun, JSID_VOIDHANDLE,
> +                           BaseProxyHandler::CALL, true);

You basically want to bypass the security policy here, right? This will actually call through to it, which probably isn't what you want.

I think you want AutoWaivePolicy here.
Attachment #8716771 - Flags: review?(bobbyholley) → review-
Depends on: 1246697
Created attachment 8717144 [details] [diff] [review]
Part 4: Add self-hosting intrinsic for calling wrapped functions without wrapper security checks

(In reply to Bobby Holley (busy) from comment #109)

> What is args[1]? It isn't documented, and doesn't seem to be used anywhere...

It's used as the this value a few lines below this. I added a couple of sentences on how the function is used to the doc comment.
> 
> @@ +562,5 @@
> > +    RootedObject wrappedFun(cx, &args[0].toObject());
> > +#ifdef DEBUG
> > +    RootedObject fun(cx, UncheckedUnwrap(wrappedFun));
> > +#endif
> > +    MOZ_ASSERT(fun->as<JSFunction>().isSelfHostedBuiltin());
> 
> Unless performance is a serious concern, I think we should
> MOZ_RELEASE_ASSERT this, along with any assumptions about the argument
> needed to get here.

Ok.

> You basically want to bypass the security policy here, right? This will
> actually call through to it, which probably isn't what you want.
> 
> I think you want AutoWaivePolicy here.

Ah, I wasn't aware of that. It worked with the AutoEnterPolicy, but clearly this makes more sense.
Attachment #8717144 - Flags: review?(bobbyholley)
Attachment #8716771 - Attachment is obsolete: true
Attachment #8716771 - Flags: review?(efaustbmo)
Comment on attachment 8717144 [details] [diff] [review]
Part 4: Add self-hosting intrinsic for calling wrapped functions without wrapper security checks

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

::: js/src/vm/SelfHosting.cpp
@@ +592,5 @@
> +    MOZ_ASSERT(args[1].isObject() || args[1].isUndefined());
> +
> +    RootedObject wrappedFun(cx, &args[0].toObject());
> +    RootedObject fun(cx, UncheckedUnwrap(wrappedFun));
> +    MOZ_RELEASE_ASSERT(fun->as<JSFunction>().isSelfHostedBuiltin());

This is UB if |fun| is not actually a JSFunction here, or arg[0] is not actually an object, right? I would prefer to release-assert all of that (given that this probably isn't all that hot).

I think we should _also_ release-assert that the topmost frame is self-hosted code. That will guarantee that this can only be used self-hosted to self-hosted, which will help a lot.

It might be worth adding a helper to do the "caller is self-hosted" assertion, and then we can call it from various other non-perf-sensitive-but-potentially-dangerous intrinsics to reduce our attack surface. Unless we have some sort of strong way to guarantee that property already?
Attachment #8717144 - Flags: review?(bobbyholley) → review-
Created attachment 8717665 [details] [diff] [review]
Part 2: Add self-hosting intrinsic for calling wrapped functions without wrapper security checks

(In reply to Bobby Holley (busy) from comment #112)
> > +    RootedObject wrappedFun(cx, &args[0].toObject());
> > +    RootedObject fun(cx, UncheckedUnwrap(wrappedFun));
> > +    MOZ_RELEASE_ASSERT(fun->as<JSFunction>().isSelfHostedBuiltin());
> 
> This is UB if |fun| is not actually a JSFunction here, or arg[0] is not
> actually an object, right? I would prefer to release-assert all of that
> (given that this probably isn't all that hot).

I added these checks.

> I think we should _also_ release-assert that the topmost frame is
> self-hosted code. That will guarantee that this can only be used self-hosted
> to self-hosted, which will help a lot.

These, however, I didn't add, see below.

> It might be worth adding a helper to do the "caller is self-hosted"
> assertion, and then we can call it from various other
> non-perf-sensitive-but-potentially-dangerous intrinsics to reduce our attack
> surface. Unless we have some sort of strong way to guarantee that property
> already?

There's no good way to assert this without fairly substantial overhead: we have to create a frame iterator to get the caller.

More importantly, I would argue that we already have pretty good protections in place: these intrinsics aren't available to content. The only way to change that for a particular intrinsic would be to, in a self-hosted function, explicitly take a reference to it and return it or store it on an object that is then returned. There is no reason whatsoever to do that with any intrinsic, much less one with "Unsafe" in its name, so it would have to be pretty deliberate. If our reviews let *that* through, we're all doomed anyway.
Attachment #8717665 - Flags: review?(bobbyholley)
Attachment #8717144 - Attachment is obsolete: true
Comment on attachment 8717665 [details] [diff] [review]
Part 2: Add self-hosting intrinsic for calling wrapped functions without wrapper security checks

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

Self-hosted stuff still needs to be super-carefully written to avoid interacting with the global, right? It's not just deliberately exposing the property - even doing someIntrinsic.call exposes it to content, right?

I guess it's not a unique problem here, but it still creeps me out. r=me on the patch
Attachment #8717665 - Flags: review?(bobbyholley) → review+
(In reply to Bobby Holley (busy) from comment #116)
> Self-hosted stuff still needs to be super-carefully written to avoid
> interacting with the global, right? It's not just deliberately exposing the
> property - even doing someIntrinsic.call exposes it to content, right?

someIntrinsic.call doesn't exist, partly to avoid problems like this. In general, all the builtins or C++-implemented intrinsics don't have any properties on them. For calls with a non-default receiver, you'd use callFunction. That, however, asserts when the callee is not a self-hosted function, so in those rare places where you actually want to call something that might be content-provided (Array.prototype.every, say) you have to use callContentFunction instead.
> 
> I guess it's not a unique problem here, but it still creeps me out. r=me on
> the patch

Right, it's still a lot more powerful than normal JS. We've tried pretty hard to clamp down on the footguns, and think we've got a fairly good system by now. I'd be interested in discussing further mitigations at some point, though.
Ok - that makes me feel a bit better about it.
https://hg.mozilla.org/integration/mozilla-inbound/rev/fe2ad6c8ba91a85463c753d17478e2fc5563a344
Bug 911216 - Part 2: Add self-hosting intrinsic for calling wrapped functions without wrapper security checks. r=efaust,bholley
Depends on: 1254092
Created attachment 8727867 [details] [diff] [review]
Part 3: Add --enable-sm-promise configure flag

This adds a configure flag for enabling the SpiderMonkey implementation of Promise. I'd like to land this soon, as the configure backend is so much in flux that it bitrots regularly. It'll be used once everything else works (which I hope will be Real Soon Now.)
Attachment #8709010 - Attachment is obsolete: true
Attachment #8709016 - Attachment is obsolete: true
Attachment #8711484 - Attachment is obsolete: true
Attachment #8715845 - Attachment is obsolete: true
Attachment #8715846 - Attachment is obsolete: true
Attachment #8715850 - Attachment is obsolete: true
Attachment #8715853 - Attachment is obsolete: true
Attachment #8715846 - Flags: review?(efaustbmo)
Attachment #8715850 - Flags: review?(efaustbmo)
Attachment #8715853 - Flags: feedback?(bzbarsky)
Attachment #8727867 - Flags: review?(mh+mozilla)
Attachment #8717665 - Attachment description: Part 4: Add self-hosting intrinsic for calling wrapped functions without wrapper security checks → Part 2: Add self-hosting intrinsic for calling wrapped functions without wrapper security checks
Attachment #8727867 - Attachment description: Add --enable-sm-promise configure flag → Part 3: Add --enable-sm-promise configure flag
Created attachment 8727868 [details] [diff] [review]
Part 4: Disambiguate JS Telemetry macro names

This is needed in cases where both SelfHostingDefines and jsfriendapi.h are (indirectly) included. Turning the JS_TELEMETRY enum into an enum class would also work, but is much more annoying to do.
Attachment #8727868 - Flags: review?(evilpies)
Created attachment 8727869 [details] [diff] [review]
Part 5: Allow wrapped self-hosted functions and intrinsics in the callFunction debug check
Attachment #8727869 - Flags: review?(efaustbmo)
Created attachment 8727870 [details] [diff] [review]
Part 6: Add support for running JS builtins' constructors over Xray wrappers without unwrapping the newTarget

I think this addresses all feedback. It certainly fixes the failing asserts.
Attachment #8727870 - Flags: review?(bobbyholley)
Created attachment 8727871 [details] [diff] [review]
Part 7: Add self-hosting intrinsic for creating arrays in other compartments

Needed to implement the correct semantics in Promise.all.
Attachment #8727871 - Flags: review?(efaustbmo)
Created attachment 8727872 [details] [diff] [review]
Part 8: Implement ES6 Promises in the JavaScript engine

The meat of things. Well, much of it, but there's an additional dozen or so patches to come ...
Attachment #8727872 - Flags: review?(efaustbmo)
Created attachment 8727873 [details] [diff] [review]
Part 9: Shim new promise-related Debugger.Object accessors using PromiseDebugging

We talked about this on IRC: it's very much not pretty, but it's also temporary and will fully go away once we rip out the old Promise implementation for good.
Attachment #8727873 - Flags: review?(shu)
Created attachment 8727876 [details] [diff] [review]
Part 10: Properly wrap and unwrap |then| callbacks for xrayed Promises

See the lengthy comments for explanations. This is somewhat ugly, but the alternative would be to move all of this to C++.
Attachment #8727876 - Flags: review?(efaustbmo)
Attachment #8727876 - Flags: feedback?(bzbarsky)
Created attachment 8727879 [details] [diff] [review]
Part 11: Properly handle rejecting wrapped promises in the face of xray wrappers

Somewhat of a follow-up to the previous patch. Again, the comments should hopefully explain what's going on.
Attachment #8727879 - Flags: review?(efaustbmo)
Attachment #8727879 - Flags: feedback?(bzbarsky)
Created attachment 8727881 [details] [diff] [review]
Part 12: Change Promise debugger hook tests to use Promise ctor instead of makeFakePromise
Attachment #8727881 - Flags: review?(shu)
Created attachment 8727882 [details] [diff] [review]
Part 13: Support debugger hooks for creation and settling of promises

Carrying r+.
Created attachment 8727885 [details] [diff] [review]
Part 14: Implement all Promise inspection functionality as Debugger getters

Carrying r+.
Attachment #8727882 - Flags: review+
Attachment #8727885 - Flags: review+
Created attachment 8727887 [details] [diff] [review]
Part 15: Add support for tracking unhandled promise rejections, exposed through a JSAPI function
Attachment #8727887 - Flags: review?(efaustbmo)
Created attachment 8727888 [details] [diff] [review]
Part 16: Use new Promise inspection Debugger getters to implement legacy functions on PromiseDebugging

Converting all use cases to Debugger.Object would be extremely annoying because PromiseDebugging can work in the same global as the inspected Promise, whereas I'd have to change things to add a new global for the Debugger to live in.
Attachment #8727888 - Flags: review?(bzbarsky)
Created attachment 8727890 [details] [diff] [review]
Part 17: Use promise rejection tracking to report unhandled rejections to the console
Attachment #8727890 - Flags: review?(nfitzgerald)
Attachment #8727890 - Flags: review?(bzbarsky)
Created attachment 8727891 [details] [diff] [review]
Part 18: Change DOM interface tests to assume Promise is an ES builtin, not a DOM one
Attachment #8727891 - Flags: review?(bzbarsky)
Created attachment 8727892 [details] [diff] [review]
Part 19: Remove DOM version of dependent Promises test
Attachment #8727892 - Flags: review?(bzbarsky)
Created attachment 8727893 [details] [diff] [review]
Part 20: Remove some PromiseDebugging references
Attachment #8727893 - Flags: review?(bzbarsky)
Created attachment 8727895 [details] [diff] [review]
Part 21: Adapt promise rejections test to new xray-unwrapping error
Attachment #8727895 - Flags: review?(bzbarsky)
Created attachment 8727896 [details] [diff] [review]
Part 22: Set JS execution reason when invoking Promise callbacks
Attachment #8727896 - Flags: review?(bzbarsky)
Created attachment 8727899 [details] [diff] [review]
Part 23: Fix instanceof checks that assume `promiseFromSomeGlobal instanceof otherGlobal.Promise` succeeds
Attachment #8727899 - Flags: review?(bzbarsky)
Attachment #8727899 - Attachment description: Part NN: Fix instanceof checks that assume `promiseFromSomeGlobal instanceof otherGlobal.Promise` succeeds → Part 23: Fix instanceof checks that assume `promiseFromSomeGlobal instanceof otherGlobal.Promise` succeeds
Created attachment 8727902 [details] [diff] [review]
Part 24: Fix console tracing async stack test

We talked about this on IRC: the stacks are different, and I'd argue more correct with the new implementation.
Attachment #8727902 - Flags: review?(ttromey)
Created attachment 8727906 [details] [diff] [review]
Part 25: Fix expectations in browser_timelineMarkers tests

Same as the previous patch, but for timeline markers.
Attachment #8727906 - Flags: review?(ttromey)
Created attachment 8727907 [details] [diff] [review]
Part 26: Fix DebuggeeWouldRun asserts in onNewPromise and onPromiseSettled hook tests

As discussed on IRC.
Attachment #8727907 - Flags: review?(shu)
Comment on attachment 8727876 [details] [diff] [review]
Part 10: Properly wrap and unwrap |then| callbacks for xrayed Promises

>@@ -667,45 +667,26 @@ function Promise_then(onFulfilled, onRej
>+        let handlerForwarders = GetPromiseHandlerForwarders(onFulfilled, onRejected);

Please add a comment here pointing to the documentation for GetPromiseHandlerForwarders for an explanation of what's going on.

>@@ -717,29 +698,89 @@ function EnqueuePromiseReactions(promise
>+        let handlerForwarders = GetPromiseHandlerForwarders(onFulfilled, onRejected);

Likewise.

>+ * Returns a set of functions that are exact forwarders of the passed-in
>+ * functions.

I think this should emphasize that the return values are (1) self-hosted and (2) exact forwarders.

>+ * Callers of UnwrappedPerformPromiseThen pass in the forwarded handlers
>+ * instead of the original ones. This guarantees that the handlers that end
>+ * up being used in PerformPromiseThen are wrapped functions

No, it guarantees that the handlers used in PerformPromiseThen are self-hosted functions.  They would be wrapped no matter what.   This is the part that confused me.

>, which can be
>+ * called by UnsafeCallWrappedFunction (which can only call wrapped
>+ * self-hosted functions).

This comment should clearly emphasize that the only reason for this function is to create a self-hosted function, which we can then call via UnsafeCallWrappedFunction once we cross-compartment wrap it.

>+ * A scenario that'd break without this:
>+ * 1. Privileged code gets functions from non-privileged compartment.
>+ * 2. Privileged code calls .then on a Promise created by non-privileged code.
>+ * 3. The privileged code passes the function from 1) to .then.
>+ *
>+ * This'd cause the handler to be an *unwrapped* function when it reaches
>+ * UnwrappedPerformPromiseThen

No, it would not.  It would be a cross-compartment wrapper to a non-self-hosted function, unless I'm totally misunderstanding something.  The problem would be the non-self-hosted aspect, not the "unwrapped" aspect, since it would not be "unwrapped" at all.

>+ * Forwarder used to invoke PerformPromiseThen on an unwrapped Promise, while
>+ * wrapping the resolve/reject callbacks into functions that invoke them in
>+ * their original compartment. Otherwise, calling them with objects as
>+ * arguments would throw if they're wrapped in Xray wrappers.

I'm not sure what the "otherwise" is trying to say...

Which is being wrapped in Xray wrappers?  The arguments?  The promise?  The functions?  Something else?

I think it would really help to clearly document here a scenario that would break if we were not jumping through these hoops, then explain why the way we're doing it makes it not break.
Attachment #8727876 - Flags: feedback?(bzbarsky) → feedback-
(In reply to Till Schneidereit [:till] from comment #133)
> Created attachment 8727870 [details] [diff] [review]
> Part 6: Add support for running JS builtins' constructors over Xray wrappers
> without unwrapping the newTarget
> 
> I think this addresses all feedback. It certainly fixes the failing asserts.

Can you explain to me how this patch is meaningfully different from the one I r+ed in comment 116?
Flags: needinfo?(till)
Attachment #8727868 - Flags: review?(evilpies) → review+
Comment on attachment 8727870 [details] [diff] [review]
Part 6: Add support for running JS builtins' constructors over Xray wrappers without unwrapping the newTarget

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

::: js/public/Class.h
@@ +674,5 @@
> +                                                // compartment and invoked
> +                                                // with a wrapped newTarget.
> +                                                // The constructor has to
> +                                                // detect and handle this
> +                                                // Situation.

nit: no need to capitalize.

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +935,5 @@
> +        RootedValue ctorVal(cx, ObjectValue(*ctor));
> +        HandleValueArray vals(args);
> +        RootedObject result(cx);
> +        if (!JS::Construct(cx, ctorVal, wrapper, vals, &result))
> +            return false;

Add some comments here indicating that we're explicitly invoking the constructor in the Xray compartment, and that the constructor has a handshake by which it detects this case.

@@ +936,5 @@
> +        HandleValueArray vals(args);
> +        RootedObject result(cx);
> +        if (!JS::Construct(cx, ctorVal, wrapper, vals, &result))
> +            return false;
> +        args.rval().setObject(*result);

Please AssertSameCompartment(cx, result);
Attachment #8727870 - Flags: review?(bobbyholley) → review+
Comment on attachment 8727879 [details] [diff] [review]
Part 11: Properly handle rejecting wrapped promises in the face of xray wrappers

Seems ok, on the premise that this is never really hit.

I wonder which test tickled this and how it works right now; I don't think we do this rejection-value remapping at the moment, right?
Attachment #8727879 - Flags: feedback?(bzbarsky) → feedback+
Comment on attachment 8727890 [details] [diff] [review]
Part 17: Use promise rejection tracking to report unhandled rejections to the console

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

I'll defer to :bz for this, since I'm not very familiar with the unhandled promise rejection tracking implementation.
Attachment #8727890 - Flags: review?(nfitzgerald)
Comment on attachment 8727888 [details] [diff] [review]
Part 16: Use new Promise inspection Debugger getters to implement legacy functions on PromiseDebugging

>+PromiseDebugging::GetState(GlobalObject&, JS::Handle<JSObject*> aPromise,
>+  AutoJSAPI jsapi;

Is there a reason you can't use the JSContext from the GlobalObject that was passed in?  I can't see one.

>+    obj = js::CheckedUnwrap(obj, /* stopAtWindowProxy = */ false);

You don't need that second arg, right?  It's never going to be an issue in practice for promises.  Just obj = js::CheckedUnwrap(obj).

Also, CheckeUnwrap already tests IsWrapper.  So this can just be:

  JS::Rooted<JSObject*> obj(cx, js::CheckedUnwrap(aPromise));

>+    if (aRv.Failed()) {
>+      return;

Er, no, that doesn't make sense, since you haven't .  What you want to do is check whether obj is null, and if it is throw something on aRv (MSG_IS_NOT_PROMISE seems reasonable) and return.

>+    aRv.ThrowTypeError<MSG_IS_NOT_PROMISE>(NS_LITERAL_STRING("Argument"));

"Argument of PromiseDebugging.getState", please, since you know what it is.

>+PromiseDebugging::GetPromiseID

All the same comments apply here.

>+  aID.AppendInt(uint64_t(promiseID));

I assume it will in fact never be fractional?  If so, could we make JS::GetPromiseID return uint64_t instead?

r=me
Attachment #8727888 - Flags: review?(bzbarsky) → review+
Comment on attachment 8727891 [details] [diff] [review]
Part 18: Change DOM interface tests to assume Promise is an ES builtin, not a DOM one

r=me
Attachment #8727891 - Flags: review?(bzbarsky) → review+
Comment on attachment 8727892 [details] [diff] [review]
Part 19: Remove DOM version of dependent Promises test

What's replacing this?  The commit message should probably say, if we don't add the new thing in the same changeset.  Better yet would be to add the new thing in the same changeset.  Ideally via hg mv and then edits, so we can preserve history....
Comment on attachment 8727893 [details] [diff] [review]
Part 20: Remove some PromiseDebugging references

r=me, I guess, if all the relevant tests are still passing....
Attachment #8727893 - Flags: review?(bzbarsky) → review+
Comment on attachment 8727895 [details] [diff] [review]
Part 21: Adapt promise rejections test to new xray-unwrapping error

I expect that you're going to be doing some changeset-folding here or something, right?

>+              ok(false, "One of our catch statements totally failed with err" + err + ', stack: ' + (err ? err.stack : ''));

Ah, thank you... ;)

r=me
Attachment #8727895 - Flags: review?(bzbarsky) → review+
Comment on attachment 8727896 [details] [diff] [review]
Part 22: Set JS execution reason when invoking Promise callbacks

r=me.  This could land right now, under a different bug#, just to simplify things.
Attachment #8727896 - Flags: review?(bzbarsky) → review+
Comment on attachment 8727899 [details] [diff] [review]
Part 23: Fix instanceof checks that assume `promiseFromSomeGlobal instanceof otherGlobal.Promise` succeeds

r=me on the first and last hunks.  Those could land right now, and should, under a separate bug#.

The middle hunk can't land until we actually turn on spidermonkey promises, so presumably should be in the same changeset as the actual configure switch flip, as should the other test changes above.  That would be in this bug.  r=me with that.
Attachment #8727899 - Flags: review?(bzbarsky) → review+

Comment 166

2 years ago
Comment on attachment 8727906 [details] [diff] [review]
Part 25: Fix expectations in browser_timelineMarkers tests

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

Looks good.
Attachment #8727906 - Flags: review?(ttromey) → review+

Comment 167

2 years ago
Comment on attachment 8727902 [details] [diff] [review]
Part 24: Fix console tracing async stack test

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

I think this new result is somewhat worse than the status quo ante.  It no longer mentions promises, which seems relevant to the user.

However, it doesn't seem worse enough to warrant rejection.  My belief is that the result will still guide users to the code they're interested in.
Attachment #8727902 - Flags: review?(ttromey) → review+
Comment on attachment 8727867 [details] [diff] [review]
Part 3: Add --enable-sm-promise configure flag

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

This is a good candidate to happen in python configure directly. It's better to do so than to land it in old-configure and migrate it afterwards. I can either do it, or give you hints how to do it, whichever you prefer, but I first need to know if you need SPIDERMONKEY_PROMISE on the gecko side or if you need it in spidermonkey only?
Attachment #8727867 - Flags: review?(mh+mozilla)
We need it on the Gecko side.
Depends on: 1254873
Created attachment 8728281 [details]
MozReview Request: Bug 911216 - Add --enable-sm-promise configure flag

Review commit: https://reviewboard.mozilla.org/r/38893/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/38893/
Attachment #8728281 - Flags: review?(cmanchester)
Attachment #8727867 - Attachment is obsolete: true
Depends on: 1254943
Depends on: 1254947
Depends on: 1254966
Depends on: 1254968
(In reply to Boris Zbarsky [:bz] from comment #157)
> Comment on attachment 8727879 [details] [diff] [review]
> Part 11: Properly handle rejecting wrapped promises in the face of xray
> wrappers
> 
> Seems ok, on the premise that this is never really hit.
> 
> I wonder which test tickled this and how it works right now; I don't think
> we do this rejection-value remapping at the moment, right?

It is hit in practice - see the test I changed in attachment 8727895 [details] [diff] [review]. The way we do this right now is pretty subtle, I remember that it took me some time to figure it out. We have a comment about it, though:
https://dxr.mozilla.org/mozilla-central/source/dom/promise/Promise.cpp#1134-1148
Flags: needinfo?(till)
Created attachment 8728475 [details] [diff] [review]
Part 10: Properly wrap and unwrap |then| callbacks for xrayed Promises

I rewrote the comments quite substantially to address bz's feedback. Hopefully they're more clear now.
Attachment #8728475 - Flags: review?(efaustbmo)
Attachment #8728475 - Flags: feedback?(bzbarsky)
Attachment #8727876 - Attachment is obsolete: true
Attachment #8727876 - Flags: review?(efaustbmo)
Attachment #8728281 - Flags: review?(cmanchester) → review+
Comment on attachment 8728281 [details]
MozReview Request: Bug 911216 - Add --enable-sm-promise configure flag

https://reviewboard.mozilla.org/r/38893/#review35693
Created attachment 8728986 [details] [diff] [review]
Part 12: Change Promise debugger hook tests to use Promise ctor instead of makeFakePromise

Turns out skip-if doesn't work for jit-tests, so I changed bug-1102549.js to a stupid workaround instead.
Attachment #8728986 - Flags: review?(shu)
Attachment #8727881 - Attachment is obsolete: true
Attachment #8727881 - Flags: review?(shu)

Comment 175

2 years ago
Comment on attachment 8727902 [details] [diff] [review]
Part 24: Fix console tracing async stack test

Hm, the new async stack trace looks suspect, I'd like to make sure the implications of this test change are understood. I recommend verifying a few things.

DOM promises currently provide the allocation stack (the place where "new Promise()" is called) to the callbacks:

http://mxr.mozilla.org/mozilla-central/source/dom/promise/Promise.cpp#94

Unfortunately the test does "new Promise(function(resolve, reject) {" on the same line:

http://mxr.mozilla.org/mozilla-central/source/devtools/client/webconsole/test/test-console-trace-async.html?force=1#13

This might lead to confusion as to whether in the new stack we are still seeing the place where "new Promise()" is called, or the place where "resolve" is called, which in this case happens to be in the function passed to the Promise constructor.

This might seem OK because calling resolve directly inside the function provided to the constructor is a common pattern, but there is also the Deferred object pattern where a reference to "resolve" is stored to be called later.

The fact we're not seeing any involvement of Promise in the async stack is also suspect.

I'd make sure, with an additional test case if one doesn't exist already elsewhere, that the async stack is still correct if we resolve the promise _before_ attaching the "then" function to the resolved promise. I guess we don't want to lose track of where the Promise was created in this case.

It's also not clear to me what bug 1254943 is about if it doesn't take effect here.
Attachment #8727902 - Flags: review+ → review?(bzbarsky)

Comment 176

2 years ago
Comment on attachment 8727890 [details] [diff] [review]
Part 17: Use promise rejection tracking to report unhandled rejections to the console

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

I'm a Toolkit peer so I'm reviewing the Toolkit part only.

::: toolkit/modules/tests/PromiseTestUtils.jsm
@@ +65,5 @@
>  
>      // Promise.jsm rejections are only reported to this observer when requested,
>      // so we don't have to store a key to remove them when consumed.
>      JSMPromise.Debugging.addUncaughtErrorObserver(
> +                            rejection => this._rejections.set({}, rejection));

Clever trick to store an arbitrary number of entries in a Map :-) But see below...

@@ +147,5 @@
>      } catch (ex) {}
>  
>      // It's important that we don't store any reference to the provided Promise
>      // object or its value after this function returns in order to avoid leaks.
> +    this._rejections.set(promise, {

The reason we don't store the Promise object anywhere is that this would prevent it from being garbage collected, and this generally keeps a lot of other functions and closures reachable, likely causing leaks as the comment says. It's much rarer (though possible) to have that situation for rejection reasons.

It's true that in this case we'd likely fail the test soon anyways, but we know code gets copied and pasted, and it's best to stick to the general recommendation for onLeftUncaught and discard any reference to the Promise object that is provided.

@@ +152,3 @@
>        message,
>        date: new Date(),
> +      stack: PromiseDebugging.getRejectionStack ? PromiseDebugging.getRejectionStack(promise) : null,

I'd really like this stack to be available. It's difficult to debug a test failure without any hint about where it occurred and what it is about! This would obviously also break our treeherder classifications for current failures.

I should have added tests that the stack trace is present to xpcshell's selftests.py. If you feel like adding a test for this, it would be appreciated.

@@ +213,5 @@
>                  `A promise chain failed to handle a rejection:` +
>                  ` ${rejection.message} - rejection date: ${rejection.date}`+
>                  ` - stack: ${rejection.stack}`);
>      }
> +    this._rejections.clear();

The reason the code was done the way it was done is that Assert may throw, in which case we want to still have removed the single value from the array.

I'm not sure how you could use a Map here, I suspect calling "delete" during iteration might lead to unexpected results?
Attachment #8727890 - Flags: review-
Comment on attachment 8727902 [details] [diff] [review]
Part 24: Fix console tracing async stack test

Yeah, it's a bit weird that we totally lose track of the "promise callback" bit here.

Fundamentally, there are kind of two async reasons why a promise callback is called: the fact that we passed it to then() and the fact that the promise was resolved.  It looks like we used to show the former as the async reason and now we're showing the latter, right?  It's not clear to me which is more useful: the then() bit explains why we're called but the "where Promise was resolved" bit shows where our argument value comes from...
Attachment #8727902 - Flags: review?(bzbarsky)
Comment on attachment 8728475 [details] [diff] [review]
Part 10: Properly wrap and unwrap |then| callbacks for xrayed Promises

Thanks, this is much clearer!
Attachment #8728475 - Flags: feedback?(bzbarsky) → feedback+
> It is hit in practice - see the test I changed in attachment 8727895 [details] [diff] [review]

Ah, hmm.  So in that test, we're checking that content doesn't see the original chrome-side exception... but that test does NOT test that the original chrome-side exception is reported to the browser console.  Which of course it should be.  

Have you checked whether things are properly reported in the browser console in that test after these patches?

Comment 180

2 years ago
Comment on attachment 8727873 [details] [diff] [review]
Part 9: Shim new promise-related Debugger.Object accessors using PromiseDebugging

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

Nit patch-wide: "" instead of ''

Man the legacy code is very scary. unsafeDereference everywhere!
Attachment #8727873 - Flags: review?(shu) → review+

Comment 181

2 years ago
Comment on attachment 8727907 [details] [diff] [review]
Part 26: Fix DebuggeeWouldRun asserts in onNewPromise and onPromiseSettled hook tests

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

::: js/src/jit-test/tests/debug/Debugger-onNewPromise-05.js
@@ +15,5 @@
>    assertEq(promise.seen, undefined);
>    promise.seen = true;
>  
> +  if (depth < 2)
> +      gw.executeInGlobal(`new Promise(_=>{})`);

Why'd you change the depth? Decided 2 was without loss of generality?

::: js/src/jit-test/tests/debug/Debugger-onPromiseSettled-05.js
@@ +16,5 @@
>    promise.seen = true;
>  
> +  if (depth < 2) {
> +    let promise = gw.executeInGlobal(`new Promise(_=>{})`).return.unsafeDereference();
> +    g.settlePromiseNow(promise);

Hm I guess settlePromiseNow is a C++-implemented thing and so doesn't trigger DebuggeeWouldRun?
Attachment #8727907 - Flags: review?(shu) → review+

Comment 182

2 years ago
Comment on attachment 8728986 [details] [diff] [review]
Part 12: Change Promise debugger hook tests to use Promise ctor instead of makeFakePromise

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

::: js/src/jit-test/lib/debuggerNXHelper.js
@@ +31,5 @@
>  
>    testDebuggerHook("onNewGlobalObject",
>                     () => { newGlobal(); });
>  
> +  if ('Promise' in g) {

What builds do not have the Promise constructor?
Attachment #8728986 - Flags: review?(shu) → review+
> What builds do not have the Promise constructor?

In shell?  Ones without SPIDERMONKEY_PROMISE, right?
(In reply to Boris Zbarsky [:bz] from comment #183)
> > What builds do not have the Promise constructor?
> 
> In shell?  Ones without SPIDERMONKEY_PROMISE, right?

Exactly. I changed the tests to just not do anything if we don't have the Promise ctor. The reasoning is that we really, really want to just flip the switch and keep them on, and SPIDERMONKEY_PROMISE is our emergency escape hatch in case we have to revert that late in the cycle for some unknown reason. In that case, losing a bit of test coverage doesn't seem too bad. Especially compared to the lengths I'd have to go to in order to keep the coverage.

Updated

2 years ago
Attachment #8727869 - Flags: review?(efaustbmo) → review+

Updated

2 years ago
Attachment #8727871 - Flags: review?(efaustbmo) → review+
Depends on: 1257030

Updated

2 years ago
Blocks: 1257172
Comment on attachment 8727872 [details] [diff] [review]
Part 8: Implement ES6 Promises in the JavaScript engine

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

Whew! OK. Most of this is nits and requests for comments.

Thanks for putting Promise.js in more-or-less specwise order. That helped a lot.

::: js/src/builtin/Promise.cpp
@@ +110,5 @@
> +    args.setCallee(resolvingFunctionsVal);
> +    args.setThis(UndefinedValue());
> +    args[0].set(promiseVal);
> +
> +    if (!Invoke(cx, args))

Man, the C++->self-hosted boundary is heavyweight. Totally not your fault, though.

@@ +117,5 @@
> +    RootedArrayObject resolvingFunctions(cx, &args.rval().toObject().as<ArrayObject>());
> +    RootedValue resolveVal(cx, resolvingFunctions->getDenseElement(0));
> +    MOZ_ASSERT(IsCallable(resolveVal));
> +    RootedValue rejectVal(cx, resolvingFunctions->getDenseElement(1));
> +    MOZ_ASSERT(IsCallable(rejectVal));

nit: this runs together a little to my eye. Prefer either a line break between the resolve and reject bits, or moving the asserts to after rooting both.

@@ +135,5 @@
> +    // Now unwrap the resolution functions again to call the executor with
> +    // them as arguments.
> +    if (wrappedProto &&
> +        !(proto->compartment()->wrap(cx, &resolveVal) &&
> +          proto->compartment()->wrap(cx, &rejectVal)))

Why can't we just use a second rooted to wrap and store in the fixed slots, and then we don't have to wrap them back, here, right? This seems subtle, so maybe I'm missing something?

If for some reason we have to keep these, can this be cx->compartment, instead? I don't want to have to consider whether that object was wrapped or not when reading this.

@@ +152,5 @@
> +    bool success = Invoke(cx, args2);
> +
> +    // Step 10.
> +    if (!success) {
> +        if (!cx->isExceptionPending())

OOM, right? Can we comment to this effect, or maybe an assert that cx->isThrowingOutOfMemory()?

@@ +197,5 @@
> +    bool needsWrapping = false;
> +
> +    // If the constructor is called via an Xray wrapper, then the newTarget
> +    // hasn't been unwrapped. We want that because, while the actual instance
> +    // should be created in the wrappee's compartment, the constructor's code

"target compartment", please. Wrapee might be better, but we have an existing lexicon already.

@@ +231,5 @@
> +            AutoCompartment ac(cx, newTarget);
> +            RootedObject promiseCtor(cx);
> +            if (!GetBuiltinConstructor(cx, JSProto_Promise, &promiseCtor))
> +                return false;
> +            if (newTarget == promiseCtor)

// Since subclasses don't get Xrays, we only need to do the song
// and dance if it's the builtin constructor

or something. Needs a liiiitle bit more, despite the helpful block.

@@ +239,5 @@
> +
> +    RootedObject proto(cx);
> +    if (!GetPrototypeFromConstructor(cx, newTarget, &proto))
> +        return false;
> +    if (needsWrapping && !originalNewTarget->compartment()->wrap(cx, &proto))

I still prefer cx->compartment()->wrap(), but I could be convinced to leave this, since it's obviously trying to make it explicit which one we're using, and AutoCompartment makes cx unreliable in that sense.

@@ +248,5 @@
> +
> +    // Step 11.
> +    args.rval().setObject(*promise);
> +    if (needsWrapping)
> +        return originalNewTarget->compartment()->wrap(cx, args.rval());

and here.

@@ +261,5 @@
> +    if (this->getFixedSlot(PROMISE_STATE_SLOT).toInt32() != unsigned(JS::PromiseState::Pending))
> +        return true;
> +    RootedValue funVal(cx, this->getReservedSlot(PROMISE_RESOLVE_FUNCTION_SLOT));
> +    MOZ_ASSERT(funVal.toObject().is<JSFunction>());
> +    InvokeArgs args(cx);

nit: blank line above either InvokeArgs or funVal decl.

@@ +277,5 @@
> +    if (this->getFixedSlot(PROMISE_STATE_SLOT).toInt32() != unsigned(JS::PromiseState::Pending))
> +        return true;
> +    RootedValue funVal(cx, this->getReservedSlot(PROMISE_REJECT_FUNCTION_SLOT));
> +    MOZ_ASSERT(funVal.toObject().is<JSFunction>());
> +    InvokeArgs args(cx);

same here.

::: js/src/builtin/Promise.js
@@ +14,5 @@
> +    // they're created in a higher-privileged compartment from the Promise,
> +    // so they can be invoked seamlessly by code in that compartment.
> +    //
> +    // See the comment in PromiseConstructor (in builtin/Promise.cpp) for more
> +    // details. 

nit: whitesapce after "details."

@@ +56,5 @@
> +
> +        // Step 7.
> +        if (!IsObject(resolution)) {
> +            if (unwrap) {
> +                return callFunction(CallPromiseMethodIfWrapped, promise, resolution,

A helper for resolving and rejecting based on unwrap and the exception might cut down on some of the verbosity here. Maybe it's not worth it.

@@ +267,5 @@
> +// ES6, 25.4.2.2.
> +function EnqueuePromiseResolveThenableJob(promiseToResolve, thenable, then) {
> +    _EnqueuePromiseJob(function PromiseResolveThenableJob() {
> +        // Step 1.
> +        let {0: resolve, 1: reject} = CreateResolvingFunctions(promiseToResolve);

cute.

@@ +335,5 @@
> +    // Immediately create an Array instead of a List, so we can skip step 6.d.iii.1.
> +    //
> +    // We might be dealing with a wrapped instance from another Realm. In that
> +    // case, we want to create the `values` array in that other Realm so if 
> +    // it's less-privileged than the current one, code in that Realm can still

nit: whitespace after "so if"

@@ +355,5 @@
> +    let nextValue;
> +    while (true) {
> +        try {
> +            // Step 6.a.
> +            next = callContentFunction(iterator.next, iterator);

do we really not have a helpr for this in Iterator.js?

@@ +446,5 @@
> +        assert(callFunction(std_Object_hasOwnProperty, promises, i),
> +               "GetWaitForAllPromise must be called with a dense array of promises");
> +        let nextPromise = promises[i];
> +        assert(IsPromise(nextPromise) || IsWrappedPromise(nextPromise),
> +               "promises list must only contain, maybe wrapped, promises");

I would change this to "promises list must only contain possibly wrapped promises". It seems to read better that way.

@@ +534,5 @@
> +        // Steps 7,9.
> +        return PerformPromiseRace(iteratorRecord, promiseCapability, C);
> +    } catch (e) {
> +        // Step 8.a.
> +        // TODO: implement iterator closing.

Nice that you do all the iteratorRecord plumbing, even though nobody is watching.

@@ +683,5 @@
> +/**
> + * Forwarder used to invoke PerformPromiseThen on an unwrapped Promise, while
> + * wrapping the resolve/reject callbacks into functions that invoke them in
> + * their original compartment. Otherwise, calling them with objects as 
> + * arguments would throw if they're wrapped in Xray wrappers.

nit: whitespace after "objects as"

@@ +713,5 @@
> +function EnqueuePromiseReactions(promise, dependentPromise, onFulfilled, onRejected) {
> +    let isWrappedPromise = false;
> +    if (!IsPromise(promise)) {
> +        assert(IsWrappedPromise(promise),
> +               "EnqueuePromiseReactions must be provided with a, maybe wrapped, promise");

and here, "possibly wrapped"

@@ +718,5 @@
> +        isWrappedPromise = true;
> +    }
> +
> +    assert(dependentPromise === null || IsPromise(dependentPromise),
> +           "EnqueuePromiseReactions' dependentPromise argument must be a Promise or null");

nit: EnqueuePromiseReactions's. As weird as it looks, there's not more than one function.

::: js/src/builtin/SelfHostingDefines.h
@@ +61,5 @@
> +#define PROMISE_RESULT_SLOT            1
> +#define PROMISE_FULFILL_REACTIONS_SLOT 2
> +#define PROMISE_REJECT_REACTIONS_SLOT  3
> +#define PROMISE_RESOLVE_FUNCTION_SLOT  4
> +#define PROMISE_REJECT_FUNCTION_SLOT   5

Nuh-uh. I don't like having these here and the count that has to be in sync with them in builtin/Promise.h, not one bit. No sir, not one bit. If you are totally tied to splitting them, and not making the count another define here, then please at least add a big comment, since I don't see a way to static assert this.

::: js/src/jsapi.cpp
@@ +4681,5 @@
> +
> +JS_PUBLIC_API(JS::PromiseState)
> +JS::GetPromiseState(JS::HandleObject promise)
> +{
> +    return promise->as<PromiseObject>().state();

In future, it may be useful to make these CheckedUnwrap as well. Since the caller knows everything about the wrapping state, it seems fine, but it's a liiiitle weird that you can do

if (JS::IsPromiseObject(obj))
    return JS::GetPromiseState(obj); // May assert!

@@ +4775,5 @@
> +    args[1].setNull();
> +    args[2].setObject(*onResolve);
> +    args[3].setObject(*onReject);
> +
> +    return !!js::CallSelfHostedFunction(cx, "EnqueuePromiseReactions", args);

!! is unnecessary, since CallSelfHostedFunction return bool.

::: js/src/jsapi.h
@@ +4331,5 @@
> + */
> +extern JS_PUBLIC_API(JSObject*)
> +GetPromisePrototype(JSContext* cx);
> +
> +// Keep this in sync with the PROMISE_STATE defines in SelfHostingDefines.h.

I understand why, but :(. I am open to finding unified places to store this information.

::: js/src/shell/js.cpp
@@ +635,5 @@
> +        return true;
> +    RootedObject job(cx);
> +    JS::HandleValueArray args(JS::HandleValueArray::empty());
> +    RootedValue rval(cx);
> +    for (size_t i = 0; i < sr->jobQueue.length(); i++) {

This loop is actually somewhat subtle. Calling a job can enqueue new jobs, so checking jobQueue.length() is *essential to correctness*. This needs a comment, since it's an intentional usage of a common performance anti-pattern.

@@ +909,5 @@
>      return true;
>  }
>  
>  static bool
> +CreatePromiseThroughJSAPI(JSContext* cx, unsigned argc, Value* vp)

So, while I don't strictly object to having these as shell-only testing functions, it's certinaly more direct to test the jsapi with jsapi-tests. If that's all we're doing here, I would mildly prefer that.

::: js/src/tests/ecma_6/Promise/enqueue-promise-reactions.js
@@ +1,4 @@
> +// |reftest| skip-if(!xulRuntime.shell) -- needs getSelfHostedValue and drainJobQueue
> +
> +if (!this.Promise) {
> +    this.reportCompare && reportCompare(true,true);

I think the idiom is to use typeof === 'function', but this is fine.

::: js/src/tests/ecma_6/Promise/promise-all.js
@@ +11,5 @@
> +  .then(val=>{results.push('then ' + val); return 'first then rval';})
> +  .then(val=>{results.push('chained then with val: ' + val); return 'p1 then, then'});
> +
> +let p2 = new Promise((res, rej)=>rej('rejection'))
> +  .catch(val=>results.push('catch ' + val))

Can we do braces with an explicit return, here? It took me 10 minutes and a direct ping on IRC to notice why this should resolve with 2. Every other results.push in this test is not returned.

::: js/src/tests/ecma_6/Promise/promise-basics.js
@@ +11,5 @@
> +  .then(val=>{results.push('then ' + val); return 'first then rval';})
> +  .then(val=>results.push('chained then with val: ' + val));
> +
> +new Promise((res, rej)=>rej('rejection'))
> +  .catch(val=>results.push('catch ' + val))

ah, I'm wise to it now, but this should also be rewritten.

::: js/src/tests/ecma_6/Promise/promise-subclassing.js
@@ +6,5 @@
> +}
> +
> +let results = [];
> +
> +class SubPromise extends Promise {

Can you add a comment referencing this test to ecma_6/Class/extendBuiltinConstructors.js? I agree this is more in depth than what's there, but I would like to keep that as a place where we keep all the basic subclass instancing testing information.

@@ +27,5 @@
> +let allSubPromise = SubPromise.all([subPromise]);
> +
> +assertEq(subPromise instanceof SubPromise, true);
> +assertEq(allSubPromise instanceof SubPromise, true);
> +assertEq(intermediatePromise instanceof SubPromise, true);

Good.

::: js/src/tests/ecma_6/Promise/resolve-reject-jsapi-functions.js
@@ +12,5 @@
> +let rejectVal;
> +
> +var p = new Promise(function() {});
> +EnqueuePromiseReactions(p, null, function(val) {resolveVal = val;});
> +resolvePromiseThroughJSAPI(p, 1);

Arguably, this could be a jsapi-test. Up to you.

::: js/src/vm/Runtime.cpp
@@ +771,5 @@
>  
> +bool
> +JSRuntime::enqueuePromiseJob(JSContext* cx, HandleFunction job) {
> +    if (!cx->runtime()->enqueuePromiseJobCallback) {
> +        JS_ReportErrorNumber(cx, GetErrorMessage, nullptr,

If we can, I'd rather assert here, than throw. Anybody who doesn't set necessary hooks is improperly embedding, in my world view. It's not like we're gonna succeed to call through nullptr anyway.

::: js/src/vm/SelfHosting.cpp
@@ +1694,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    MOZ_ASSERT(args.length() == 1);
> +
> +    RootedObject obj(cx, &args[0].toObject());
> +    MOZ_ASSERT(!obj->is<PromiseObject>());

this assert seems amazingly harsh for the function's name. I would expect IsWrapper(obj) && IsPromiseObject(obj).

What does this protect against?
Attachment #8727872 - Flags: review?(efaustbmo) → review+
Comment on attachment 8727872 [details] [diff] [review]
Part 8: Implement ES6 Promises in the JavaScript engine

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

Whew! OK. Most of this is nits and requests for comments.

Thanks for putting Promise.js in more-or-less specwise order. That helped a lot.

::: js/src/builtin/Promise.cpp
@@ +110,5 @@
> +    args.setCallee(resolvingFunctionsVal);
> +    args.setThis(UndefinedValue());
> +    args[0].set(promiseVal);
> +
> +    if (!Invoke(cx, args))

Man, the C++->self-hosted boundary is heavyweight. Totally not your fault, though.

@@ +117,5 @@
> +    RootedArrayObject resolvingFunctions(cx, &args.rval().toObject().as<ArrayObject>());
> +    RootedValue resolveVal(cx, resolvingFunctions->getDenseElement(0));
> +    MOZ_ASSERT(IsCallable(resolveVal));
> +    RootedValue rejectVal(cx, resolvingFunctions->getDenseElement(1));
> +    MOZ_ASSERT(IsCallable(rejectVal));

nit: this runs together a little to my eye. Prefer either a line break between the resolve and reject bits, or moving the asserts to after rooting both.

@@ +135,5 @@
> +    // Now unwrap the resolution functions again to call the executor with
> +    // them as arguments.
> +    if (wrappedProto &&
> +        !(proto->compartment()->wrap(cx, &resolveVal) &&
> +          proto->compartment()->wrap(cx, &rejectVal)))

Why can't we just use a second rooted to wrap and store in the fixed slots, and then we don't have to wrap them back, here, right? This seems subtle, so maybe I'm missing something?

If for some reason we have to keep these, can this be cx->compartment, instead? I don't want to have to consider whether that object was wrapped or not when reading this.

@@ +152,5 @@
> +    bool success = Invoke(cx, args2);
> +
> +    // Step 10.
> +    if (!success) {
> +        if (!cx->isExceptionPending())

OOM, right? Can we comment to this effect, or maybe an assert that cx->isThrowingOutOfMemory()?

@@ +197,5 @@
> +    bool needsWrapping = false;
> +
> +    // If the constructor is called via an Xray wrapper, then the newTarget
> +    // hasn't been unwrapped. We want that because, while the actual instance
> +    // should be created in the wrappee's compartment, the constructor's code

"target compartment", please. Wrapee might be better, but we have an existing lexicon already.

@@ +231,5 @@
> +            AutoCompartment ac(cx, newTarget);
> +            RootedObject promiseCtor(cx);
> +            if (!GetBuiltinConstructor(cx, JSProto_Promise, &promiseCtor))
> +                return false;
> +            if (newTarget == promiseCtor)

// Since subclasses don't get Xrays, we only need to do the song
// and dance if it's the builtin constructor

or something. Needs a liiiitle bit more, despite the helpful block.

@@ +239,5 @@
> +
> +    RootedObject proto(cx);
> +    if (!GetPrototypeFromConstructor(cx, newTarget, &proto))
> +        return false;
> +    if (needsWrapping && !originalNewTarget->compartment()->wrap(cx, &proto))

I still prefer cx->compartment()->wrap(), but I could be convinced to leave this, since it's obviously trying to make it explicit which one we're using, and AutoCompartment makes cx unreliable in that sense.

@@ +248,5 @@
> +
> +    // Step 11.
> +    args.rval().setObject(*promise);
> +    if (needsWrapping)
> +        return originalNewTarget->compartment()->wrap(cx, args.rval());

and here.

@@ +261,5 @@
> +    if (this->getFixedSlot(PROMISE_STATE_SLOT).toInt32() != unsigned(JS::PromiseState::Pending))
> +        return true;
> +    RootedValue funVal(cx, this->getReservedSlot(PROMISE_RESOLVE_FUNCTION_SLOT));
> +    MOZ_ASSERT(funVal.toObject().is<JSFunction>());
> +    InvokeArgs args(cx);

nit: blank line above either InvokeArgs or funVal decl.

@@ +277,5 @@
> +    if (this->getFixedSlot(PROMISE_STATE_SLOT).toInt32() != unsigned(JS::PromiseState::Pending))
> +        return true;
> +    RootedValue funVal(cx, this->getReservedSlot(PROMISE_REJECT_FUNCTION_SLOT));
> +    MOZ_ASSERT(funVal.toObject().is<JSFunction>());
> +    InvokeArgs args(cx);

same here.

::: js/src/builtin/Promise.js
@@ +14,5 @@
> +    // they're created in a higher-privileged compartment from the Promise,
> +    // so they can be invoked seamlessly by code in that compartment.
> +    //
> +    // See the comment in PromiseConstructor (in builtin/Promise.cpp) for more
> +    // details. 

nit: whitesapce after "details."

@@ +56,5 @@
> +
> +        // Step 7.
> +        if (!IsObject(resolution)) {
> +            if (unwrap) {
> +                return callFunction(CallPromiseMethodIfWrapped, promise, resolution,

A helper for resolving and rejecting based on unwrap and the exception might cut down on some of the verbosity here. Maybe it's not worth it.

@@ +267,5 @@
> +// ES6, 25.4.2.2.
> +function EnqueuePromiseResolveThenableJob(promiseToResolve, thenable, then) {
> +    _EnqueuePromiseJob(function PromiseResolveThenableJob() {
> +        // Step 1.
> +        let {0: resolve, 1: reject} = CreateResolvingFunctions(promiseToResolve);

cute.

@@ +335,5 @@
> +    // Immediately create an Array instead of a List, so we can skip step 6.d.iii.1.
> +    //
> +    // We might be dealing with a wrapped instance from another Realm. In that
> +    // case, we want to create the `values` array in that other Realm so if 
> +    // it's less-privileged than the current one, code in that Realm can still

nit: whitespace after "so if"

@@ +355,5 @@
> +    let nextValue;
> +    while (true) {
> +        try {
> +            // Step 6.a.
> +            next = callContentFunction(iterator.next, iterator);

do we really not have a helpr for this in Iterator.js?

@@ +446,5 @@
> +        assert(callFunction(std_Object_hasOwnProperty, promises, i),
> +               "GetWaitForAllPromise must be called with a dense array of promises");
> +        let nextPromise = promises[i];
> +        assert(IsPromise(nextPromise) || IsWrappedPromise(nextPromise),
> +               "promises list must only contain, maybe wrapped, promises");

I would change this to "promises list must only contain possibly wrapped promises". It seems to read better that way.

@@ +534,5 @@
> +        // Steps 7,9.
> +        return PerformPromiseRace(iteratorRecord, promiseCapability, C);
> +    } catch (e) {
> +        // Step 8.a.
> +        // TODO: implement iterator closing.

Nice that you do all the iteratorRecord plumbing, even though nobody is watching.

@@ +683,5 @@
> +/**
> + * Forwarder used to invoke PerformPromiseThen on an unwrapped Promise, while
> + * wrapping the resolve/reject callbacks into functions that invoke them in
> + * their original compartment. Otherwise, calling them with objects as 
> + * arguments would throw if they're wrapped in Xray wrappers.

nit: whitespace after "objects as"

@@ +713,5 @@
> +function EnqueuePromiseReactions(promise, dependentPromise, onFulfilled, onRejected) {
> +    let isWrappedPromise = false;
> +    if (!IsPromise(promise)) {
> +        assert(IsWrappedPromise(promise),
> +               "EnqueuePromiseReactions must be provided with a, maybe wrapped, promise");

and here, "possibly wrapped"

@@ +718,5 @@
> +        isWrappedPromise = true;
> +    }
> +
> +    assert(dependentPromise === null || IsPromise(dependentPromise),
> +           "EnqueuePromiseReactions' dependentPromise argument must be a Promise or null");

nit: EnqueuePromiseReactions's. As weird as it looks, there's not more than one function.

::: js/src/builtin/SelfHostingDefines.h
@@ +61,5 @@
> +#define PROMISE_RESULT_SLOT            1
> +#define PROMISE_FULFILL_REACTIONS_SLOT 2
> +#define PROMISE_REJECT_REACTIONS_SLOT  3
> +#define PROMISE_RESOLVE_FUNCTION_SLOT  4
> +#define PROMISE_REJECT_FUNCTION_SLOT   5

Nuh-uh. I don't like having these here and the count that has to be in sync with them in builtin/Promise.h, not one bit. No sir, not one bit. If you are totally tied to splitting them, and not making the count another define here, then please at least add a big comment, since I don't see a way to static assert this.

::: js/src/jsapi.cpp
@@ +4681,5 @@
> +
> +JS_PUBLIC_API(JS::PromiseState)
> +JS::GetPromiseState(JS::HandleObject promise)
> +{
> +    return promise->as<PromiseObject>().state();

In future, it may be useful to make these CheckedUnwrap as well. Since the caller knows everything about the wrapping state, it seems fine, but it's a liiiitle weird that you can do

if (JS::IsPromiseObject(obj))
    return JS::GetPromiseState(obj); // May assert!

@@ +4775,5 @@
> +    args[1].setNull();
> +    args[2].setObject(*onResolve);
> +    args[3].setObject(*onReject);
> +
> +    return !!js::CallSelfHostedFunction(cx, "EnqueuePromiseReactions", args);

!! is unnecessary, since CallSelfHostedFunction return bool.

::: js/src/jsapi.h
@@ +4331,5 @@
> + */
> +extern JS_PUBLIC_API(JSObject*)
> +GetPromisePrototype(JSContext* cx);
> +
> +// Keep this in sync with the PROMISE_STATE defines in SelfHostingDefines.h.

I understand why, but :(. I am open to finding unified places to store this information.

::: js/src/shell/js.cpp
@@ +635,5 @@
> +        return true;
> +    RootedObject job(cx);
> +    JS::HandleValueArray args(JS::HandleValueArray::empty());
> +    RootedValue rval(cx);
> +    for (size_t i = 0; i < sr->jobQueue.length(); i++) {

This loop is actually somewhat subtle. Calling a job can enqueue new jobs, so checking jobQueue.length() is *essential to correctness*. This needs a comment, since it's an intentional usage of a common performance anti-pattern.

@@ +909,5 @@
>      return true;
>  }
>  
>  static bool
> +CreatePromiseThroughJSAPI(JSContext* cx, unsigned argc, Value* vp)

So, while I don't strictly object to having these as shell-only testing functions, it's certinaly more direct to test the jsapi with jsapi-tests. If that's all we're doing here, I would mildly prefer that.

::: js/src/tests/ecma_6/Promise/enqueue-promise-reactions.js
@@ +1,4 @@
> +// |reftest| skip-if(!xulRuntime.shell) -- needs getSelfHostedValue and drainJobQueue
> +
> +if (!this.Promise) {
> +    this.reportCompare && reportCompare(true,true);

I think the idiom is to use typeof === 'function', but this is fine.

::: js/src/tests/ecma_6/Promise/promise-all.js
@@ +11,5 @@
> +  .then(val=>{results.push('then ' + val); return 'first then rval';})
> +  .then(val=>{results.push('chained then with val: ' + val); return 'p1 then, then'});
> +
> +let p2 = new Promise((res, rej)=>rej('rejection'))
> +  .catch(val=>results.push('catch ' + val))

Can we do braces with an explicit return, here? It took me 10 minutes and a direct ping on IRC to notice why this should resolve with 2. Every other results.push in this test is not returned.

::: js/src/tests/ecma_6/Promise/promise-basics.js
@@ +11,5 @@
> +  .then(val=>{results.push('then ' + val); return 'first then rval';})
> +  .then(val=>results.push('chained then with val: ' + val));
> +
> +new Promise((res, rej)=>rej('rejection'))
> +  .catch(val=>results.push('catch ' + val))

ah, I'm wise to it now, but this should also be rewritten.

::: js/src/tests/ecma_6/Promise/promise-subclassing.js
@@ +6,5 @@
> +}
> +
> +let results = [];
> +
> +class SubPromise extends Promise {

Can you add a comment referencing this test to ecma_6/Class/extendBuiltinConstructors.js? I agree this is more in depth than what's there, but I would like to keep that as a place where we keep all the basic subclass instancing testing information.

@@ +27,5 @@
> +let allSubPromise = SubPromise.all([subPromise]);
> +
> +assertEq(subPromise instanceof SubPromise, true);
> +assertEq(allSubPromise instanceof SubPromise, true);
> +assertEq(intermediatePromise instanceof SubPromise, true);

Good.

::: js/src/tests/ecma_6/Promise/resolve-reject-jsapi-functions.js
@@ +12,5 @@
> +let rejectVal;
> +
> +var p = new Promise(function() {});
> +EnqueuePromiseReactions(p, null, function(val) {resolveVal = val;});
> +resolvePromiseThroughJSAPI(p, 1);

Arguably, this could be a jsapi-test. Up to you.

::: js/src/vm/Runtime.cpp
@@ +771,5 @@
>  
> +bool
> +JSRuntime::enqueuePromiseJob(JSContext* cx, HandleFunction job) {
> +    if (!cx->runtime()->enqueuePromiseJobCallback) {
> +        JS_ReportErrorNumber(cx, GetErrorMessage, nullptr,

If we can, I'd rather assert here, than throw. Anybody who doesn't set necessary hooks is improperly embedding, in my world view. It's not like we're gonna succeed to call through nullptr anyway.

::: js/src/vm/SelfHosting.cpp
@@ +1694,5 @@
> +    CallArgs args = CallArgsFromVp(argc, vp);
> +    MOZ_ASSERT(args.length() == 1);
> +
> +    RootedObject obj(cx, &args[0].toObject());
> +    MOZ_ASSERT(!obj->is<PromiseObject>());

this assert seems amazingly harsh for the function's name. I would expect IsWrapper(obj) && IsPromiseObject(obj).

What does this protect against?
Comment on attachment 8727879 [details] [diff] [review]
Part 11: Properly handle rejecting wrapped promises in the face of xray wrappers

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

::: js/src/vm/SelfHosting.cpp
@@ +1705,5 @@
> +    // values might be wrapped into a wrapper that throws whenever the
> +    // Promise's reaction handler wants to do anything useful with it. To
> +    // avoid that situation, we synthesize a generic error that doesn't
> +    // expose any privileged information but can safely be used in the
> +    // rejection handler.

OOPS! I should have caught this in the last review. Nice.

@@ +1721,5 @@
> +        return false;
> +    RootedPropertyName name(cx, atom->asPropertyName());
> +
> +    RootedValue selfHostedFun(cx);
> +    if (!GlobalObject::getIntrinsicValue(cx, cx->global(), name, &selfHostedFun))

CallSelfHostedFunction()
Attachment #8727879 - Flags: review?(efaustbmo) → review+
Comment on attachment 8727887 [details] [diff] [review]
Part 15: Add support for tracking unhandled promise rejections, exposed through a JSAPI function

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

Oh, BOY, devtools! This all looks expensive, but necessary :/
Attachment #8727887 - Flags: review?(efaustbmo) → review+
Comment on attachment 8728475 [details] [diff] [review]
Part 10: Properly wrap and unwrap |then| callbacks for xrayed Promises

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

Wheeeeeeeeeeee.

::: js/src/builtin/Promise.js
@@ +750,5 @@
> + * higher-privileged compartment, and after unwrapping we end up in the
> + * lower-privileged one), that's not necessarily the case. One or both of the
> + * handlers can originate from the lower-privileged compartment, so they'd
> + * actually be unwrapped functions when they reach
> + * UnwrappedPerformPromiseThen.

Oh my god, the complexity here is astonishing.
Attachment #8728475 - Flags: review?(efaustbmo) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/5e11576f3d8bd27362ddaaceab9891ff69f7a98f
Bug 911216 - Part 3: Allow wrapped self-hosted functions and intrinsics in the callFunction debug check. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/7521187099901a39edfea213b477975bbb98ba99
Bug 911216 - Part 4: Add self-hosting intrinsic for creating arrays in other compartments. r=efaust
Depends on: 1257588
Created attachment 8732844 [details] [diff] [review]
Part NN: Do a checked unwrap in all Promise-taking JSAPI functions

As efaust suggested in the review of the core Promise patch, this changes all Promise-taking JSAPI functions to do a CheckedUnwrap on them.

bz, I'm asking for feedback in case I'm overlooking something that'd make this a bad idea.
Attachment #8732844 - Flags: review?(efaustbmo)
Attachment #8732844 - Flags: feedback?(bzbarsky)
Created attachment 8732847 [details] [diff] [review]
Part NN: Set Promise allocation stack as async stack for Promise callback jobs

This uses the new Callback overload added in bug 1257030 to supply the Promise allocation stack as the async stack parent when running Promise jobs. Fixes (in combination with a few tweaks of tests here and there) all the regressions of async stack tests I encountered previously.
Attachment #8732847 - Flags: review?(efaustbmo)
Attachment #8732847 - Flags: review?(bzbarsky)
Created attachment 8732849 [details] [diff] [review]
Part NN - Make SM Promise's allocation and resolution stacks available on PromiseDebugging

Restores the ability to dump a rejection stack for test failures in PromiseTestUtils.jsm. It seems somewhat likely that it'd be possible to somehow use a Debugger object here instead, but I don't know how, and don't feel too inclined to spend even more time on figuring it out, when this works just fine.
Attachment #8732849 - Flags: review?(bzbarsky)
Created attachment 8732855 [details] [diff] [review]
Part NN: Use promise rejection tracking to report unhandled rejections to the console

This should address the feedback in comment 175 and comment 176. Using the infrastructure added in bug 1257030 I restored (see comment 196) the async stack we have in the DOM Promise implementation, and in comment 197 I restored the rejection stack dumping.
Attachment #8732855 - Flags: review?(paolo.mozmail)
Attachment #8732855 - Flags: review?(bzbarsky)
Attachment #8727890 - Attachment is obsolete: true
Attachment #8727890 - Flags: review?(bzbarsky)
Created attachment 8732860 [details] [diff] [review]
Part NN: Port Promise reaction jobs to C++ to enable correct async stacks

I had to convert these to C++, sadly. Without that, I wasn't able to make the stack inspection surrender and give me the correct full stack for Promise callbacks, including async execution reasons and all. The issue is that self-hosted frames are filtered out, which we normally want, but with the callback being implemented in self-hosted JS, it's the point of entry for the inspection, and filtering it out is Not Good.
Attachment #8732860 - Flags: review?(efaustbmo)

Comment 200

2 years ago
Comment on attachment 8732855 [details] [diff] [review]
Part NN: Use promise rejection tracking to report unhandled rejections to the console

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

Thanks! I'd appreciate if you could add "self.assertInLog" lines for the stack traces for the Promise rejection test cases in "testing/xpcshell/selftest.py", similar for example to the other stack tests for Task. The tests can be run with this command line:

./mach python-test testing/xpcshell/selftest.py

For the PromiseTestUtils.jsm changes, the new approach relies on onLeftUncaught observers for the same Promise to be called on the same tick. If this is the case, this should be documented in onLeftUncaught.

If this isn't true now or it changes in the future, the new approach could race in the following case:
- ensureDOMPromiseRejectionsProcessed is called, creates Promise 1
- ensureDOMPromiseRejectionsProcessed>onLeftUncaught is called for Promise 1
- ensureDOMPromiseRejectionsProcessed returns
- ensureDOMPromiseRejectionsProcessed is called again, creates Promise 2
- this.onLeftUncaught is called for Promise 1 - FAIL
- ensureDOMPromiseRejectionsProcessed>onLeftUncaught is called for Promise 2
- this.onLeftUncaught is called for Promise 2

::: toolkit/modules/tests/PromiseTestUtils.jsm
@@ +101,5 @@
>        onConsumed() {},
>      };
>  
>      PromiseDebugging.addUncaughtRejectionObserver(observer);
> +    this._ensureDOMPromiseRejectionsProcessedPromise = Promise.reject({});

nit: Promise.reject() should work as well.
Attachment #8732855 - Flags: review?(paolo.mozmail)

Comment 201

2 years ago
(In reply to :Paolo Amadini from comment #200)
> If this is the case, this should be documented in onLeftUncaught.

To clarify, I mean in the definition of the observers in the "PromiseDebugging.webidl".
Created attachment 8733394 [details] [diff] [review]
Part NN: Add Promise checks to test_xrayToJS.xul

I realized that a) this needs an xpconnect peer's review, and b) it can't land with the implementation of Promise itself, because I'm going to land that switched off at first, so the tests would fail.

I'll create a single patch for all the test changes that I can't use configure flags to enable/disable and land those together with the change that actually flips the switch to enable JS Promises.
Attachment #8733394 - Flags: review?(bobbyholley)
https://hg.mozilla.org/integration/mozilla-inbound/rev/672da685c6fc330d9b8d68e3f1db6cb951f34f8d
Bug 911216 - Part 5: Add --enable-sm-promise configure flag. r=chmanchester

https://hg.mozilla.org/integration/mozilla-inbound/rev/17385ac2980201df48efb904afa7da8af547b251
Bug 911216 - Part 6: Shim new promise-related Debugger.Object accessors using PromiseDebugging. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/021f70a04fadc6155030df3d30d8c4f01278dd6a
Bug 911216 - Part 7: Implement ES6 Promises in the JavaScript engine. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf7722889ed96e7deaaaa9eef4b8b0caf8421d7d
Bug 911216 - Part 8: Properly wrap and unwrap |then| callbacks for xrayed Promises. r=efaust,f=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/33ad12d6ff452fa3f5f12f623a8837024706ab52
Bug 911216 - Part 9: Properly handle rejecting wrapped promises in the face of xray wrappers. r=efaust,f=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/87c4e3921c4c419001c3ae554ab4249d3ee13c0a
Bug 911216 - Part 10: Support debugger hooks for creation and settling of promises. r=shu

https://hg.mozilla.org/integration/mozilla-inbound/rev/e947c9941fe17266770e9f56f283f0d7628b2b65
Bug 911216 - Part 11: Implement all Promise inspection functionality as Debugger getters. r=shu,fitzgen
(No, the above push doesn't mean this is done. It does contain the meat of the Promise implementation, but doesn't enable it, because other things need to be fixed before that can happen.)
Comment on attachment 8732844 [details] [diff] [review]
Part NN: Do a checked unwrap in all Promise-taking JSAPI functions

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

::: js/src/jsapi.cpp
@@ +4689,5 @@
>  
>  JS_PUBLIC_API(double)
> +JS::GetPromiseID(JS::HandleObject obj)
> +{
> +    JSObject* promise = CheckedUnwrap(obj);

if these really want to fault when they're not allowed, can we MOZ_ASSERT(promise) everywhere?
Attachment #8732844 - Flags: review?(efaustbmo) → review+
Comment on attachment 8732847 [details] [diff] [review]
Part NN: Set Promise allocation stack as async stack for Promise callback jobs

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

r=me on the selfhosted and js bits. I can't speak for the CycleCollected* bits.
Attachment #8732847 - Flags: review?(efaustbmo) → review+
Created attachment 8733931 [details] [diff] [review]
Part NN: Use promise rejection tracking to report unhandled rejections to the console

I ended up reverting all the changes to PromiseTestUtils.jsm: after re-adding most functionality to PromiseDebugging it wasn't required to change this. Other feedback from comment 200 addressed.
Attachment #8733931 - Flags: review?(paolo.mozmail)
Attachment #8733931 - Flags: review?(bzbarsky)
Attachment #8732855 - Attachment is obsolete: true
Attachment #8732855 - Flags: review?(bzbarsky)

Comment 217

2 years ago
Comment on attachment 8733931 [details] [diff] [review]
Part NN: Use promise rejection tracking to report unhandled rejections to the console

Thanks! I guess the string "A Promise rejection wasn't immediately handled" is specific to the new implementation, but it's fine if this lands all together and the string appears for both the JS/DOM and the JSM test cases.
Attachment #8733931 - Flags: review?(paolo.mozmail) → review+
Comment on attachment 8732860 [details] [diff] [review]
Part NN: Port Promise reaction jobs to C++ to enable correct async stacks

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

wfm

::: js/src/builtin/Promise.cpp
@@ +457,5 @@
> +        }
> +    } else {
> +        // Step 6.
> +        InvokeArgs args2(cx);
> +        if (!args2.init(1))

It hasn't landed just yet (bug 1257077), but if it has before you land this, please convert this and the others to use FixedInvokeArgs.

@@ +514,5 @@
> +    args2.setThis(thenable);
> +    args2.setCallee(then);
> +    args2[0].set(resolveVal);
> +    args2[1].set(rejectVal);
> +    if (Invoke(cx, args2))

Please add a silly comment here.

// Return immediately if we succeed.

The |if (!...) return| idiom is too strong to reverse on people ;)

::: js/src/vm/SelfHosting.cpp
@@ +1549,5 @@
>  {
>      CallArgs args = CallArgsFromVp(argc, vp);
>      MOZ_ASSERT(args.length() == 2);
> +    MOZ_ASSERT(args[0].toObject().as<NativeObject>().getDenseInitializedLength() == 4);
> +    // When using JS::AddPromiseReactions, no actual promise is created, so we

nit: blank line before comment

@@ +1602,5 @@
> +    if (!job)
> +        return false;
> +
> +    job->setExtendedSlot(0, args[0]);
> +    RootedObject promise(cx, CheckedUnwrap(&args[1].toObject()));

Cute that we don't have to check this. We'll just not get allocation data.
Attachment #8732860 - Flags: review?(efaustbmo) → review+
Comment on attachment 8732847 [details] [diff] [review]
Part NN: Set Promise allocation stack as async stack for Promise callback jobs

>+    _EnqueuePromiseJob(reaction.capabilities.promise, function PromiseReactionJob() {

So for the case of a then() call, will this be the allocation stack for the return value of "then", or for the `this` value of the "then" call?  Which one is it right now for DOM Promise?

>+    _EnqueuePromiseJob(promiseToResolve, function PromiseResolveThenableJob() {

Does the DOM promise code do any sort of async stack setup in this case?  I don't think it does....  Though maybe it sets up something via the PromiseInit stored in the PromiseResolveThenableJob, but then that would be the stack to the place where we got resolved with the thenable, right?

>+                                JS::HandleObject allocationSite, void* data);

Should document that allocationSite is a SavedFrame thing or something, right?  And that it's allowed to be null?

>+    MOZ_ASSERT(args[0].toObject().is<PromiseObject>());

I hope that's been checked in various weird cross-compartment cases?

>+  nsCOMPtr<nsIRunnable> runnable = new PromiseJobRunnable(aJob, aAllocationSite);

If aAllocationSite is null (can that happen?) should we grab the stack from the JSContext instead?

r=me modulo those issues.
Attachment #8732847 - Flags: review?(bzbarsky) → review+
Comment on attachment 8732849 [details] [diff] [review]
Part NN - Make SM Promise's allocation and resolution stacks available on PromiseDebugging

>+PromiseDebugging::GetAllocationStack(GlobalObject&, JS::Handle<JSObject*> aPromise,

Why did this code get moved?  Just leave it be, please.  Same for the other two moved methods.

r=me
Attachment #8732849 - Flags: review?(bzbarsky) → review+
Comment on attachment 8733931 [details] [diff] [review]
Part NN: Use promise rejection tracking to report unhandled rejections to the console

>+  JSErrorReport* report = new JSErrorReport();

Is there a reason we're initing this with the file/line/column from the rejection stack, not from the actual exception?  I think the current setup uses the exception here.

Speaking of which, we should file a followup to actually propagate the full stack, not just the file/line/column to the console; that's a preexisting issue, though.

Also, is it me or is this JSErrorReport leaked?  What frees it?

>+  xpcReport->Init(report, "A Promise rejection wasn't immediately handled", isChrome,

And here we lose the message from the exception we were rejected with.  Maybe that's ok....

>+  // Since Promises preserve their wrapper, it is essential to RefPtr<> the

Is this still true?  It used to be true when we did it from cc unlink, and hence could do it quite late in shutdown.  But I'm not sure it's true in the new setup.

In any case, this would have nothing to do with wrapper preservation, since that only mattered in terms of when unlink ended up happening.  Please adjust the comments accordingly, and file a followup to nix this complexity.

>+  static void ReportRejectedPromise(JSContext* cx, JS::HandleObject promise);

Please document what the args are (e.g. is the second arg an actual Promise, or could it be a cross-compartment wrapper?).  Especially since you then start entering compartments in the impl, for reasons that are not obvious.

>+    AutoJSAPI jsapi;
>+    jsapi.Init();
>+    JS_ClearPendingException(jsapi.cx());

No, this doesn't make sense.  Why are we grabbing some random JSContext and then clearing the pending exception on it?  That has nothing to do with us!

Of course AutoJSAPI::Init will assert if there is a pending exception on the JSContext it ends up using, so this code would fail asserts if it were doing something other than a no-op anyway....

The same applies to the other places where you use this pattern.

>+PromiseDebugging::AddConsumedRejection(JS::HandleObject aPromise)

Ick.  Having a O(N) algorithm here is not great.  Can we not just have a hashset or something?

>+  // The Promise that have been left uncaught (rejected and last in

s/Promise/Promises/

>+  // The Promise that have been left uncaught (rejected and last in
>+  // their chain) since the last call to this function in |uncaught|.

I don't understand what this comment is trying to say.  Is it trying to say that "|uncaught| is the list of Promies that have been ...."?  Or that these promises "are in |uncaught|" or something?

>+  // The Promise that have been left uncaught at some point, but that
>+  // have eventually had their `then` method called in |consumed|.

Similar issue here.

>+      obs->OnLeftUncaught(promise, err); // Ignore errors

Then you should make err be a IgnoredErrorResult.  Otherwise errors, when they happen, will assert that you didn't ignore them.

+      obs->OnConsumed(promise, err); // Ignore errors

Same here.

>+  JS::PersistentRooted<js::GCVector<JSObject*, 0, js::SystemAllocPolicy>> mUncaughtRejections;
>+  JS::PersistentRooted<js::GCVector<JSObject*, 0, js::SystemAllocPolicy>> mConsumedRejections;

These need documentation.

>   nsTArray<nsCOMPtr<nsISupports /* UncaughtRejectionObserver */ >> mUncaughtRejectionObservers;

Er... why does it not just store nsRefPtr<UncaughtRejectionObserver>?  :(  Ah, well.  Followup, please.

r- because I don't understand the JSContext shenanigans here.
Attachment #8733931 - Flags: review?(bzbarsky) → review-
Comment on attachment 8733394 [details] [diff] [review]
Part NN: Add Promise checks to test_xrayToJS.xul

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

::: js/xpconnect/tests/chrome/test_xrayToJS.xul
@@ +51,5 @@
>    function go() {
>      window.iwin = document.getElementById('ifr').contentWindow;
>  
> +    // Test constructors that can be instantiated with zero arguments, or with
> +    // a fixed set of arguments provided using `...rest`.

Please describe this new format in a comment above simpleConstructors. Also, I'd prefer it to be object-y to be more understandable, i.e.:

{ name: string, args: array }

instead of arrays of arrays.

@@ +776,5 @@
>        }
>      }
>    }
>  
> +  function testPromise() {

Can you add a comment that the behavior here is tested in much more detail in test_promise_xrays.html?
Attachment #8733394 - Flags: review?(bobbyholley) → review+
Created attachment 8734835 [details] [diff] [review]
Part NN: Store incumbent global when creating Promise reactions for Promise#then and set it on the PromiseJobCallback

This fixes the test bz added in bug 1258391. It's annoying to have to do yet another callback, and to store yet another piece of information on the promise reactions, but here we are.
Attachment #8734835 - Flags: review?(efaustbmo)
Attachment #8734835 - Flags: review?(bzbarsky)
Comment on attachment 8734835 [details] [diff] [review]
Part NN: Store incumbent global when creating Promise reactions for Promise#then and set it on the PromiseJobCallback

>+    // The global might be wrapped, in which case we need to unwrap it first so

Or we could have the Gecko side unwrap.

Either way, this interaction should be documented in the jsapi.h bits for the promise callback, right?  Passing in objects that are not same-compartment with cx is NOT normal.

>+    // The global might be different from the current one, so wrap it.

Again, the fact that the incumbent global callback is allowed to return things not in the compartment of cx is weird and should be documented.

Why do we pass a cx here at all?  Just so shell can pretend to do something?  Does it not have a better way of doing it?  :(

>+    global = xpc::NativeGlobal(aIncumbentGlobal);

We should probably have an assert here about aIncumbentGlobal not being a wrapper or something...

I didn't really have a good way to verify whether you're calling the new intrinsic in the right places, but I'll assume you are.

I do think there is one issue here which is a bit worrisome: If the incumbent global ends up being a non-current window, then aren't we going to end up with the wrong nsIGlobalObject once we've round-tripped through the whole thing?  This is especially worrisome given the use of UncheckedUnwrap...  I'm pretty sure this part is not OK.  :(
Attachment #8734835 - Flags: review?(bzbarsky) → review-
Comment on attachment 8734835 [details] [diff] [review]
Part NN: Store incumbent global when creating Promise reactions for Promise#then and set it on the PromiseJobCallback

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

Given that bz already has concerns about this, I'm going to clear review, for now. Most of the JS bits look unobjectionable, though I did not do the due dilligence to fully understand all of the wrapping and unwrapping going on.
Attachment #8734835 - Flags: review?(efaustbmo)
So I've been trying to think of a way to deal with the problem I described (and which I should have thought of when I suggested handing back the global... I forgot it was going into JS code and would get messed with as a result).

We _could_ do something like return an object that has the nsIGlobalObject as a private and has the usual private-is-nsisupports thing.  That means an extra object allocation for every job, but maybe that's not an issue; this code is already all super-allocation-happy as far as I can tell...

I really wish we just had a way to hand out an inner window and have people not outerize it.  :(  Bobby, this really does keep coming up!  Do you have any brighter ideas for having a way for selfhosted JS to capture the incumbent global?
Flags: needinfo?(bobbyholley)
Created attachment 8735292 [details] [diff] [review]
Part NN: Use promise rejection tracking to report unhandled rejections to the console

(In reply to Boris Zbarsky [:bz] from comment #222)
> >+  JSErrorReport* report = new JSErrorReport();
> 
> Is there a reason we're initing this with the file/line/column from the
> rejection stack, not from the actual exception?  I think the current setup
> uses the exception here.

It did this in an effort to improve the error reporting. however, that was back when I was optimistic about this part, and wanted to do crazy things like show stack traces. I have now reverted all this.

> Speaking of which, we should file a followup to actually propagate the full
> stack, not just the file/line/column to the console; that's a preexisting
> issue, though.

Including this.

> Also, is it me or is this JSErrorReport leaked?  What frees it?

I improved the cargo-culting of the code to the point where it's largely identical to Promise::MaybeReportRejected. AFAICT, that should take care of this.

> >+  xpcReport->Init(report, "A Promise rejection wasn't immediately handled", isChrome,
> 
> And here we lose the message from the exception we were rejected with. 
> Maybe that's ok....

See above. Ideally we'd show both a message about this being a promise rejection and the original message. But that's not really related to changing the Promise implementation, so, reverted.

> 
> >+  // Since Promises preserve their wrapper, it is essential to RefPtr<> the
> 
> Is this still true?  It used to be true when we did it from cc unlink, and
> hence could do it quite late in shutdown.  But I'm not sure it's true in the
> new setup.
> 
> In any case, this would have nothing to do with wrapper preservation, since
> that only mattered in terms of when unlink ended up happening.  Please
> adjust the comments accordingly, and file a followup to nix this complexity.

I changed this to use DispatchToMainThread after all. AFAIUI, the concerns preventing that shouldn't exist anymore.

> 
> >+  static void ReportRejectedPromise(JSContext* cx, JS::HandleObject promise);
> 
> Please document what the args are (e.g. is the second arg an actual Promise,
> or could it be a cross-compartment wrapper?).  Especially since you then
> start entering compartments in the impl, for reasons that are not obvious.

I hope the documentation is enough as-is. I'm not sure what else you'd be looking for.

> 
> >+    AutoJSAPI jsapi;
> >+    jsapi.Init();
> >+    JS_ClearPendingException(jsapi.cx());
> 
> No, this doesn't make sense.  Why are we grabbing some random JSContext and
> then clearing the pending exception on it?  That has nothing to do with us!
> 
> Of course AutoJSAPI::Init will assert if there is a pending exception on the
> JSContext it ends up using, so this code would fail asserts if it were doing
> something other than a no-op anyway....

Bleh, sorry. I changed this to take a cx, which I have at the callsite anyway.

> 
> The same applies to the other places where you use this pattern.
> 
> >+PromiseDebugging::AddConsumedRejection(JS::HandleObject aPromise)
> 
> Ick.  Having a O(N) algorithm here is not great.  Can we not just have a
> hashset or something?

Do we have a collection I could easily use here that'd keep the order correct? That seems important. I left it as-is for now on the theory that we won't ever have a ridiculous amount of entries in this list because it's flushed after each event loop turn.

> 
> >+  // The Promise that have been left uncaught (rejected and last in
> 
> s/Promise/Promises/
> 
> >+  // The Promise that have been left uncaught (rejected and last in
> >+  // their chain) since the last call to this function in |uncaught|.
> 
> I don't understand what this comment is trying to say.  Is it trying to say
> that "|uncaught| is the list of Promies that have been ...."?  Or that these
> promises "are in |uncaught|" or something?

This was largely copied, but I changed it substantially now. Also moved to where the lists are defined.

> >+  JS::PersistentRooted<js::GCVector<JSObject*, 0, js::SystemAllocPolicy>> mUncaughtRejections;
> >+  JS::PersistentRooted<js::GCVector<JSObject*, 0, js::SystemAllocPolicy>> mConsumedRejections;

... here.

> 
> These need documentation.
> 
> >   nsTArray<nsCOMPtr<nsISupports /* UncaughtRejectionObserver */ >> mUncaughtRejectionObservers;
> 
> Er... why does it not just store nsRefPtr<UncaughtRejectionObserver>?  :( 
> Ah, well.  Followup, please.

*Sigh*, I don't even have the vocabulary to file a bug for this. Like, why would your suggestion work, what needs to be done, which are the words to describe these things with?
Attachment #8735292 - Flags: review?(bzbarsky)
Attachment #8733931 - Attachment is obsolete: true
Comment on attachment 8732844 [details] [diff] [review]
Part NN: Do a checked unwrap in all Promise-taking JSAPI functions

>+JS::GetPromiseResult(JS::HandleObject obj)

If we're going to allow cross-compartment wrappers here, we should document that the returned value may not be same-compartment with the passed-in value, right?

>+JS::GetPromiseAllocationSite(JS::HandleObject obj)

Same here.

>+JS::GetPromiseResolutionSite(JS::HandleObject obj)

And here.

>+JS::ResolvePromise(JSContext* cx, JS::HandleObject obj, JS::HandleValue resolutionValue)

For this one, I don't understand how we wouldn't just get a compartment mismatch assert in the case when CheckedUnwrap(obj) != obj.  We'd end up doing an Invoke() with cx and args[0] in one compartment, but the callee in a different compartment, no?

Either this function needs to take only actual promises or we need to enter the compartment of "promise" and wrap "resolutionValue" into it, or something.

>+JS::RejectPromise(JSContext* cx, JS::HandleObject obj, JS::HandleValue rejectionValue)

Same issues here.

>+JS::CallOriginalPromiseThen(JSContext* cx, JS::HandleObject obj,

And here, with "this" being not-same-compartment with cx/args as oppposed to the callee...

And the return value here is in which compartment?  Needs documenting.

>+JS::AddPromiseReactions(JSContext* cx, JS::HandleObject obj,

Same issues here.
Attachment #8732844 - Flags: feedback?(bzbarsky) → feedback-
Comment on attachment 8735292 [details] [diff] [review]
Part NN: Use promise rejection tracking to report unhandled rejections to the console

>+  JS::Rooted<JS::Value> result(aCx, JS::GetPromiseResult(aPromise));

I hope there are jsapi docs saying this can return something not same-compartment with aPromise?

>@@ -2461,28 +2501,17 @@ Promise::MaybeReportRejected()

Please don't change this function.  Especially if the change doesn't compile.  ;)  But more to the point, in that function, and given when it's called, the comments and code are correct.

>+  static void ReportRejectedPromise(JSContext* cx, JS::HandleObject promise);

This still needs documentation explaining that cx is allowed to be in some random compartment not matching that of aPromise.

Or better would be to NOT allow that and do the compartment-entering in the one caller.  I would much prefer that, in fact.

In any case, API documentation should go in the header, not the implementation file.

>+PromiseDebugging::AddUncaughtRejection(JSContext* aCx, JS::HandleObject aPromise)

Why do we need to clear exceptions on aCx here?  We're not using it for anything; how would it get an exception?

>+PromiseDebugging::AddConsumedRejection(JSContext* aCx, JS::HandleObject aPromise)

Same thing here.

>+  void onLeftUncaught(object p);

Please have the impl throw if the object is not a Promise.  Same for onConsumed.  Otherwise bad things will happen with later code trying to assume it _is_ a Promise and read its slots!

> Do we have a collection I could easily use here that'd keep the order
> correct? 

Hrmph.  We could make it a collection of pairs of (counter, promise) or something and sort by counter...  Then we could even sort at the one point where we care about order, that being before we report them.  

I can sort of live with this being a followup, but I do think we should do this, because I'm pretty sure your theory is wrong.  If a single script execution just rejects a few tens of thousands of promises before going to the event loop and then does then() calls on them all, things will be painful.  Even if it does the calls in the same order, since we'll memmove so much, but even worse if the order is random.

> I don't even have the vocabulary to file a bug for this.

Looks like we do in fact have a comment explaining why things are as they are; I just missed it in my first review.  It's sad, but I guess I can live with it...

r=me with these comments addressed.
Attachment #8735292 - Flags: review?(bzbarsky) → review+
(In reply to Boris Zbarsky [:bz] from comment #228)
> So I've been trying to think of a way to deal with the problem I described
> (and which I should have thought of when I suggested handing back the
> global... I forgot it was going into JS code and would get messed with as a
> result).
> 
> We _could_ do something like return an object that has the nsIGlobalObject
> as a private and has the usual private-is-nsisupports thing.  That means an
> extra object allocation for every job, but maybe that's not an issue; this
> code is already all super-allocation-happy as far as I can tell...
> 
> I really wish we just had a way to hand out an inner window and have people
> not outerize it.  :(  Bobby, this really does keep coming up!  Do you have
> any brighter ideas for having a way for selfhosted JS to capture the
> incumbent global?

In general, you can capture a reference to a global indirectly by referencing something in its scope. If you generally trusted the page, you could just hold onto a "new Object()", and then later check the prototype chain. Self-hosted SM code has all these weird constraints in general, and needs to use intrinsics, so we could presumably provide some intrinsic for that, or even use the intrinsic scope object itself?

Anyway, this question isn't really specific enough for me to answer...
Flags: needinfo?(bobbyholley)
(In reply to Bobby Holley (busy) from comment #239)
> In general, you can capture a reference to a global indirectly by
> referencing something in its scope. If you generally trusted the page, you
> could just hold onto a "new Object()", and then later check the prototype
> chain.

I don't actually need to do this in self-hosted code, so I ended up creating a NativeObject, wrapping and storing it, and then getting the global from the unwrapped object later on.

> Self-hosted SM code has all these weird constraints in general, and
> needs to use intrinsics, so we could presumably provide some intrinsic for
> that, or even use the intrinsic scope object itself?

Well, the constraints are that it should get as few additional capabilities compared to content script as possible, and it should be as clear as possible that you're doing something special and need to be extra careful if you you them :)
Created attachment 8736681 [details] [diff] [review]
Part NN: Make promise-related Debugger.Object.prototype getters work with wrapped promises

This adds CheckedUnwrap calls to all promise-related Debugger.Object getters.

One of the two reasons is purely pragmatic: the devtools rely on this quite deeply, so there are lots of annoying subtle issues caused by it. That's because the getters on the old PromiseDebugging DOM interface did automatic unwrapping, too. I fear that weeding out all these issues would turn into another chase down a rabbit hole, of which I had a few already for Promises.

The other reason is that a D.O for a wrapped Promise still has the `class` value "Promise". It seems quite weird to me to not be able to treat something as a D.O. for a Promise if we're saying that it is one.
Attachment #8736681 - Flags: review?(shu)

Comment 242

2 years ago
(In reply to Till Schneidereit [:till] from comment #241)
> Created attachment 8736681 [details] [diff] [review]
> Part NN: Make promise-related Debugger.Object.prototype getters work with
> wrapped promises
> 
> This adds CheckedUnwrap calls to all promise-related Debugger.Object getters.
> 
> One of the two reasons is purely pragmatic: the devtools rely on this quite
> deeply, so there are lots of annoying subtle issues caused by it. That's
> because the getters on the old PromiseDebugging DOM interface did automatic
> unwrapping, too. I fear that weeding out all these issues would turn into
> another chase down a rabbit hole, of which I had a few already for Promises.
> 

I find this compelling enough. Ain't nobody wanna rewrite devtools to land SM Promises.

Comment 243

2 years ago
Comment on attachment 8736681 [details] [diff] [review]
Part NN: Make promise-related Debugger.Object.prototype getters work with wrapped promises

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

Looks fine. I'd rename the test simple "wrapped-promise". The Debugger- prefix means the test is for methods or properties on Debugger instances.
Attachment #8736681 - Flags: review?(shu) → review+
Depends on: 1261784
Created attachment 8737796 [details] [diff] [review]
Part 18: Only capture Promise allocation and resolution stacks if async stacks are enabled

For all other async stacks, we already check if async stacks are enabled in the settings. For Promise, we unconditionally take them, with the old and the new implementation. Let's change that to bring Promise in line.
Attachment #8737796 - Flags: review?(efaustbmo)
Attachment #8737796 - Flags: feedback?(nfitzgerald)
Comment on attachment 8737796 [details] [diff] [review]
Part 18: Only capture Promise allocation and resolution stacks if async stacks are enabled

I think a better check would be if async stacks are enabled OR the compartment is a debuggee. We certainly want this information when debugging promises.
Attachment #8737796 - Flags: feedback?(nfitzgerald) → feedback-
Created attachment 8737809 [details] [diff] [review]
Part 18: Only capture Promise allocation and resolution stacks if async stacks are enabled or the Promise's compartment is debugged

Now with more stack-capturing during debugging. (And an f=fitzgen via IRC)
Attachment #8737809 - Flags: review?(efaustbmo)
Attachment #8737796 - Attachment is obsolete: true
Attachment #8737796 - Flags: review?(efaustbmo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/58716e5626909a33ba00a3355df79d6ffad60916
Bug 911216 - Part 13: Set Promise allocation stack as async stack for Promise callback jobs. r=efaust,bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd5acdf4a2a1d587a40658303857ca61c0e87abb
Bug 911216 - Part 14: Add support for tracking unhandled promise rejections, exposed through a JSAPI function. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e98f8b36bc68b505c473ef3bcdc0825dc29708b
Bug 911216 - Part 15: Port Promise reaction jobs to C++ to enable correct async stacks. r=efaust

https://hg.mozilla.org/integration/mozilla-inbound/rev/2f503e373e6f74b9c22b402e6bcac997e06ee80e
Bug 911216 - Part 16: Use new Promise inspection Debugger getters to implement legacy functions on PromiseDebugging. r=bz

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7023522452523bef218db9a9b565dafd24f1e9f
Bug 911216 - Part 17: Make promise-related Debugger.Object.prototype getters work with wrapped promises. r=shu

Updated

2 years ago
Attachment #8737809 - Flags: review?(efaustbmo) → review+
Till, why do we need the (rather expensive) PRMJ_Now thing on every Promise construction?
Created attachment 8751785 [details]
fix some bitrot in DebuggerObject_getPromiseDependentPromises()
Created attachment 8751786 [details]
Fix nullptr deref when creating promises from c++

I think you have this fix in one of your unlanded patches.

Updated

2 years ago
Blocks: 1272697

Updated

2 years ago
Blocks: 1272748
Depends on: 1272887