Improve stack/message generation for uncaught Promise.jsm rejections

RESOLVED FIXED in mozilla28

Status

()

enhancement
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: nmaier, Assigned: nmaier)

Tracking

Trunk
mozilla28
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Right now uncaught Promise.jsm rejections will only include a stack if the rejection reason is an Error-like value having a `.stack` member.
Also the message is always the stringified reason.

This does not play particularly nice with the quirky nsIException implementation that XPConnect and some DOM stuff throws at you, compared to JS Errors:
- |.toString()| will actually contain the top level stack frame along with the |.message|.
- |.filename| vs |.fileName|
- |.location| (nsIStackFrame) instead of |.stack| (string)

Also, the implementation does not attempt to derive a stack if there is no valid |.stack|. But it could be derived from Components.stack.

So improve the code to a) handle nsIException objects and b) derive a stack if there isn't any.
I'm not sure about using |.stack| at all. It is not necessarily Error.stack ;)

Also not sure about tests... Adding a console listener and waiting around for a GC run to collect the witness seems like overkill.
Or I could cheat:
{PendingErrors} = Cu.import("resource://gre/modules/Promise.jsm", {});
While a lot of (add-on) code relies on this Cu.import property to return the global of the module instead of just the EXPORTED_SYMBOLS, it might not be wise to rely on undocumented behavior...
Assignee: nobody → maierman
Status: NEW → ASSIGNED
Attachment #824340 - Flags: review?(dteller)
There is already such a GC test in test_Promise.js, you can use it.

Regarding |Components.stack|, I'm almost sure that the info will be completely useless, since our implementation of Promise is mostly stackless.
(In reply to David Rajchenbach Teller [:Yoric] from comment #2)
> Regarding |Components.stack|, I'm almost sure that the info will be
> completely useless, since our implementation of Promise is mostly stackless.

No, the code path goes like follows and is not interrupted by nsIThread.dispatch (or any other high level API, such as setTimeout).
User code: deferedInstance.reject()
Promise.jsm: Deferred.prototype.reject()
Promise.jsm: PromiseWalker.prototype.completePromise()
Promise.jsm: PendingErrors.register() -> Bingo!
So the rejection callsite is still within the stack frames at the point the error is registered. It doesn't matter that later on when the uncaught rejection is logged the stack is completely different, of course.

Have an extremely simple test case, to be run in a privileged Scratchpad:
Components.utils.import("resource://gre/modules/Promise.jsm");
Promise.defer().reject();

And here is the result with the patch applied (this takes the Components.stack codepath as the rejection reason is undefined and therefore neither nsIException nor Error-like):
A promise chain failed to handle a rejection.
Date: Wed Oct 30 2013 12:33:34 GMT+0100 (CET)
Full Message: undefined
Full Stack: JS frame :: Scratchpad/1 :: <TOP_LEVEL> :: line 2
JS frame :: resource://gre/modules/devtools/dbg-server.jsm -> resource://gre/modules/devtools/server/main.js -> resource://gre/modules/devtools/server/actors/webconsole.js :: WCA_evalWithDebugger :: line 958
...
The result without the patch completely lacks any mention of the call stack, of course.
Refined the existing tests and added a new test for nsIException objects.
Attachment #824340 - Attachment is obsolete: true
Attachment #824340 - Flags: review?(dteller)
Attachment #824601 - Flags: review?(dteller)
Comment on attachment 824601 [details] [diff] [review]
Improve stack/message generation for uncaught Promise.jsm rejections.

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

Nice :)
Attachment #824601 - Flags: review?(dteller) → review+
Part of this green(-ish) try: https://tbpl.mozilla.org/?tree=Try&rev=eac3d574aaf6
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/4892bfe63cf8
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4892bfe63cf8
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla28
You need to log in before you can comment on or make changes to this bug.