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.

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.

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.

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.

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.

Set internal properties in slots.

(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.

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?

(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.

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.

Yep, that sounds right.

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?

https://github.com/domenic/promises-unwrapping talks about microtasks. (not clear to me why we'd want microtasks in this case)

(In reply to Bobby Holley (:bholley) from comment #20) > Do DOM Promises do the same thing? Yes. http://mxr.mozilla.org/mozilla-central/search?find=%2Fdom%2Fpromise%2F&string=NS_DispatchTo

Ok, then it sounds like this is not ES-compatible.

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.

Hey Nikhil, what are your plans about reconciling this implementation with the one done by Andrea in dom/promise?

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?

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

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.

> 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/

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.

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.

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.

(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.

(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.

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.

Some implementation discussion in the logs here: http://logs.glob.uno/?c=content#c289609

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.

Now without the self-hosted ctor, so doesn't depend on bug 1226261.

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.

Now with less assert triggering

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.

(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?

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>

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.

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.

This should be pretty much done, but needs more tests. Asking for feedback as a sanity check on the implementation.

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.

(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

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.

> 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.

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.

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.

(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.

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.

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

Now with JS_GetPromiseConstructor and JS_GetPromisePrototype JSAPI functions. Sorry for the bugspam.

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

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.

(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

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.

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

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.

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.

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.

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...

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.

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!

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?

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.

Fixes various issues uncovered by bz's try push at https://treeherder.mozilla.org/#/jobs?repo=try&revision=5c0605407883.

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))...

Fixes a bunch of issues bz and/or tests uncovered, and adds Xray support.

(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.

This actually contains the Xray changes - I accidentally added those to a later patch before.

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.

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.

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.

Enters the compartment that the passed-in wrapped object originates from, creates an array, wraps it for the calling compartment, returns it. Simple.

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.

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!

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.

(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. ;)

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.

> 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...

(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.

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

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.

(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.

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?

(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.

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

(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

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.)

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.

I think this addresses all feedback. It certainly fixes the failing asserts.

Needed to implement the correct semantics in Promise.all.

The meat of things. Well, much of it, but there's an additional dozen or so patches to come ...