Ensure parity of JS Promises with Promise.jsm

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: nsm, Unassigned)

Tracking

(Blocks 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

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
Assignee: nobody → nsm.nikhil
Depends on: 939332
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.
Attachment #8334083 - Flags: review?(bobbyholley+bmo)
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.
Attachment #8334083 - Flags: review?(bobbyholley+bmo) → review-

Updated

6 years ago
Blocks: 856878
(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().
(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)?
bholley, how about this?
Attachment #8363867 - Flags: review?(bobbyholley+bmo)
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.
Attachment #8363868 - Flags: review?(paolo.mozmail)
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 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
Attachment #8363867 - Flags: review?(bobbyholley+bmo) → review+
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...
(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'])|
(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

6 years ago
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.

Updated

6 years ago
Depends on: 966452

Updated

6 years ago
Depends on: 966462

Updated

6 years ago
Depends on: 966471

Updated

6 years ago
Depends on: 966472

Comment 13

6 years ago
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.
Attachment #8363868 - Flags: review?(paolo.mozmail)
Duplicate of this bug: 988130
Comment on attachment 8363867 [details] [diff] [review]
Part 1: Enable Promise bindings on chrome global scope.

This patch has been superseded by bug 988122.
Attachment #8363867 - Attachment is obsolete: true

Comment 16

5 years ago
So, re-purposing given the scope of discussion and the dependencies.
Summary: Try to replace uses of Promise.jsm and promise.js with DOM Promises in codebase. → Ensure parity of DOM Promises with Promise.jsm

Updated

5 years ago
Attachment #8363868 - Attachment is obsolete: true
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

5 years ago
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.
Attachment #8363868 - Attachment is obsolete: false

Updated

5 years ago
Depends on: 989960

Updated

5 years ago
Blocks: 885333
No longer depends on: 966462, 966472
Assignee: nsm.nikhil → nobody
Blocks: 920281
No longer blocks: 920281

Updated

5 years ago
Depends on: 1033153
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.
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.
No longer blocks: dbg-source

Updated

4 years ago
Depends on: 1242505

Updated

3 years ago
Summary: Ensure parity of DOM Promises with Promise.jsm → Ensure parity of JS Promises with Promise.jsm

Updated

2 years ago
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED

Updated

2 years ago
Blocks: 1368456
You need to log in before you can comment on or make changes to this bug.