Improve layout of uncaught Promise.jsm error messages

RESOLVED FIXED in mozilla27

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Yoric, Assigned: tareqakhandaker)

Tracking

unspecified
mozilla27
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Async][mentor=Yoric][lang=js])

Attachments

(1 attachment, 1 obsolete attachment)

«
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
(Assignee)

Comment 1

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

Is there anything I can run to test my patch? Thanks,
Assignee: nobody → tareqakhandaker
Attachment #819504 - Flags: review?(dteller)
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?
Attachment #819504 - Flags: review?(dteller) → review+
(Assignee)

Comment 3

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

Like this? (so that the line is <80 characters)
Attachment #819504 - Attachment is obsolete: true
Attachment #819511 - Flags: review?(dteller)
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
Attachment #819511 - Flags: review?(dteller) → review+
That looks good. Unless you want to add any changes, we can land this.
(Assignee)

Comment 6

5 years ago
I did not actually see what the new error message actually looked like because I did not know where to look.
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.
(Assignee)

Comment 8

5 years ago
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.
Have you built Firefox?
(Assignee)

Comment 10

5 years ago
Yeah, I have.
Ah, that should have been test_Promise.js (with an uppercase P).
(Assignee)

Comment 12

5 years ago
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"}]"
Looks good to me.
If you're ready, we'll mark this bug checkin-needed.
Flags: needinfo?(tareqakhandaker)
(Assignee)

Comment 14

5 years ago
I'm ready.
Flags: needinfo?(tareqakhandaker)
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/419600845610
Keywords: checkin-needed
Whiteboard: [Async][mentor=Yoric][lang=js] → [Async][mentor=Yoric][lang=js][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/419600845610
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [Async][mentor=Yoric][lang=js][fixed-in-fx-team] → [Async][mentor=Yoric][lang=js]
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.