Closed
Bug 1115405
Opened 6 years ago
Closed 6 years ago
Erroneous Cu.reportError in Http.jsm
Categories
(Toolkit :: General, defect)
Toolkit
General
Tracking
()
RESOLVED
FIXED
mozilla38
Tracking | Status | |
---|---|---|
firefox38 | --- | fixed |
People
(Reporter: mkaply, Assigned: mkaply)
Details
Attachments
(1 file, 1 obsolete file)
1.41 KB,
patch
|
mossop
:
review+
|
Details | Diff | Splinter Review |
Http.jsm does a Cu.reportError here: http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Http.jsm#73 This shouldn't be there at all. It's up to the caller to catch it and report it.
Comment 1•6 years ago
|
||
Can I assume by this you mean that it is up to the caller to provide an onerror handler? (And thus decide whether or how they want to log the error?) you don't actually mean not catching the error, right?
Assignee | ||
Comment 2•6 years ago
|
||
(In reply to Patrick Cloke [:clokep] from comment #1) > Can I assume by this you mean that it is up to the caller to provide an > onerror handler? (And thus decide whether or how they want to log the > error?) you don't actually mean not catching the error, right? Yes, it's up to the caller. Http.jsm should be catching the error but not putting out the error message on its own. For instance, of an extension wanted to use this, they would get dinged by AMO for showing a message on the console, but they can't prevent the message from showing up.
Assignee | ||
Comment 3•6 years ago
|
||
Simple patch that removes the reportError.
Assignee: nobody → mozilla
Attachment #8543968 -
Flags: review?(clokep)
Comment 4•6 years ago
|
||
Should this reportError call happen only when aOptions.onError is not set? I don't think this code should be silently eating errors.
Assignee | ||
Comment 5•6 years ago
|
||
We silently eat the errors in the xhr onerror case if there is no onError handler... I think the assumption here is if the caller wants to see errors, they will add an onError handler.
Comment 6•6 years ago
|
||
Comment on attachment 8543968 [details] [diff] [review] Remove reportError Review of attachment 8543968 [details] [diff] [review]: ----------------------------------------------------------------- (In reply to Mike Kaply [:mkaply] from comment #5) > I think the assumption here is if the caller wants to see errors, they will > add an onError handler. This generally matches the usage of promises, I think. I find this to be a reasonable assumption. This problem with this is it can make development kind of painful, but you can always just set onError back to Cu.reportError and you're done. Anyway, actual review comments: Since Cu isn't used anywhere else, we should remove the definition on Line 7. Also...I'm not actually a toolkit reviewer, so we'll need Mossop or someone else to look at this at some point?
Attachment #8543968 -
Flags: review?(clokep) → review-
Comment 7•6 years ago
|
||
(In reply to Mike Kaply [:mkaply] from comment #5) > I think the assumption here is if the caller wants to see errors, they will > add an onError handler. Where is this assumption coming from? If it's because it's the XHR behavior, then my assumption is that people who want the raw XHR behavior won't be using a helper function wrapping it. I'm especially hesitant here because the try block contains the aOptions.onLoad call, so if the onLoad method provided by the caller causes JS errors, they will be eaten.
Assignee | ||
Comment 8•6 years ago
|
||
> Where is this assumption coming from? See: http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/Http.jsm#16 If you want errors, you pass an onError callback when you create the httpRequest via the helper.
Comment 9•6 years ago
|
||
Mike is right. Call error handler, caller is responsible. The caller might want to ignore the error. Heck, AMO is rejecting extensions for printing stuff on the console!
Assignee | ||
Comment 10•6 years ago
|
||
Pretty straightforward patch. Remove reportError that the caller would be responsible for catching. Remove Cu since we don't use it now. Discussion in bug.
Attachment #8543968 -
Attachment is obsolete: true
Attachment #8560560 -
Flags: review?(dtownsend)
Updated•6 years ago
|
Attachment #8560560 -
Flags: review?(dtownsend) → review+
Assignee | ||
Comment 11•6 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/41e390a2cf6e
Comment 12•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/41e390a2cf6e
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox38:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in
before you can comment on or make changes to this bug.
Description
•