Closed Bug 879245 Opened 6 years ago Closed 6 years ago

Implement thenables for promises

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: baku, Assigned: nsm)

References

(Blocks 1 open bug, )

Details

(Keywords: dev-doc-complete, Whiteboard: [qa-])

Attachments

(3 files, 10 obsolete files)

4.03 KB, text/plain
Details
12.32 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
13.42 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
No description provided.
Summary: FutureResolver.resolve() should support then-able objects → PromiseResolver.resolve() should support then-able objects
Attached patch patch (obsolete) — Splinter Review
Attachment #761429 - Flags: review?(bzbarsky)
Andrea, makes sure this gets an sr.
Comment on attachment 761429 [details] [diff] [review]
patch

This looks like the wrong diff.
Attachment #761429 - Flags: review?(bzbarsky)
Attached patch patch (obsolete) — Splinter Review
Too many patches with the same name...
Attachment #761429 - Attachment is obsolete: true
Attachment #761684 - Flags: review?(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?
Attachment #761684 - Flags: review?(bzbarsky) → review-
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.
Attached patch WIP (obsolete) — Splinter Review
Attachment #761684 - Attachment is obsolete: true
Attachment #762077 - Flags: feedback?(bzbarsky)
Attached patch patch (obsolete) — Splinter Review
bz, do you think I can rewrite this using less JS API?
Attachment #762077 - Attachment is obsolete: true
Attachment #762077 - Flags: feedback?(bzbarsky)
Attachment #762106 - Flags: review?(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.
Attachment #762106 - Flags: review?(bzbarsky) → review-
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.
Attached patch interdiff (obsolete) — Splinter Review
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!
Attachment #762106 - Attachment is obsolete: true
Attachment #762606 - Flags: review?(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.
Attachment #762606 - Flags: review?(bzbarsky) → review+
Comment on attachment 762606 [details] [diff] [review]
interdiff

Er, r-.  ;)
Attachment #762606 - Flags: review+ → review-
Attached patch WIP (obsolete) — Splinter Review
Attachment #762606 - Attachment is obsolete: true
Attachment #762737 - Flags: review?(bzbarsky)
Attached patch patch (obsolete) — Splinter Review
Attachment #762737 - Attachment is obsolete: true
Attachment #762737 - Flags: review?(bzbarsky)
Attachment #762745 - Flags: review?(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).
Attachment #762745 - Flags: review?(bzbarsky) → review+
Oh, and also worth testing in the "then" functions that their "this" value is correct.
Attached patch patch (obsolete) — Splinter Review
2 new tests added. Bug 875289 is needed before landing this patch.
Attachment #762745 - Attachment is obsolete: true
Depends on: 875289
Attachment #763060 - Flags: superreview?(mounir)
Depends on: 884754
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.
Attachment #763060 - Flags: superreview?(mounir) → superreview?(dherman)
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.
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.
(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.
Blocks: 885333
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.
Attached patch patch (obsolete) — Splinter Review
rebased
Attachment #763060 - Attachment is obsolete: true
Attachment #763060 - Flags: superreview?(dherman)
Attachment #767709 - Flags: superreview?(dherman)
We should implement this as part of the implementing the new design.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 911216
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.
Flags: needinfo?(annevk)
Status: RESOLVED → REOPENED
Flags: needinfo?(annevk)
Resolution: DUPLICATE → ---
Summary: PromiseResolver.resolve() should support then-able objects → Implement thenables for promises
Status: REOPENED → ASSIGNED
(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?
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. :/
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.
Yes and yes and agreed.
Blocks: 918806
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.
Assignee: amarchesini → nsm.nikhil
Attachment #767709 - Attachment is obsolete: true
Attachment #767709 - Flags: superreview?(dherman)
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.
Flags: needinfo?(nsm.nikhil)
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?
(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.
Flags: needinfo?(nsm.nikhil)
The PromiseInit class has the generated code
(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.
> 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?
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?
Attachment #8355640 - Flags: review?(bzbarsky)
Attachment #8355408 - Attachment is obsolete: true
Attachment #8355408 - Flags: feedback?(bzbarsky)
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 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...
Attachment #8355640 - Flags: review?(bzbarsky) → review+
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 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.
Attachment #8364052 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/7bcc6805f932
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Whiteboard: [qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.