1115405 - Erroneous Cu.reportError in Http.jsm

Status: RESOLVED FIXED

(Toolkit :: General, defect)

Description

[Mike Kaply [:mkaply]]

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

[Patrick Cloke [:clokep]]

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?

Comment 2

[Mike Kaply [:mkaply]]

(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.

Comment 3

[Mike Kaply [:mkaply]]

Attachment: Remove reportError

Simple patch that removes the reportError.

Comment 4

[Florian Quèze [:florian]]

Should this reportError call happen only when aOptions.onError is not set? I don't think this code should be silently eating errors.

Comment 5

[Mike Kaply [:mkaply]]

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

[Patrick Cloke [:clokep]]

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?

Comment 7

[Florian Quèze [:florian]]

(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.

Comment 8

[Mike Kaply [:mkaply]]

> 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

[Ben Bucksch (:BenB)]

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!

Comment 10

[Mike Kaply [:mkaply]]

Attachment: 1115405.diff

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.

Comment 11

[Mike Kaply [:mkaply]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/41e390a2cf6e

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/41e390a2cf6e