Closed Bug 926635 Opened 12 years ago Closed 12 years ago

Improve layout of uncaught Promise.jsm error messages

Categories

(Toolkit :: Async Tooling, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: Yoric, Assigned: tareqakhandaker)

Details

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

Attachments

(1 file, 1 obsolete file)

« 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
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+
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.
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.
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?
Yeah, I have.
Ah, that should have been test_Promise.js (with an uppercase P).
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)
I'm ready.
Flags: needinfo?(tareqakhandaker)
Keywords: checkin-needed
Whiteboard: [Async][mentor=Yoric][lang=js] → [Async][mentor=Yoric][lang=js][fixed-in-fx-team]
Status: NEW → RESOLVED
Closed: 12 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.

Attachment

General

Created:
Updated:
Size: