Closed
Bug 926635
Opened 11 years ago
Closed 11 years ago
Improve layout of uncaught Promise.jsm error messages
Categories
(Toolkit :: Async Tooling, defect)
Toolkit
Async Tooling
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: Yoric, Assigned: tareqakhandaker)
Details
(Whiteboard: [Async][mentor=Yoric][lang=js])
Attachments
(1 file, 1 obsolete file)
1.40 KB,
patch
|
Yoric
:
review+
|
Details | Diff | Splinter Review |
« 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•11 years ago
|
||
Is there anything I can run to test my patch? Thanks,
Assignee: nobody → tareqakhandaker
Attachment #819504 -
Flags: review?(dteller)
Reporter | ||
Comment 2•11 years ago
|
||
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•11 years ago
|
||
Like this? (so that the line is <80 characters)
Attachment #819504 -
Attachment is obsolete: true
Attachment #819511 -
Flags: review?(dteller)
Reporter | ||
Comment 4•11 years ago
|
||
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+
Reporter | ||
Comment 5•11 years ago
|
||
That looks good. Unless you want to add any changes, we can land this.
Assignee | ||
Comment 6•11 years ago
|
||
I did not actually see what the new error message actually looked like because I did not know where to look.
Reporter | ||
Comment 7•11 years ago
|
||
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•11 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.
Reporter | ||
Comment 9•11 years ago
|
||
Have you built Firefox?
Assignee | ||
Comment 10•11 years ago
|
||
Yeah, I have.
Reporter | ||
Comment 11•11 years ago
|
||
Ah, that should have been test_Promise.js (with an uppercase P).
Assignee | ||
Comment 12•11 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"}]"
Reporter | ||
Comment 13•11 years ago
|
||
Looks good to me. If you're ready, we'll mark this bug checkin-needed.
Flags: needinfo?(tareqakhandaker)
Reporter | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 15•11 years ago
|
||
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]
Comment 16•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/419600845610
Status: NEW → RESOLVED
Closed: 11 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.
Description
•