1011642 - Make Promises usable from external API code

Status: NEW

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

Description

[Joshua Cranmer [:jcranmer]]

Attachment: promise-utils

Here's the start of a patch I developed to make it possible to use promises from outside of libxul.

I defined this patch in the context of mailnews code because it's easier for me to test it from there. There's a requirement of fixing several header includes in mozilla-central to make mozilla/dom/Promise.h #includable when MOZILLA_INTERNAL_API isn't defined and also to export the vtable and methods of mozilla::dom::Promise and mozilla::dom::PromiseNativeHandler.

I've only tested this stuff on Linux, so there's no guarantee it works on all platforms.

The methods I've exposed are:
1. nsRefPtr<Promise> MakePromise(). Equivalent to |Promise.defer()|. I'm not sure about the global I ended up using here.
2. PromiseToJsval(Promise *, JSContext *, MutableHandleValue&). Converts a promise from a mozilla::dom::Promise to a jsval outparam for XPIDL conversions.
3. class PromiseWrapper. Converts a jsval thennable/Promise to a mozilla::dom::Promise. Used as follows:

PromiseWrapper wrapper;
callJSValAPI->GetPromise(&wrapper); // Returns a mutable handle value
nsRefPtr<Promise> p = wrapper.GetPromise();

4. class CXPush. Because AutoJSContext isn't usable from external code.
[I don't recall, but I think if I made AutoJSContext export its API methods, on Linux, every call to AutoJSContext would have to have a relocation applied, so I don't think it's necessarily viable to have AutoJSContext export its methods. Feel free to bikeshed on the name, though.]

In addition, I started developing a magic C++ conversion utility. The basic idea is to be able to match the Resolve/Reject handler of a promise to C++ methods:

PromiseThen<A, void, void>(promise, a, &A::OnSuccess, &A::OnFailure);
->
promise.then(bind(A::OnSuccess, a), bind(A::OnFailure, a));

I'm less sure about this step, especially since I've been unsucessful in trying to design the API so that it can automatically find the types of the template parameters.

A particular problem here is the inability of using ToJSValue from external code. For primitive types, it's not hard to explicitly instantiate templates and reference them with extern template, but that doesn't work for nsISupports* types in general or things like nsTArray or other corner cases. This also suggests the need for a simplified FromJSValue setup as well too.

Oh, and screw gcc 4.4 for the uint32_t/nsresult issue. :-(

Comment 1

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8424055 [details] [diff] [review]
promise-utils

It seems a bit weird, and quite a bit of overhead, to create a new global for every promise...

Comment 2

[Joshua Cranmer [:jcranmer]]

Any thoughts on the magic C++ stuff?

Comment 3

[Boris Zbarsky [:bzbarsky]]

Not offhand.  I can try to read through the details to see what it's doing, if needed...

But note that I'm happy to export whatever existing promise bits we have from libxul if that would help this out.

Comment 4

[Joshua Cranmer [:jcranmer]]

Attachment: Simpler external promise API

Comment 5

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8499962 [details] [diff] [review]
Simpler external promise API

Accidentally smacked the enter key...

After discussing use of promises again, I realized I could build a simple (if ugly) bridge to XPCOM-ish external code simply by using nsIVariant rather than trying to somehow make the ToJSValue template magic work for JS code. That's not to say I have no template magic--I use a little bit to punt a context variable to a pointer-to-member.

Another major change was that I didn't attempt to make mozilla/dom/Promise.h usable from external API code. That may be useful in its own right, but if I can get away without needing to, it's actually not a bad idea. This also means that the internal uses of Promise don't have to go through an exported blurb that potentially deoptimizes code.

This code is certainly buggy (there's a nice failure condition if you give nullptrs to the wrong parameters), but there are a few points that really need to be resolved before making a final, reviewable patch:

1. How do I go from a jsval "error" to an nsresult value? In the use cases I'm thinking of, preserving the result codes is definitely needed (e.g., it controls the user error messages we display).

2. I need the bridge between ExternalAPIPromise and jsval for XPIDL code. I have prototype code in the prior patch that I can probably shove in; testing that logic is another issue. My JSAPI ability is very weak though.

3. I think I need to tweak visibility of the templated methods somehow... (or maybe an anonymous namespace?)

4. Global objects... since I'm so used to the xpconnect/xpcom model of not needing to care about global objects, I have no idea what the best options are here.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8499962 [details] [diff] [review]
Simpler external promise API

> 1. How do I go from a jsval "error" to an nsresult value? In the use cases I'm
> thinking of, preserving the result codes is definitely needed (e.g., it
> controls the user error messages we display).

What makes you think there's an nsresult involved at all?  :(

That said, if you know for a fact an nsresult was involved then at the moment you have two options:

1)  Just get the .result property off the JS object, if it's an object.  If that's a number, get it as a number.

2)  Try to unwrap to Exception or DOMException (might be either one) and ->GetResult() in the C++.

> 2. I need the bridge between ExternalAPIPromise and jsval for XPIDL code.

What do you want this bridge to do?

> 3. I think I need to tweak visibility of the templated methods somehow...

I thought everything was visibility hidden by default?

> I have no idea what the best options are here

That really depends on the behavior you want.

The way the xpconnect/xpcom model works is it creates a separate JS object for every global you touch.  So you can set properties on your thing, then poke it in a different jsm or whatever and those properties are gone.  Oh, and it tries to keep things sane-ish by throwing if you set said properties...

The reason DOM objects need to be tied to globals is because they actually need a strong identity in terms of which proto chain they use.  Just creating new JS objects all the time and bailing on expandos is not an option there.

So what is your situation and where do the possible worries about globals come in?

Comment 7

[Joshua Cranmer [:jcranmer]]

(In reply to Boris Zbarsky [:bz] from comment #6)
> Comment on attachment 8499962 [details] [diff] [review]
> Simpler external promise API
> 
> > 1. How do I go from a jsval "error" to an nsresult value? In the use cases I'm
> > thinking of, preserving the result codes is definitely needed (e.g., it
> > controls the user error messages we display).
> 
> What makes you think there's an nsresult involved at all?  :(

Because the error codes involved in most use cases come from C++ (and use nsresult) or they are JS code throwing Cr.NS_ERROR_*; if neither of these are the case, falling back to something like NS_ERROR_DOM_JS_THREW_EXCEPTION is acceptable I think.

Effectively, I want the error handling done by nsXPCWrappedJS... except not necessarily the error reporting ... ?

> > 2. I need the bridge between ExternalAPIPromise and jsval for XPIDL code.
> 
> What do you want this bridge to do?

I need to basically be able to call or implement a function like
interface Foo {
  // Really a Promise<Bar>
  jsval completionPromise;
};

I don't necessarily need it to be a jsval, but I do want to support JS-implemented methods using thenables (e.g., Promise.jsm) in lieu of "real" promises.

> > 3. I think I need to tweak visibility of the templated methods somehow...
> 
> I thought everything was visibility hidden by default?

I'm exporting the call so it can be used by external API. The nm results are a little confusing, so I need to run a few tests to make sure things work.

> So what is your situation and where do the possible worries about globals
> come in?

Honestly... I don't think I care about expandos? Although, after observing the AbortableProgressPromise stuff, supporting "extra" abort methods might not be a bad idea, but I'm not necessarily sure that expandos would be the best way of doing that? (Especially since, at present, my concerns are external API code written in C++ which can't use expandos anyways).

My issues with globals are that I don't fully understand what starts happening when compartments and wrappers and globals are all fully involved. So, to a large degree, it's I don't fully understand what's going on, but I know enough background noise to fear that my decisions have strong consequences.

Comment 8

[Kent James (:rkent)]

Comment on attachment 8424055 [details] [diff] [review]
promise-utils

You have a bit of a name conflict here, in that Suyash and I landed mailnews/test/resources/PromiseTestUtils.jsm this summer which is used to wrap standard mailnews methods with promises. From my ExQuilla experience, I know that that code will probably be uplifted to non-test code, and then it becomes PromiseUtils.jsm

I don't believe that we should have two important libraries with such similar names within mailnews. I don't really care where it is changed, but since this is the new code it would be easier to just pick a different name here.

Otherwise, I am delighted that you are working on this.

Comment 9

[Joshua Cranmer [:jcranmer]]

(In reply to Joshua Cranmer [:jcranmer] from comment #7)
> I need to basically be able to call or implement a function like
> interface Foo {
>   // Really a Promise<Bar>
>   jsval completionPromise;
> };
> 
> I don't necessarily need it to be a jsval, but I do want to support
> JS-implemented methods using thenables (e.g., Promise.jsm) in lieu of "real"
> promises.

An alternative idea I thought as I started working on this support was building nsIVariant-ish magic into xpidl and/or xpconnect. This may introduce layering concerns (xpconnect depending on DOM), but I suspect it would work far better for both C++ people, JS people, and documentation purposes.

Thoughts?

Comment 10

[Boris Zbarsky [:bzbarsky]]

(In reply to Joshua Cranmer [:jcranmer] from comment #7)
> Because the error codes involved in most use cases come from C++ (and use
> nsresult) or they are JS code throwing Cr.NS_ERROR_*; if neither of these
> are the case, falling back to something like NS_ERROR_DOM_JS_THREW_EXCEPTION
> is acceptable I think.

I see.

So if someone threw Cr.foo then you get an exception Value that is the 32-bit unsigned integer for the nsresult.

But if someone threw in C++ then you get a DOMException or XPCException, which has a .result property.

So dealing with the JS and C++ cases here are quite different.

> Effectively, I want the error handling done by nsXPCWrappedJS...

Never thought I'd hear someone say that.  ;)  You almost certainly don't, because it does random stuff like swallowing exceptions.

> I need to basically be able to call or implement a function like
> interface Foo {
>   // Really a Promise<Bar>
>   jsval completionPromise;
> };
> 
> I don't necessarily need it to be a jsval, but I do want to support
> JS-implemented methods using thenables (e.g., Promise.jsm) in lieu of "real"
> promises.

OK, so something like this:

  callback interface FakePromise {
    FakePromise then(...); // We can figure out the args
  };

the idea being that all you can do with such a beastie is call then() on it and get another thenable, right?

> Honestly... I don't think I care about expandos? Although, after observing
> the AbortableProgressPromise stuff, supporting "extra" abort methods might
> not be a bad idea, but I'm not necessarily sure that expandos would be the
> best way of doing that? (Especially since, at present, my concerns are
> external API code written in C++ which can't use expandos anyways).

OK.  Then you could create multiple JS representations of your thing, I guess, if you wanted to.

Comment 11

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

We discussed this on IRC.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8499962 [details] [diff] [review]
Simpler external promise API

>+  if (VariantToJsval(aes.cx(), aValue, &asJsval)) {

Why this instead of adding a ToJSValue overload for nsIVariant?

>+nsCOMPtr<nsIGlobalObject> MakeGlobalObject(JSContext* cx)

Is there really no utility xpc:: API for this?  Might be worth getting bholley to look over this part.

>+  nsCOMPtr<nsIGlobalObject> global(MakeGlobalObject(cx));

Doesn't this give each promise a separate global?  That seems moderately fishy to me.

>+class EXPORTED_API PromiseDipper MOZ_STACK_CLASS

Where is this used?

Comment 13

[Joshua Cranmer [:jcranmer]]

(In reply to Please do not ask for reviews for a bit [:bz] from comment #12)
> >+nsCOMPtr<nsIGlobalObject> MakeGlobalObject(JSContext* cx)
> 
> Is there really no utility xpc:: API for this?  Might be worth getting
> bholley to look over this part.

There's no utility xpc:: API for creating new global objects, in so far as I can tell...

> >+  nsCOMPtr<nsIGlobalObject> global(MakeGlobalObject(cx));
> 
> Doesn't this give each promise a separate global?  That seems moderately
> fishy to me.

... but I think I could probably get away with using xpc::NativeGlobal(xpc::UnprivilegedJunkScope()), although those methods have a note saying "Callers MUST consult with the XPConnect module owner before
 * using this compartment. If you don't, bholley will hunt you down."

... so, bholley, comments?

> >+class EXPORTED_API PromiseDipper MOZ_STACK_CLASS
> 
> Where is this used?

That was the original API I used to convert between JSVal and XPCOMPromise for xpidl usages. Given that the IRC discussion agreed that adding a Promise primitive to xpidl was acceptable to the xpidl peer, I decided to replace that with doing this magic in xpidl itself. I don't have a preview of this, unfortunately.

Comment 14

[Joshua Cranmer [:jcranmer]]

Attachment: External API promise

Here's a newer version of the XPCOM promise API patch...

Comment 15

[Joshua Cranmer [:jcranmer]]

Attachment: Add a promise type to xpidl

... and here is a rough version of the xpidl side of things. The testing here is non-existent, but I've run tests locally on patches that I want to use this feature and it seems to work (on an older version of these patches, though).

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 8530598 [details] [diff] [review]
External API promise

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

Yeah, I'm ok with using the junk scope here - but note that you should be using the _privileged_ junk scope, not the unprivileged one, since this code is all system-only.

Comment 17

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: bz://1011642/jcranmer

Comment 18

[Joshua Cranmer [:jcranmer]]

/r/2299 - Bug 1011642, part 1: Make DOM Promises usable from external API code, r=bz
/r/2301 - Bug 1011642, part 2: Add the ability to pass promises through XPIDL, r=khuey,bholley

Pull down these commits:

hg pull review -r 3ecfa7c1043515783d6235024b486b90801b7996

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8546784 [details]
MozReview Request: bz://1011642/jcranmer

https://reviewboard.mozilla.org/r/2297/#review2521

::: dom/promise/XPCOMPromise.h
(Diff revision 1)
> +  NS_INLINE_DECL_REFCOUNTING(XPCOMPromise)

What, if anything, is the cycle collection story here?

::: dom/promise/XPCOMPromise.h
(Diff revision 1)
> +   * When the promise gets resolved or rejected, call the appropriate function

Should point out that aContext will be passed to aResolve or aReject as its third argument.

::: dom/promise/XPCOMPromise.h
(Diff revision 1)
> +  explicit XPCOMPromise(dom::Promise* promise);

Should the ctor be #ifdef IMPL_LIBXUL?

::: dom/promise/XPCOMPromise.h
(Diff revision 1)
> +  void Resolve(nsIVariant* aValue);

Should this be MaybeResolve, like for a normal Promise?

::: dom/promise/XPCOMPromise.h
(Diff revision 1)
> +  void Reject(nsresult aError);

And MaybeReject?

::: dom/promise/XPCOMPromise.h
(Diff revision 1)
> +  dom::Promise* AsPromise() { return mPromise; }

Should this be #ifdef IMPL_LIBXUL?

::: dom/promise/XPCOMPromise.h
(Diff revision 1)
> +    nsresult (T::*aResolve)(nsIVariant*, nsIWritableVariant*),

I think aFulfillCallback would be clearer here than aResolve.  And aRejectCallback instead of aReject.  Same for the non-template Then()

::: dom/promise/XPCOMPromise.h
(Diff revision 1)
> +  typedef nsresult (*ResolveFn)(nsIVariant*, nsIWritableVariant*, void*);
> +  typedef nsresult (*RejectFn)(nsresult, nsIWritableVariant*, void*);

Document that the second argument is used as the return value (filling in a caller-provided variant), since that's not a normal XPCOM pattern.

And maybe aFulfillFn?

::: dom/promise/XPCOMPromise.h
(Diff revision 1)
> +    nsresult (T::*mResolve)(nsIVariant*, nsIWritableVariant*);

Again, mFulfill.  Or maybe mFulfilled and mRejected?

::: dom/promise/XPCOMPromise.h
(Diff revision 1)
> +    static nsresult DoResolve(nsIVariant*, nsIWritableVariant*, void*);
> +    static nsresult DoReject(nsresult, nsIWritableVariant*, void*);

Document DoResolve/DoReject?

::: dom/promise/XPCOMPromise.cpp
(Diff revision 1)
> +  nsCOMPtr<nsIWritableVariant> output(do_CreateInstance(NS_VARIANT_CONTRACTID));

Why not just new nsVariant?

::: dom/promise/XPCOMPromise.cpp
(Diff revision 1)
> +  nsRefPtr<XPCVariant> input = XPCVariant::newVariant(aCx, aValue);

Can't "input" be null here, in which case an exception is presumably (at least possibly) sitting on aCx or something?

::: dom/promise/XPCOMPromise.cpp
(Diff revision 1)
> +    mResult->MaybeResolve(static_cast<nsIVariant*>(output));

We should just have an nsIWritableVariant* specialization of ToJSValue, no?

::: dom/promise/XPCOMPromise.cpp
(Diff revision 1)
> +    if (!JS_GetProperty(cx, obj, "result", &resultVal)) {

You need to either enter the compartment of "obj" or wrap it into the compartment of "cx".  Probably the former.  Otherwise in a debug build this will do fun asserty things.

But also, why not use aCx?

::: dom/promise/XPCOMPromise.cpp
(Diff revision 1)
> +      resultVal.setUndefined();

Need to at least clear the exception off the JSContext here, no?

::: dom/promise/XPCOMPromise.cpp
(Diff revision 1)
> +  nsCOMPtr<nsIWritableVariant> output(do_CreateInstance(NS_VARIANT_CONTRACTID));

Again, just allocate with new.

::: dom/promise/XPCOMPromise.cpp
(Diff revision 1)
> +    mResult->MaybeResolve(static_cast<nsIVariant*>(output));

And ditch the cast.

::: dom/promise/XPCOMPromise.cpp
(Diff revision 1)
> +{

Please assert mainthread, since you're depending on XPConnect variants, and I suspect those are not threadsafe?

::: dom/promise/XPCOMPromise.cpp
(Diff revision 1)
> +    xpc::NativeGlobal(xpc::PrivilegedJunkScope());

This bit needs review from bholley.

::: dom/promise/XPCOMPromise.cpp
(Diff revision 1)
> +  AutoJSContext cx;

Where is this used?

::: dom/promise/XPCOMPromise.h
(Diff revision 1)
> +// This header isn't necessary for this file, but if the declaration contained
> +// in there comes after the definition here, then GCC warns about "useless"

Document that you mean the declaration and definition of XPCOMPromise.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8546784 [details]
MozReview Request: bz://1011642/jcranmer

https://reviewboard.mozilla.org/r/2297/#review2525

I'd like bholley to look over the xpconnect changes.  r=me otherwise.

Comment 21

[Joshua Cranmer [:jcranmer]]

https://reviewboard.mozilla.org/r/2297/#review2527

> This bit needs review from bholley.

He approved it already: https://bugzilla.mozilla.org/show_bug.cgi?id=1011642#c16

> What, if anything, is the cycle collection story here?

I'm used to comm-central code, where cycle collection is basically unused.

Where I've used XPCOMPromise in further patches, it's only been on the stack, not a member variable, so XPCOMPromise doesn't get involved in any refcount cycles. Of course, I haven't used it much in further patches. :-)

The URL listener pattern that this would replace often looks basically like this:

```
m_urlListener->OnStopRunningUrl(m_runningUrl, status);
m_urlListener = nullptr;
```

So while it does admit the possibility of leaks in badly written code, the prevailing code pattern is already clean up after resolution, so I don't think cycle collection is necessary.

Comment 22

[Boris Zbarsky [:bzbarsky]]

I've seen tons of cycles when people normally use promises.  Things like loops in the promise resolution chain, even.

Is there a reason to _not_ cycle-collect it?

Comment 23

[Joshua Cranmer [:jcranmer]]

https://reviewboard.mozilla.org/r/2297/#review2647

> Should the ctor be #ifdef IMPL_LIBXUL?

If I guard the only constructor with IMPL_LIBXUL, then we get a default constructor generated for us... to which an available but unusable constructor seems preferable.

Comment 24

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8546784 [details]
MozReview Request: bz://1011642/jcranmer

/r/2299 - Bug 1011642, part 1: Make DOM Promises usable from external API code, r=bz
/r/2301 - Bug 1011642, part 2: Add the ability to pass promises through XPIDL, r=khuey,bholley

Pull down these commits:

hg pull review -r cf014035a83ed07e56de7ed80c52da6809fb0a09

Comment 25

[Boris Zbarsky [:bzbarsky]]

Can you make it an explicitly deleted ctor outside IMPL_LIBXUL?

Past that, I'd like to see an interdiff from the last thing I reviewed to what you want me to review now.

Comment 26

[Joshua Cranmer [:jcranmer]]

(In reply to Not doing reviews right now from comment #25)
> Can you make it an explicitly deleted ctor outside IMPL_LIBXUL?

I'll need to take some time to assure myself that it won't impact ABI, but otherwise, yes.

> Past that, I'd like to see an interdiff from the last thing I reviewed to
> what you want me to review now.

It's a built-in feature to reviewboard: <https://reviewboard.mozilla.org/r/2297/diff/1-2/>. (The top of a diff page has a slider that lets you see interdiff between different revisions of the patches).

Comment 27

[Bobby Holley (:bholley)]

https://reviewboard.mozilla.org/r/2301/#review4413

Cool stuff! Thanks for adding such nice tests. :-)

::: js/xpconnect/tests/idl/xpctest_promise.idl
(Diff revision 2)
> +  Promise doAddOne(in Promise newPromise);

Is "newPromise" (here and elsewhere) really the appropriate name? Shouldn't it be "firstPromise" or somesuch?

Comment 28

[Bobby Holley (:bholley)]

https://reviewboard.mozilla.org/r/2301/#review4415

Ship It!

Comment 29

[Bobby Holley (:bholley)]

What is the appropriate way to clear the review request in bugzilla?

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8546784 [details]
MozReview Request: bz://1011642/jcranmer

https://reviewboard.mozilla.org/r/2297/#review4549

::: dom/promise/XPCOMPromise.h
(Diff revisions 1 - 2)
> +     * The DoFulfill and DoReject methods are variants are the methods that get

This comment is not making sense.  Please fix it to do so.

::: dom/promise/XPCOMPromise.h
(Diff revisions 1 - 2)
> -  typedef nsresult (*ResolveFn)(nsIVariant*, nsIWritableVariant*, void*);
> +  typedef nsresult (*FulfilledFn)(nsIVariant*, nsIWritableVariant*, void*);

Still need to document the meaning of the second argument here.

::: dom/promise/XPCOMPromise.cpp
(Diff revisions 1 - 2)
>      if (!JS_GetProperty(cx, obj, "result", &resultVal)) {

Need to address previous review comment here.

::: dom/promise/XPCOMPromise.cpp
(Diff revisions 1 - 2)
>        resultVal.setUndefined();

And here.

Please do go through and address all the old review comments...  Pretty please.

Comment 31

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

Comment on attachment 8546784 [details]
MozReview Request: bz://1011642/jcranmer

To be perfectly honest, I'm not really interested in reviewing this.  If we do agree that exposing Promises outside of core Gecko is a good idea (and I'm not convinced by that), make whatever changes to XPIDL you need.  If there's a specific thing you're not sure about I'll answer questions, but I'm not really interested in reviewing working code here.

Comment 33

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: Bug 1011642, part 1: Make DOM Promises usable from external API code, r=bz

Comment 34

[Joshua Cranmer [:jcranmer]]

Attachment: MozReview Request: Bug 1011642, part 2: Add the ability to pass promises through XPIDL, r=khuey,bholley

Comment 35

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.