Closed Bug 1115405 Opened 6 years ago Closed 6 years ago

Erroneous Cu.reportError in Http.jsm

Categories

(Toolkit :: General, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
firefox38 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

Details

Attachments

(1 file, 1 obsolete file)

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.
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?
(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.
Attached patch Remove reportError (obsolete) — Splinter Review
Simple patch that removes the reportError.
Assignee: nobody → mozilla
Attachment #8543968 - Flags: review?(clokep)
Should this reportError call happen only when aOptions.onError is not set? I don't think this code should be silently eating errors.
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 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-
(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.
> 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.
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!
Attached patch 1115405.diffSplinter Review
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)
Attachment #8560560 - Flags: review?(dtownsend) → review+
https://hg.mozilla.org/mozilla-central/rev/41e390a2cf6e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.