939636 - Ensure parity of JS Promises with Promise.jsm

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

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

Since DOM Promises has landed (preffed off) and is compliant with what will eventually become the ECMAScript Promise spec [1] it seems worthy to convert chrome code to it. In addition, tests can use DOM Promises by toggling the pref in the test runner configuration.

If everything works out, we can get rid of the two promise implementations we ship, Promise.jsm and promise.js.

I'm working on this in spare time, so might take a while. Patches from various code owners are welcome.

[1]: https://github.com/domenic/promises-unwrapping

Comment 1

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

Attachment: Part 1: Enable Promise bindings on chrome global scope.

bholley, this patch introduces Promise in chrome scope. Is the IsCallerChrome() check necessary? I want to enforce that content code calling those promise methods should always have a valid window, but I'm hoping this check will already be enforced in other places, because these frequent calls seem like a performance hit.

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8334083 [details] [diff] [review]
Part 1: Enable Promise bindings on chrome global scope.

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

(In reply to Nikhil Marathe [:nsm] from comment #1)
> bholley, this patch introduces Promise in chrome scope. Is the
> IsCallerChrome() check necessary? I want to enforce that content code
> calling those promise methods should always have a valid window

Why, exactly? As noted below, I think we should support this for Sandboxes as well, which don't have a window.

::: js/xpconnect/src/XPCRuntimeService.cpp
@@ +84,5 @@
>          *objpArg = objp;
>          return NS_OK;
>      }
>  
> +    *_retval = mozilla::dom::PromiseBinding::GetConstructorObject(cx, obj);

This isn't the right place to do this. This should go in the xpc::GlobalProperties stuff in Sandbox.cpp. This makes it accessible to all privileged globals (BSP or not) via Cu.importGlobalProperties(['Promise']), and sandboxes as well via wantGlobalProperties.

Given all the confusion with the different promise implementations, having an explicit import of the constructor into the namespace seems important IMO. If we really want to expose this everywhere by default, we can talk about a way to do that, but the basic machinery should still live in xpc::GlobalProperties.

Comment 3

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

(In reply to Bobby Holley (:bholley) from comment #2)
> Comment on attachment 8334083 [details] [diff] [review]
> Part 1: Enable Promise bindings on chrome global scope.
> 
> Review of attachment 8334083 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Nikhil Marathe [:nsm] from comment #1)
> > bholley, this patch introduces Promise in chrome scope. Is the
> > IsCallerChrome() check necessary? I want to enforce that content code
> > calling those promise methods should always have a valid window
> 
> Why, exactly? As noted below, I think we should support this for Sandboxes
> as well, which don't have a window.

Do sandboxes have only one global like workers then? Because the window is only required for GetParentObject().

Comment 4

[Bobby Holley (:bholley)]

(In reply to Nikhil Marathe [:nsm] (use needinfo?) from comment #3)
> (In reply to Bobby Holley (:bholley) from comment #2)
> > Comment on attachment 8334083 [details] [diff] [review]
> > Part 1: Enable Promise bindings on chrome global scope.
> > 
> > Review of attachment 8334083 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > (In reply to Nikhil Marathe [:nsm] from comment #1)
> > > bholley, this patch introduces Promise in chrome scope. Is the
> > > IsCallerChrome() check necessary? I want to enforce that content code
> > > calling those promise methods should always have a valid window
> > 
> > Why, exactly? As noted below, I think we should support this for Sandboxes
> > as well, which don't have a window.
> 
> Do sandboxes have only one global like workers then? Because the window is
> only required for GetParentObject().

This question is ill-formed. Sandboxes are globals. There are many sandboxes (and globals) in the main-thread (XPConnect) JSRuntime.

Why is a window required? If it is, then this won't work on BackstagePass scopes either. Can't you get what you need from an nsIGlobalObject (which both sandbox and BSP globals have)?

Comment 5

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

Attachment: Part 1: Enable Promise bindings on chrome global scope.

bholley, how about this?

Comment 6

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

Attachment: Promise.jsm uses DOM Promises underneath.

This replaces the JS implemented promises with DOM Promises in promise.jsm.
Converting all users of Promise.jsm is intensive, and may be done slowly, but this works for now.

Comment 7

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

This will lose some of the advantages list here: https://bugzil.la/810490#c0
- The ability to see which Promises are outstanding.
- Peeking into Promises

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8363867 [details] [diff] [review]
Part 1: Enable Promise bindings on chrome global scope.

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

Looks great! Please add a simple test along the lines of test_textDecoder.js. r=bholley with that

Comment 9

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

Really do we need to impoer "DOM" Promises to chrome global scope?

By ECMAScript 6 draft rev.22, ECMAScript 6 will introduce Promises as same as DOM one.
http://wiki.ecmascript.org/doku.php?id=harmony:specification_drafts#january_20_2014_draft_rev_22

I think that we can get Promises as ECMAScript native object in global scope when we get it in SpiderMonkey...

Comment 10

[Bobby Holley (:bholley)]

(In reply to Tetsuharu OHZEKI [UTC+9] from comment #9)
> Really do we need to impoer "DOM" Promises to chrome global scope?

The existing patch doesn't do that. Non-DOM chrome scopes need to do |Cu.importGlobalProperties['Promise'])|

Comment 11

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

(In reply to Tetsuharu OHZEKI [UTC+9] from comment #9)
> Really do we need to impoer "DOM" Promises to chrome global scope?
> 
> By ECMAScript 6 draft rev.22, ECMAScript 6 will introduce Promises as same
> as DOM one.
> http://wiki.ecmascript.org/doku.php?id=harmony:
> specification_drafts#january_20_2014_draft_rev_22
> 
> I think that we can get Promises as ECMAScript native object in global scope
> when we get it in SpiderMonkey...

The DOM Promises are ECMA compliant for the most part. It is not possible to add Spidermonkey promises in the near term.

Comment 12

[:Paolo Amadini]

Comment on attachment 8363868 [details] [diff] [review]
Promise.jsm uses DOM Promises underneath.

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

Hi Nikhil, thanks for working on the Promise conversion!

As you noted in comment 7, switching to DOM Promises now would make us lose several advantages that developers have found when writing Firefox chrome code using Promise.jsm.

We should look into closing the gap between the two implementations before making this move. This patch will be helpful, in that we can now compare the implementations more easily. I'll test your patch in the next few days and file bugs, though you should feel free to go ahead and file those bugs yourself if you prefer. The main concern areas are:

::: toolkit/modules/Promise.jsm
@@ -262,5 @@
> -    dump("A promise chain failed to handle a rejection\n\n");
> -    dump("On: " + date + "\n");
> -    dump("Full message: " + message + "\n");
> -    dump("See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise\n");
> -    dump("Full stack: " + (stack||"not available") + "\n");

Detailed console reporting of rejected promises with no handlers, on garbage collection.

Note how this includes a timestamp, error message, stack trace, and informative explanation.

This is done on all errors - even on promises that are rejected with a string or error code, as opposed to an Error object.

@@ -624,5 @@
> -   * promise will cause its state to be eventually propagated instead.  When the
> -   * N_STATUS property is STATUS_REJECTED, this contains the final rejection
> -   * reason, that could be a promise, even if this is uncommon.
> -   */
> -  Object.defineProperty(this, N_VALUE, { writable: true });

Inspection of current promise state and value/reason in the debugger (not accessible by code).

@@ -630,5 @@
> -  /*
> -   * Array of Handler objects registered by the "then" method, and not processed
> -   * yet.  Handlers are removed when the promise is resolved or rejected.
> -   */
> -  Object.defineProperty(this, N_HANDLERS, { value: [] });

Inspection of registered, pending handlers. This is fundamental in debugging the root cause of Promise-related memory leaks.

See https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Promise.jsm/Promise#Debugging.

@@ -788,5 @@
> -             ((nextStatus == STATUS_RESOLVED) ? "resolution":"rejection") +
> -             " callback.\n\n");
> -        dump("Full message: " + ex + "\n");
> -        dump("See https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Promise\n");
> -        dump("Full stack: " + (("stack" in ex)?ex.stack:"not available") + "\n");

Immediate "dump" of coding exceptions thrown by handlers. This is done for thrown exceptions, not explicit rejections, and is made in addition to the console report on garbage collection, if the error is not handled.

The list of early reported errors include EvalError, RangeError, ReferenceError, and TypeError.

This helps a lot in debugging automated tests, despite it is not meant for environments where "dump" is disabled.

Comment 13

[:Paolo Amadini]

Comment on attachment 8363868 [details] [diff] [review]
Promise.jsm uses DOM Promises underneath.

I filed detailed bugs for the above differences. We may define which ones we want to tackle first, but at least error and state reporting seem important enough to block migration.

Regardless of these important missing properties of the implementation, DOM Promises look like a simple drop-in replacement for Promise.jsm, so there should be no trouble in migrating later.

Comment 15

[Masatoshi Kimura [:emk]]

Comment on attachment 8363867 [details] [diff] [review]
Part 1: Enable Promise bindings on chrome global scope.

This patch has been superseded by bug 988122.

Comment 16

[:Paolo Amadini]

So, re-purposing given the scope of discussion and the dependencies.

Comment 17

[Masatoshi Kimura [:emk]]

Attachment #8363868 [details] [diff] is still needed. The most chrome codes are still using the object from Promise.jsm because the ES6 Promise object is replaceable. Importing Promise.jsm will replace the global "Promise" property.

Comment 18

[:Paolo Amadini]

Comment on attachment 8363868 [details] [diff] [review]
Promise.jsm uses DOM Promises underneath.

(In reply to Masatoshi Kimura [:emk] from comment #17)
> Attachment #8363868 [details] [diff] is still needed.

Sure, I obsoleted it by mistake. The attachment itself is old, but the concept remains valid. That conversion should only be done when the dependencies of this bug are addressed.

(In reply to Masatoshi Kimura [:emk] from comment #17)
> Importing Promise.jsm will replace the global "Promise" property.

This is what we want, until the dependencies are addressed.

Comment 19

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

https://hg.mozilla.org/users/nsm.nikhil_gmail.com/service-workers2/rev/4438fd9f3ae8
https://hg.mozilla.org/users/nsm.nikhil_gmail.com/service-workers2/rev/6b363bfd74f3
https://hg.mozilla.org/users/nsm.nikhil_gmail.com/service-workers2/rev/236f9ddeeb6e
https://hg.mozilla.org/users/nsm.nikhil_gmail.com/service-workers2/rev/167eeaee89df
https://hg.mozilla.org/users/nsm.nikhil_gmail.com/service-workers2/rev/82d7962b2e28
https://hg.mozilla.org/users/nsm.nikhil_gmail.com/service-workers2/rev/1fe757dfd53e

Comment 20

[Alex Vincent [:WeirdAl]]

Could someone please write a quick note on devmo contrasting the Promise.jsm implementation versus the native one, and how to safely convert to the new Promise model?  It sounds trivial; it's just not documented.

Comment 21

[David Teller [:Yoric] - still alive but not very active]

The main difference is that DOM Promise doesn't implement Promise.defer. Also, error reporting of Promise.jsm is better for the moment, but we are working on it.