591102 - Correctly identify network errors

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Mike Connor [:mconnor]]

Right now, if the user loses network, we show a sync error.  we should probably suppress that if the user is actually offline, and do a sync on idle when the app online state changes.

Comment 1

[Tony Chung [:tchung]]

request blocking

Comment 2

[[e-mail]]

Mike, can I yoink this?

Comment 3

[[e-mail]]

From an IRC discussion, so I don't forget:

<philikon> dwitte: so apart from this UI problem about showing notifications too often, there's also an actual bug in our code
<philikon> dwitte: certain network errors (e.g. timeouts) are represented as unknown errors
<philikon> rather than a network error
<dwitte> at what layer is that bug residing?
<philikon> services/sync/modules/service.js
<philikon> for that error misrepresentation thiing we want to amend Service._checkServerError i think
<philikon> to set Status.service = LOGIN_FAILED_NETWORK_ERROR
<philikon> the name of the constant sounds a bit stupid, but we don't have another one for network failures
<philikon> and it gets us the right user facing message with l10n and all

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

I actually think the softblocker here is supressing the errors, not the actual bug in the code, fwiw.

Comment 5

[Philipp von Weitershausen [:philikon]]

(In reply to comment #4)
> I actually think the softblocker here is supressing the errors, not the actual
> bug in the code, fwiw.

Suppressing the errors? I don't think we need to spam users with network-related errors. They're recoverable.

Comment 6

[[e-mail]]

(In reply to comment #4)
> I actually think the softblocker here is supressing the errors, not the actual
> bug in the code, fwiw.

Yeah, it's slightly confusing me: we have bug 600261 filed in Sync:UI which I was taking to mean "don't spam the user on network errors" and then this one in Sync:Backend which I was taking to mean "make network errors discernible from other errors". Which makes me think that this should block bug 600261. Amirite?

Comment 7

[Philipp von Weitershausen [:philikon]]

(In reply to comment #6)
> Amirite?

Yep

Comment 8

[[e-mail]]

I'll just yoink this and Mike can yoink it back if he wants :)

Comment 9

[Philipp von Weitershausen [:philikon]]

(In reply to comment #3)
> <philikon> for that error misrepresentation thiing we want to amend
> Service._checkServerError i think
> <philikon> to set Status.service = LOGIN_FAILED_NETWORK_ERROR

So _checkServerError should probably do this when the error result is one of NS_ERROR_NET_TIMEOUT, NS_ERROR_NET_RESET, NS_ERROR_NET_INTERRUPT.

Comment 10

[Philipp von Weitershausen [:philikon]]

NS_ERROR_PROXY_* (e.g. NS_ERROR_PROXY_CONNECTION_REFUSED) are some more candidates too.

Comment 11

[Philipp von Weitershausen [:philikon]]

YOINK

Bug 600261 is about not surfacing network errors (at least not when they can be recovered from in a reasonable time frame). This reduces the scope of this bug to just correctly identifying network errors. Some of them pop up as Unknown Errors.

To summarize, in this bug I'll be making sure that network errors are correctly identified and the right status bits are set (Status.service = LOGIN_FAILED_NETWORK_ERROR). In bug 600261 we can make the UI suck less.

Comment 12

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v1.0): Ensure resource exceptions contain status code

This ensures that exceptions from [Async]Resource contain the status code. That way the service can inspect it and set the appropriate bits accordingly (Part 2 patch).

This also makes the channel timeout configurable (for testing) and adds tests for said timeout.

Comment 13

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v1.0): Correctly identify network errors

This makes service aware of the response status codes (Part 1 patch) and set the right Status bits on network related errors.

Service._checkServerError() is already called for each exception thrown by engine.sync(). This now also calls it from Service._fetchInfo() which is called at the beginning of a sync. That way, when the network is down, we don't actually say "Unknown Error" but "Failed to connect to server." Added tests for both behaviours.

Also fixed and added tests for Service.verifyLogin() when clusterURL isn't set yet and the auth node fails to be contacted. Would throw an exception all the way up, now sets the right Status bits too.


This is it for this bug. In bug 600261 I will make sure that we don't actually spam the user when Service.login or Service.sync is LOGIN_FAILED_NETWORK_ERROR.

Comment 14

[Philipp von Weitershausen [:philikon]]

Try build: http://tbpl.mozilla.org/?tree=MozillaTry&rev=11232fc8933d

Comment 15

[Mike Connor [:mconnor]]

Comment on attachment 508630 [details] [diff] [review]
Part 1 (v1.0): Ensure resource exceptions contain status code

>-function ChannelListener(onComplete, onProgress, logger) {
>+function ChannelListener(onComplete, onProgress, logger, timeout) {
>   this._onComplete = onComplete;
>   this._onProgress = onProgress;
>   this._log = logger;
>+  this.ABORT_TIMEOUT = timeout;

style nit: I know there's probably code you want to avoid changing, but this.ABORT_TIMEOUT is really a var now, not a const, so the all caps is a little misleading.

Comment 16

[Philipp von Weitershausen [:philikon]]

Pushed with style nit addressed:

Part 1: https://hg.mozilla.org/services/fx-sync/rev/61a1553e98dd
Part 2: https://hg.mozilla.org/services/fx-sync/rev/5e0a93167fee