926635 - Improve layout of uncaught Promise.jsm error messages

Status: RESOLVED FIXED

Description

[David Teller [:Yoric] (please use "needinfo")]

«
A promise chain failed to handle a rejection: on Mon Oct 14 2013 16:50:15 GMT+0200 (CEST), Error: BOOM! at
@Scratchpad/2:27
TaskImpl_run@resource://gre/modules/Task.jsm:217
TaskImpl@resource://gre/modules/Task.jsm:182
Task_spawn@resource://gre/modules/Task.jsm:152
@Scratchpad/2:26
»

That's a bit ugly.

It would be better to have:
«
A promise chain failed to handle a rejection.

Date of error: Mon Oct 14 2013 16:50:15 GMT+0200 (CEST)
Full message: Error: BOOM!
Full stack: @Scratchpad/2:27
TaskImpl_run@resource://gre/modules/Task.jsm:217
TaskImpl@resource://gre/modules/Task.jsm:182
Task_spawn@resource://gre/modules/Task.jsm:152
@Scratchpad/2:26
»

The code that needs to be changed lives in function observe() of the following file: http://dxr.mozilla.org/mozilla-central/source/toolkit/modules/Promise.jsm?from=Promise.jsm#l1

Comment 1

[Tareq Khandaker]

Created attachment 819504 [details] [diff] [review]
better-promise-error-message-layout.patch

Is there anything I can run to test my patch? Thanks,

Comment 2

[David Teller [:Yoric] (please use "needinfo")]

Comment on attachment 819504 [details] [diff] [review]
better-promise-error-message-layout.patch

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

Looks good to me.
Do you have Try access?

::: toolkit/modules/Promise.jsm
@@ +224,4 @@
>    }
>    error.init(
> +             /*message*/"A promise chain failed to handle a rejection.\n\nDate: " +
> +               date + "\nFull Message: " + message,

Could you take the opportunity to cut the string after the newlines?

Comment 3

[Tareq Khandaker]

Created attachment 819511 [details] [diff] [review]
better-promise-error-message-layout.patch

Like this? (so that the line is <80 characters)

Comment 4

[David Teller [:Yoric] (please use "needinfo")]

Comment on attachment 819511 [details] [diff] [review]
better-promise-error-message-layout.patch

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

Not exactly what I had in mind, but that looks good.
Here's the result from automated tests: https://tbpl.mozilla.org/?tree=Try&rev=0e3449691c5c

Comment 5

[David Teller [:Yoric] (please use "needinfo")]

That looks good. Unless you want to add any changes, we can land this.

Comment 6

[Tareq Khandaker]

I did not actually see what the new error message actually looked like because I did not know where to look.

Comment 7

[David Teller [:Yoric] (please use "needinfo")]

You can launch the promise tests as follows:
   ./mach xpcshell-test toolkit/modules/test/xpcshell/test_promise.js

At some point towards the end of the test, it will display the error message.

Comment 8

[Tareq Khandaker]

I got this error: An xpcshell.ini could not be found for the passed test path. Please select a path whose directory or subdirectories contain an xpcshell.ini file. It is possible you received this error because the tree is not built or tests are not enabled.

It's fine though. I'll take your word for it that it looks alright if you think it looks ready to land.

Comment 9

[David Teller [:Yoric] (please use "needinfo")]

Have you built Firefox?

Comment 10

[Tareq Khandaker]

Yeah, I have.

Comment 11

[David Teller [:Yoric] (please use "needinfo")]

Ah, that should have been test_Promise.js (with an uppercase P).

Comment 12

[Tareq Khandaker]

I got it to work. The command is ./mach xpcshell-test toolkit/modules/tests/xpcshell/test_Promise.js

It looks good to me.

 0:04.98 TEST-INFO | /Users/me/src/mozilla-central/obj-ff-dbg/_tests/xpcshell/toolkit/modules/tests/xpcshell/test_Promise.js | "Observing [JavaScript Error: "A promise chain failed to handle a rejection.

Date: Sat Oct 26 2013 17:59:49 GMT-0400 (EDT)
Full Message: Error: This is an uncaught error 962430.284671439
Full Stack: make_error_rejection@/Users/me/src/mozilla-central/obj-ff-dbg/_tests/xpcshell/toolkit/modules/tests/xpcshell/test_Promise.js:771
@/Users/me/src/mozilla-central/obj-ff-dbg/_tests/xpcshell/toolkit/modules/tests/xpcshell/test_Promise.js:781
@/Users/me/src/mozilla-central/obj-ff-dbg/_tests/xpcshell/toolkit/modules/tests/xpcshell/test_Promise.js:756
loadTailFile@/Users/me/src/mozilla-central/testing/xpcshell/head.js:420
_load_files@/Users/me/src/mozilla-central/testing/xpcshell/head.js:430
_execute_test@/Users/me/src/mozilla-central/testing/xpcshell/head.js:344
@-e:1
" {file: "/Users/me/src/mozilla-central/obj-ff-dbg/_tests/xpcshell/toolkit/modules/tests/xpcshell/test_Promise.js" line: 771 column: 0 source: "771"}]"

Comment 13

[David Teller [:Yoric] (please use "needinfo")]

Looks good to me.
If you're ready, we'll mark this bug checkin-needed.

Comment 14

[Tareq Khandaker]

I'm ready.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/419600845610