errUtils.js' logException should dump exceptions via Components.utils.reportError too for fancy mozmill logging

RESOLVED FIXED in Thunderbird 18.0

Status

MailNews Core
Testing Infrastructure
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: asuth, Assigned: aceman)

Tracking

Trunk
Thunderbird 18.0
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.30 KB, patch
aceman
: review+
Details | Diff | Splinter Review
(Reporter)

Description

6 years ago
We are using logException all over the place.  logException is a noble and great thing, but it only dumps via stdout.  It should also log exceptions via Cu.reportError.  Maybe always, or maybe only if we're not re-throwing.

While my greatest concern is for the fancy mozmill logging, we are using it a lot, and arguably when QA people ask "anything in the error console?" and these failures are hidden from that, we are losing out.

There are other ways we could hook this up to fancy mozmill logging (and we could get fancy about it), but I think a good first step is just the Cu.reportError thing.

The argument against doing this is that the exceptions may be harmless and could needlessly concern users.  The counter-argument is that a harmless error should be explicitly ignored in the exception handler if that is the case; the fact that the developer wanted to see it dumped suggested they were concerned (or didn't know that logException wouldn't send things to the error console.)
(Assignee)

Comment 1

5 years ago
Asuth are you working on this?

The logException function in used in many places outside tests in main code. Let's try it and see what errors show up suddenly.
(Reporter)

Comment 2

5 years ago
I am not working on this.  It would be great if you want to pursue this!
Assignee: bugmail → nobody
Status: ASSIGNED → NEW
(Assignee)

Updated

5 years ago
Assignee: nobody → acelists
(Assignee)

Comment 3

5 years ago
Created attachment 666033 [details] [diff] [review]
patch
Attachment #666033 - Flags: review?(mbanner)
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
Attachment #666033 - Flags: review?(mbanner) → review?(bugmail)
(Reporter)

Comment 4

5 years ago
Comment on attachment 666033 [details] [diff] [review]
patch

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

Chronologically speaking, I think it makes sense to have the message reported first, so that when skimming one will see the message and then the specific exception.  However, either way is a major improvement!  Thanks for doing this.
Attachment #666033 - Flags: review?(bugmail) → review+
(Assignee)

Comment 5

5 years ago
Created attachment 667583 [details] [diff] [review]
patch v2
Attachment #666033 - Attachment is obsolete: true
Attachment #667583 - Flags: review+
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/ac1839e646e5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite-
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 18.0
(Assignee)

Updated

3 years ago
Blocks: 1015774
You need to log in before you can comment on or make changes to this bug.