932548 - Improve stack/message generation for uncaught Promise.jsm rejections

Status: RESOLVED FIXED

(Toolkit :: Async Tooling, enhancement)

Description

[Nils Maier [:nmaier]]

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.

Comment 1

[Nils Maier [:nmaier]]

Attachment: Improve stack/message generation for uncaught Promise.jsm rejections.

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...

Comment 2

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

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.

Comment 3

[Nils Maier [:nmaier]]

(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.

Comment 4

[Nils Maier [:nmaier]]

Attachment: Improve stack/message generation for uncaught Promise.jsm rejections.

Refined the existing tests and added a new test for nsIException objects.

Comment 5

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

Comment on attachment 824601 [details] [diff] [review]
Improve stack/message generation for uncaught Promise.jsm rejections.

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

Nice :)

Comment 6

[Nils Maier [:nmaier]]

Part of this green(-ish) try: https://tbpl.mozilla.org/?tree=Try&rev=eac3d574aaf6

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/4892bfe63cf8

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4892bfe63cf8

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4892bfe63cf8