879245 - Implement thenables for promises

Status: RESOLVED FIXED

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

Comment 1

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 2

[Mounir Lamouri (:mounir)]

Andrea, makes sure this gets an sr.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 761429 [details] [diff] [review]
patch

This looks like the wrong diff.

Comment 4

[Andrea Marchesini [:baku]]

Attachment: patch

Too many patches with the same name...

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 761684 [details] [diff] [review]
patch

So a preface.... I'm sorry you're having to deal with the minefield that is JSAPI.  :(

>+    if (JS_GetProperty(aCx, valueObj, "then", then.address()) &&

If that returns false, an exception was thrown.  You're supposed to "catch that exception and run Reject", etc.  Certainly do not leave it on aCx!

>+      JSObject* fulfillFunc = CreateFunction(aCx, valueObj, fulfillCb);

This can return null, in which case an out-of-memory error is being reported, etc.  You need to handle that.

>+      JSObject* rejectFunc = CreateFunction(aCx, valueObj, rejectCb);

Again, can return null.

Also, this is a rooting hazard.  You need to store fulfillFunc in something rooted first.  I recommend doing this, before you start creating any objects:

  AutoValueVector argv(cx);
  if (!argv.reserve(2)) {
    // report the OOM
  }

Then create your object and do:

  elements.infallibleAppend(JS::ObjectValue(*fulfillFunc));

and likewise for rejectFunc.

>+      JS_CallFunctionValue(aCx, valueObj, then, 2 /* nargs */, values,

And here you'd pass argv.length() for nargs and argv.begin() for the argv.

>+      JS::Value exn;
>+      if (JS_IsExceptionPending(aCx) && JS_GetPendingException(aCx, &exn)) {

The contract is that passing a Value* somewhere means the thing it's pointing to is rooted.  So you probably want to create your Optional<Handle<Value>> earlier and pass the pointer to its internal Rooted.

And given the number of places you need to report an exception here, you may want to factor this bit out.

>+PromiseResolver::CreateFunction(JSContext* aCx, JS::Handle<JSObject*> aObj,
>+  JSFunction* func = js::NewFunctionWithReserved(aCx, JSCallback,

That can return null...

>+                                                 aObj, nullptr);

I wonder whether we should pass in a nicer name.  Either way, though.

>+  js::SetFunctionNativeReserved(obj, 0, PRIVATE_TO_JSVAL(aCallback));

  JS::PrivateValue(aCallback);

But more importantly, what keeps aCallback alive here?  I don't see anything doing it.  And worse yet, if we give the function a reference to it, what releases the reference?

What we should probably do is instead give the JSFunction a JSObject to keep alive in this reserved slot (for example, our resolver's GetWrapper(), wrapped into the context compartment).  And I guess some sort of integer value for an enum representing whether to fulfill() or reject().  And then when it's actually called it'll do the right thing (extracting the C++ resolver object, then doing whatever it needs to with it).

Also, I suggest creating a #define or something for 0 in this case to give it a nice name, so it's clear in JSCallback that it's the same slot being used.

>+PromiseResolver::JSCallback(JSContext *aCx, unsigned aArgc, JS::Value *aVp)
>+  JSObject* callee = JSVAL_TO_OBJECT(JS_CALLEE(Cx, aVp));
>+
>+  JS::Value v = js::GetFunctionNativeReserved(callee, 0);

If you create the CallArgs right up front, this can be:

  JS::Value v = js::GetFunctionNativeReserved(&args.callee(), 0);

>+  JS::CallArgs args = JS::CallArgsFromVp(aArgc, aVp);
>+  Optional<JS::Handle<JS::Value> > value(aCx, args.length()
>+                                                ? args[0] : JS::UndefinedValue());

args.handleOrUndefinedAt(0).

By the way, the spec discussion makes it clear that a bunch of this stuff should just be taking "any", not "optional any".  I would dearly love us to fix that all up, as a followup.

>+  return JS_TRUE;

  return true;

Now that we added the thenable bit, we should take out the manual unwrap-to-a-promise bit, since that doesn't match the spec, right?

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Could we please try to use less JS API, and rely more on bindings or even xpconnect.
then() handling reminds eventlistener. We should be able reuse the infrastructure we have for event listeners.

Comment 7

[Andrea Marchesini [:baku]]

Attachment: WIP

Comment 8

[Andrea Marchesini [:baku]]

Attachment: patch

bz, do you think I can rewrite this using less JS API?

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 762106 [details] [diff] [review]
patch

In WrapperPromiseCallback::Call you need a EnterCompartment before the mNextResolver->RejectInternal call.

>+      JSObject* fulfillFunc = CreateFunction(aCx, valueObj, Promise::Fulfilled);

Again, this needs to be a Rooted.  So does the rejectFunc.

Or do what I suggested and put the argv creation _before_ the CreateFunction calls and store the objects in the argv immediately.  But the point is holding a raw JSObject* on the stack across any call which allocates memory is not allowed.

>+PromiseResolver::CreateFunction(JSContext* aCx, JS::Handle<JSObject*> aObj,
>+  JS::Rooted<JS::Value> wrappedResolver(aCx, JS::PrivateValue(this));
>+  js::SetFunctionNativeReserved(obj, SLOT_RESOLVER, wrappedResolver);

You actually don't need the Rooted here.  You can just do:

  js::SetFunctionNativeReserved(obj, SLOT_RESOLVER, JS::PrivateValue(this));

Except for the part where you do not in fact want to use the PromiseResolver here, because of the ownership issue I describe above.  You want the JSObject for it.

>+  JS::Rooted<JS::Value> wrappedState(aCx, INT_TO_JSVAL(aState));

Similar.  Also, JS::Int32Value(aState).

>+PromiseResolver::JSCallback(JSContext *aCx, unsigned aArgc, JS::Value *aVp)

>+    static_cast<PromiseResolver*>(JSVAL_TO_PRIVATE(v));

  static_cast<PromiseResolver*>(v.toPrivate());

but see above.

>+    static_cast<Promise::PromiseState>(JSVAL_TO_INT(v));

  static_cast<Promise::PromiseState>(v.toInt32());

>+      argv.infallibleAppend(OBJECT_TO_JSVAL(fulfillFunc));

JS::ObjectValue(*fulfillFunc), please.

>+  if (state == Promise::Fulfilled) {
>+    callback = PromiseCallback::Factory(resolver, nullptr,
>+                                        PromiseCallback::Fulfill);

Why not store a PromiseCallback::Task integer in the reserved slot instead of a Promise::PromiseState integer, and then you don't have to have this branch here...

>+++ b/dom/promise/PromiseResolver.h
>+  enum Task {
>+    TaskFulfill,
>+    TaskReject
>+  };

Seems to be unused?

>+  JSObject* CreateFunction(JSContext* aCx, JS::Handle<JSObject*> aObj,
>+                           Promise::PromiseState aState);
>+  static JSBool JSCallback(JSContext *aCx, unsigned aArgc, JS::Value *aVp);
>+  void HandleException(JSContext* aCx);

Please document.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh, and as far as using less JSAPI... this stuff follows different rules than WebIDL callbacks, so it's hard to reuse them here.  :(  In particular, { then: 5 } should just treat it as a non-thenable, whereas a WebIDL callback interface would try to call 5 and throw.

Comment 11

[Andrea Marchesini [:baku]]

Attachment: interdiff

I'm not sure about how I managed:

argv.infallibleAppend(JS::ObjectValue(*CreateFunction(aCx, value...

Probably this is not exactly what you meant. If not, let's talk on IRC. Thanks!

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 762606 [details] [diff] [review]
interdiff

Per irc discussion, need to fix the ownership stuff here and the possible null-derefs, and try to move the task enum to a public-enough header that we can use it here.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 762606 [details] [diff] [review]
interdiff

Er, r-.  ;)

Comment 14

[Andrea Marchesini [:baku]]

Attachment: WIP

Comment 15

[Andrea Marchesini [:baku]]

Attachment: patch

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 762745 [details] [diff] [review]
patch

r=me.

Might be worth adding a test where we take a |new Promise()|, set its .then to some function we control, and then send it through this codepath.  And have the test assert that our then() gets called (instead of the prototype one).

Comment 17

[Boris Zbarsky [:bzbarsky]]

Oh, and also worth testing in the "then" functions that their "this" value is correct.

Comment 18

[Andrea Marchesini [:baku]]

Attachment: patch

2 new tests added. Bug 875289 is needed before landing this patch.

Comment 19

[Mounir Lamouri (:mounir)]

Comment on attachment 763060 [details] [diff] [review]
patch

I haven't seen any consensus on that. I tend to think we should not implement this unless there are strong arguments for this.

This said, I think Dave should make the call.

Comment 20

[Mounir Lamouri (:mounir)]

FTR, one argument I have against implementing support for thenable objects is that having an experimental implementation without thenable support will allow us to know if thenables are actually needed. If we implement them now we will never know if supporting them is useful.

Comment 21

[Domenic Denicola]

There is fairly strong consensus on thenable support in TC39, and all of the DOM spec authors/the TAG are 100% behind it. It would be prudent to run this by es-discuss at least before creating a non-compliant fork of the DOM spec and the promise work done by the TAG.

Comment 22

[Mounir Lamouri (:mounir)]

(In reply to Domenic Denicola from comment #21)
> There is fairly strong consensus on thenable support in TC39, and all of the
> DOM spec authors/the TAG are 100% behind it. It would be prudent to run this
> by es-discuss at least before creating a non-compliant fork of the DOM spec
> and the promise work done by the TAG.

This is only going to ship behind a pref. Whatever we do, this is not going to be a fork.
Also, Dave Herman is part of TC39 AFAIK so I believe he should sr+ this patch if TC39 reached a consensus.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I too am against landing this until there is clear consensus within TC39 that this is the right thing to do.

This is an important change that changes the whole of ECMAScript in similar ways to how Object.prototype.__parent__ does.

That's not to say that it's not the right thing to do, but it's a decision that should come from TC39 and not from the DOM spec owners or the TAG.

Comment 24

[Andrea Marchesini [:baku]]

Attachment: patch

rebased

Comment 25

[Anne (:annevk)]

We should implement this as part of the implementing the new design.

Comment 26

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Anne, I believe this bug should be re-opened considering we decided to stick to the dom/promise implementation for at least a few more months and that Andrea has a patch that is almost good to go.

Comment 27

[Andrea Marchesini [:baku]]

(In reply to Nikhil Marathe [:nsm] from comment #26)
> Anne, I believe this bug should be re-opened considering we decided to stick
> to the dom/promise implementation for at least a few more months and that
> Andrea has a patch that is almost good to go.

The patch is reviewed and it's ready to land. We need to know if this is exactly what we want.
Anne, feedback?

Comment 28

[Anne (:annevk)]

So we want to support thenables, but the way it is implemented here is per the previous specification, right? So I guess this patch might not be okay. :/

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So the spec ended up with support for thenables after all? Rather than just doing branding :(

I take it in order to be compatible with currently existing libraries?

I guess if that's what TC39 decided on then that's what we should do. But I'm definitely saddened by it. Like you say, the past is shorter than the future.

Comment 30

[Anne (:annevk)]

Yes and yes and agreed.

Comment 31

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Implement thenables for Promises.

I've had to make one ugly hack and one not so ugly hacks to make things work, so bz if you could just go over it once and point out any obvious improvements, that would be appreciated.

1) Not so ugly. Change codegen so that callbacks can have a JSObject* thisObject. Thenables are required to have their 'then' method called with the thenable itself.

2) Ugly. When a thenable `then` is invoked with a resolveHandler and rejectHandler pair, it should be ensured that the first one to be called wins and is only called once. To capture this shared state, I had to use a JSObject* with a 'pending' property, since directly sharing a BooleanValue doesn't seem to be possible.

Comment 32

[Boris Zbarsky [:bzbarsky]]

1)  Is the interdiff from the earlier patch I reviewed here at all meaningful?  If not, don't worry about it, but if it is, I'd love to see it.

2)  Why remove the "FIXME Bug 878849" bit?  It's there because the code it's commenting is wrong, as bug 878849 describes.

3)  Some of this code is looking at reserved slot 1.  You should have a name for that 1.

4)  Please link to the spec you're actually implementing here.

5)  Please attach an example of the generated code you changes to the codegen produce?  I have to admit it's not quite clear to me why we want to add another overload of the method instead of just making WrapCallThisObject do the right thing in this case.  If I understand correctly what your codegen will produce, it's just begging for compartment mismatches.

As far as the shared boolean state, you could in fact just store pointers to some boolean in the reserved slots, but then you'd need to manage its lifetime somehow...  doing that via hanging it off a JS object is a bit heavyweight, but does mean you don't need custom finalize hooks or anything like that, which is good because you don't control the JSClass here.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Actually, you could do the shared boolean state like this.  You have two functions F1 and F2.  Both have slot 0 pointing to the promise.  You initially point slot 1 of F1 to F2 and slot 2 of F2 to F1.  If F1 is called, it checks whether its slot is undefined.  If it is, it does nothing.  Otherwise, it gets F2, sets both its slot and F2's to undefined, and does its stuff.  Similarly when F1 is called.  This seems to me like it would provide the invariants you want, right?

Comment 34

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Boris Zbarsky [:bz] from comment #32)
> 1)  Is the interdiff from the earlier patch I reviewed here at all
> meaningful?  If not, don't worry about it, but if it is, I'd love to see it.

Nope, things have changed quite a bit.

> 
> 2)  Why remove the "FIXME Bug 878849" bit?  It's there because the code it's
> commenting is wrong, as bug 878849 describes.

Sorry, removed some other FIXME and glossed over this bug number being different.

> 
> 3)  Some of this code is looking at reserved slot 1.  You should have a name
> for that 1.
> 
> 4)  Please link to the spec you're actually implementing here.

Spec is in the Bug URL. Particularly, https://github.com/domenic/promises-unwrapping#updatedeferredfrompotentialthenable--x-deferred- and friends.

> 
> 5)  Please attach an example of the generated code you changes to the
> codegen produce?  I have to admit it's not quite clear to me why we want to
> add another overload of the method instead of just making WrapCallThisObject
> do the right thing in this case.  If I understand correctly what your
> codegen will produce, it's just begging for compartment mismatches.

Well, the JSObject* I pass as the thisObject is not an nsISupports wrapper, it is a JavaScript object.

> 
> As far as the shared boolean state, you could in fact just store pointers to
> some boolean in the reserved slots, but then you'd need to manage its
> lifetime somehow...  doing that via hanging it off a JS object is a bit
> heavyweight, but does mean you don't need custom finalize hooks or anything
> like that, which is good because you don't control the JSClass here.

Comment 35

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Example code - PromiseBinding.h

The PromiseInit class has the generated code

Comment 36

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(In reply to Boris Zbarsky [:bz] from comment #33)
> Actually, you could do the shared boolean state like this.  You have two
> functions F1 and F2.  Both have slot 0 pointing to the promise.  You
> initially point slot 1 of F1 to F2 and slot 2 of F2 to F1.  If F1 is called,
> it checks whether its slot is undefined.  If it is, it does nothing. 
> Otherwise, it gets F2, sets both its slot and F2's to undefined, and does
> its stuff.  Similarly when F1 is called.  This seems to me like it would
> provide the invariants you want, right?

Yes, that should work. I got till making them point to each other, but didn't make the jump to using that as a state representation.

Comment 37

[Boris Zbarsky [:bzbarsky]]

> Well, the JSObject* I pass as the thisObject is not an nsISupports wrapper, it is a
> JavaScript object.

Yes, I understand that, but why can't we just have a specialization of WrapCallThisObject for that case?

Looking at the codegen, the "cx" argument to the new overload is ignored and the aThisObj is passed directly through to the private Call method.  But the private method assumes aThisObj is in the compartment of the cx it's passed, which in this case is s.GetContext().

So what I think we should do is specialize WrapCallThisObject for the T types we care about to wrap into the relevant compartment instead of adding another overload of Call().  Make sense?

Comment 38

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Implement thenables for Promises.

WrapCallThisObject specialization and callback pairs link to each other.

Domenic, just to be clear, a thenable's `then` is always called synchronously and the callbacks passed to it always resolve/reject the promise synchronously. Correct?

Comment 39

[Domenic Denicola]

Yes, but you should already be inside a new microtask with a clean stack by the time `then` is called, and resolving or rejecting a promise spawns a new microtask to execute the resolve/reject reactions.

Comment 40

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8355640 [details] [diff] [review]
Implement thenables for Promises.

>+++ b/dom/bindings/BindingUtils.h
>+WrapCallThisObject<JS::Rooted<JSObject*>>(JSContext* cx,

Maybe add a comment that we can't just add another overload of GetJSObjectFromCallback because WrapNativeParent won't compile for the case when "p" is a Rooted?

>+++ b/dom/promise/Promise.cpp
>+CreateLinks(JSContext* aCx, JS::Handle<JSObject*> aResolveFunc,

I'd prefer a slightly less generic name.  Maybe LinkThenableCallbacks?

>+CheckLinksAndBreak(JSContext* aCx, JS::Handle<JSObject*> aFunc, bool* aHadLink)

And for this one maybe call it MarkFunctionCalled and reverse the meaning of the out param and name it aCallbackCalledBefore or something?

Also, use a return value instead of an out param?  This method can never fail.

>+  JS::Rooted<JS::Value> otherFuncVal(aCx,

I don't think this actually needs to be rooted; we never do anything with it across possible GC calls.

>+  JS::Rooted<JSObject*> otherFuncObj(aCx, &otherFuncVal.toObject());

Same here.  So you could even skip the temporary here if you want and just js::SetFunctionNativeReserved(&otherFuncVal.toObject(), SLOT_DATA, JS::UndefinedValue()).

>+GetPromise(JSContext* aCx, JS::Handle<JSObject*> aFunc)
>+  JS::Rooted<JS::Value> v(aCx,
>+    js::GetFunctionNativeReserved(aFunc, SLOT_PROMISE));

Likewise doesn't need rooting.

It's probably ok to keep the rooting if this stuff is not super-hot and we feel safer with it, btw.

>+  if (NS_FAILED(UNWRAP_OBJECT(Promise, &v.toObject(), promise))) {

Can this really happen?  How?  I realize Promise::JSCallback does this sort of thing, but I think it's wrong.  ;)

The code duplication between JSCallbackThenableResolver and JSCallbackThenableRejecter is annoying.  I'd rather we either had a helper function that returns a Promise* (returning null in the case when we want to no-op) or have a helper function that takes a pointer-to-member argument and invokes it after getting the Promise* and whatnot.  The former is probably simper.

It might be a good idea to add comments explaining how these objects match up to the objects described in the spec.  

> Promise::ResolveInternal(JSContext* aCx,

So the fast path we have here for actual promise objects is wrong per spec, right?  Do we want to just remove it?  Or do we need to file a spec bug?

r=me with the above fixed.  Sorry this took so long...

Comment 41

[Boris Zbarsky [:bzbarsky]]

Per irc discussion, we want to:

1)  Add some tests that would have caught the incorrect Promise object special-casing in
    ResolveInternal.

2)  Remove the special-casing.

3)  Write a performance test that will let us see how much of a problem this is in practice.

4)  If it's a problem, use the techniques from bug 952891 to readd the special-casing in an
    undetectable way.

Comment 42

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

Attachment: Interdiff based on comment 41

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8364052 [details] [diff] [review]
Interdiff based on comment 41

>+LinkThenableCallables(JSContext* aCx, JS::Handle<JSObject*> aResolveFunc,
>             JS::Handle<JSObject*> aRejectFunc)

Fix the indent?

>Returns a Promise to
>+ * resolve/reject if it is ok to do so,

No, it just resolves/rejects it, I think.

The tests are still not testing the fast-path removal bit, right?

r=me with these fixed.

Comment 44

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7bcc6805f932

Comment 45

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7bcc6805f932

Comment 46

[Florian Scholz (Open Web Docs)]

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/then