600261 - don't spam user with notifications when there are network problems

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Every time I unsuspend my laptop, I'm greeted with an infobar in Firefox telling me: "Sync encountered an error while syncing: Unknown error.  Sync will automatically retry this action."  I don't really need to know this, and especially not the first time it happens, because it usually happens while my laptop is connecting to a wireless network.  It also often happens if there's a captive portal on the internet connection (e.g. hotel).

Suggestion: only notify the user if N retries in a row fail (perhaps N=5, with a few minutes of interval in between retries), and try to detect captive portals and ignore sync.

Comment 1

[Mike Connor [:mconnor]]

We need to revisit the timing for notifications.  We should show this once per outage for automatic syncs + show a notification on manual syncs.

Comment 2

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Once per outage is good, but not immediately when it's detected -- because that'll be the "my laptop just unsuspended and is trying to connect to the network" outage.  You still need to wait for a few retries, I think.

Comment 3

[Philipp von Weitershausen [:philikon]]

(In reply to comment #2)
> "my laptop just unsuspended and is trying to connect to the network"

That's bug 591102.

Comment 4

[LegNeato]

This doesn't look to meet the standard of a beta8 blocker. Punting to betaN, feel free to disagree and/or triage to a different beta.

Comment 5

[Mike Connor [:mconnor]]

Need to decide:

* how long can we ignore errors for before we notify the user?
* if we're "offline" do we trigger a sync when network status changes?

Comment 7

[LegNeato]

As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)

Comment 8

[LegNeato]

Fixing fields my automated script accidentally blanked. Apologies for the bugspam

Comment 9

[juan becerra [:juanb]]

QA would like this to be a hardblocker. In a particular case, I see this error all the time. It's become a permanent fixture of my browser, like a status bar, and if it has the potential to be around for a long time, because users are offline, then we should at least describe in the message what the problem is and be able to dismiss it.

Comment 11

[Philipp von Weitershausen [:philikon]]

I disagree. This doesn't inhibit the functionality of Sync at all, the error notifications are just annoying but don't make us fail hard in any way.

Comment 12

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

They are *incredibly* annoying, not "just annoying", because they pop up every time sync tries to sync.  Not to mention popping up basically every time my laptop unsuspends, because the network isn't up yet when it tries to sync.

Comment 13

[[not reading bugmail]]

So can't we tell Sync to wait till there is a network connection?  It should be smart enough to know the network is down.

Comment 14

[Philipp von Weitershausen [:philikon]]

(In reply to comment #13)
> So can't we tell Sync to wait till there is a network connection?  It should be
> smart enough to know the network is down.

That's the plan!

Comment 15

[[e-mail]]

Yoink.

I have the following proposal: the only time we notify the user (via the current system) of a sync failure is when their username/password or sync key are wrong. (And possibly a few other things we could add to this list?) In other words, we specifically exclude network connectivity or other low-level network errors.

If the client network connectivity is borked or they're behind a captive portal, I'd argue that the user really doesn't care if they're syncing or not, because regardless they can't access the network to use their fancy bookmarks or history, and they'd obviously expect that sync would fail if they know the network's down.

Now, philikon points out two potential arguments against my proposal:
1) The user has their own sync server set up and it's borked, unreachable etc. and they want to know.
2) Connection is generally OK but the ISP is blocking the connection to the sync server (e.g. https in general).

If we care about those, I'd argue we can just put a "last synced at..." indicator in Prefs/Sync somewhere. Apparently we had that, but took it out. I can't figure out why. Giving the curious user some tidbit of information rather than expecting them to wait for an OMGFAILED message that they may or may not care about sounds like a good tradeoff to me!

Comment 16

[Philipp von Weitershausen [:philikon]]

(In reply to comment #15)
> I have the following proposal: the only time we notify the user (via the
> current system) of a sync failure is when their username/password or sync key
> are wrong. (And possibly a few other things we could add to this list?) In
> other words, we specifically exclude network connectivity or other low-level
> network errors.

Yep. For that to work we need to fix bug 591102 / bug 622938 because some network errors (e.g. channel aborted due to inactivity) are currently misrepresented as Unknown Errors. For that we probably want to amend Service._checkServerError to set Status.service = LOGIN_FAILED_NETWORK_ERROR. The name of the constant sounds a bit stupid, but we don't have another one for network failures and this one gets us the right user facing message with l10n and all.

> If the client network connectivity is borked or they're behind a captive
> portal, I'd argue that the user really doesn't care if they're syncing or not,
> because regardless they can't access the network to use their fancy bookmarks
> or history, and they'd obviously expect that sync would fail if they know the
> network's down.

I agree!

> Now, philikon points out two potential arguments against my proposal:
> 1) The user has their own sync server set up and it's borked, unreachable etc.
> and they want to know.
> 2) Connection is generally OK but the ISP is blocking the connection to the
> sync server (e.g. https in general).
> 
> If we care about those, I'd argue we can just put a "last synced at..."
> indicator in Prefs/Sync somewhere. Apparently we had that, but took it out. I
> can't figure out why. Giving the curious user some tidbit of information rather
> than expecting them to wait for an OMGFAILED message that they may or may not
> care about sounds like a good tradeoff to me!

IMHO it doesn't make a lot of sense to change the UI this late in the game. If we're lucky we might get all the UI *bugs* fixed in time, not to mention additions like the one you're proposing ;)

Our idea was to surface a notification after a certain period of time a la "Hey, you haven't been able to Sync in a while. Everything ok?". Time Machine does this, for instance. This would require no UI changes, only additional notifications.

Comment 17

[Philipp von Weitershausen [:philikon]]

YOINK

Comment 18

[Philipp von Weitershausen [:philikon]]

My plan of attack is this:

The desired behaviour is that we don't *spam* the user with network errors, but we should still surface something when Sync hasn't been able to sync for a while (kinda like Time Machine on the Mac). That while could be determined based on the user's current sync interval (e.g. 10 times that interval which currently ranges from 5 minutes to 1 day) or a fixed constant interval (e.g. a day). Note: we can't add new strings at this point, so we can't exactly do the Time Machine thing where we ask "You haven't synced for a while... everything ok with you?". We'll just have to surface the network error for now.

tl;dr: ignore network errors when the last sync was less than a certain interval ago.

Comment 19

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #18)
> tl;dr: ignore network errors when the last sync was less than a certain
> interval ago.

remember to take into account possible laptop-suspended time in this; you probably need to track last sync time as well as number of sync attempts since that time and check sync attempts if the time is out of date.  (e.g. close laptop in the morning, come back home in the evening and open laptop -- should get no network warning).

As a bonus, should ignore sync errors while offline, and for N minutes after an offline->online transition.  (I think we get those events before the network interface is actually ready -- that is, we get them when the network interface comes up, but perhaps before we get a real IP address.)

Comment 20

[Philipp von Weitershausen [:philikon]]

(In reply to comment #19)
> (In reply to comment #18)
> > tl;dr: ignore network errors when the last sync was less than a certain
> > interval ago.
> 
> remember to take into account possible laptop-suspended time in this; you
> probably need to track last sync time as well as number of sync attempts since
> that time and check sync attempts if the time is out of date.  (e.g. close
> laptop in the morning, come back home in the evening and open laptop -- should
> get no network warning).

Yeah. I can't really use time here, need to count failed network attempts.

> As a bonus, should ignore sync errors while offline, and for N minutes after an
> offline->online transition.  (I think we get those events before the network
> interface is actually ready -- that is, we get them when the network interface
> comes up, but perhaps before we get a real IP address.)

We already don't schedule new syncs when nsIIOService is offline. However, as per bug 620472 Firefox will no longer be managing online/offline automatically by default, so I don't think this case is that important anymore.

Comment 21

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1: Hooks for ignoring network errors

This increments an integer pref, services.sync.ignorableErrorCount, each time an ignorable error is encountered. Right this is limited to network errors during login and sync. A Service.shouldIgnoreError() method is exposed to UI code to query whether the just observed sync error can be ignored or not (up to 10 network errors are ignored.) Part 2 will make the Firefox UI use this method.

I'm not particularly proud of this solution, but it seemed like the only feasible one in the whole _catch/_notify/_lock + notifications mess that services.js is. This stuff needs to get rewritten badly.

This patch also makes sure that we set the rights bits when in offline mode.

Comment 22

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2: UI changes (m-c)

Call Weave.Service.shouldIgnoreError() to find out whether an observed error can be ignored or not.

Also kills a few superfluous _updateLastSyncTime() calls. It is already called by updateUI().

Comment 23

[Philipp von Weitershausen [:philikon]]

Try build: http://tbpl.mozilla.org/?tree=MozillaTry&rev=4ccc85c16c6e

Comment 24

[Philipp von Weitershausen [:philikon]]

Sorry, didn't mean to RESOLVE FIX this. Driver asleep at the wheel...

Comment 25

[Benjamin Stover (:stechz)]

I was really excited for a second :)

Comment 26

[Philipp von Weitershausen [:philikon]]

(In reply to comment #25)
> I was really excited for a second :)

My fingers seem to associate pasting a URL into a bug with RESOLVE FIX :)

Comment 27

[Mike Connor [:mconnor]]

Comment on attachment 508993 [details] [diff] [review]
Part 1: Hooks for ignoring network errors

Notes from IRC discussion:

* this doesn't need to persist across sessions.
* given that, maybe make the threshold 5?
* use else if in the sync:error case (or for future-proofing, make it a switch statement) and put the network error case first.

Also:

* only increment the counter if we're online.  we shouldn't get to that point, so let's not have bad codepaths impact users.

Comment 28

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v2): Hooks for ignoring network errors

Addresses mconnor's review comments.

Comment 29

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v2.1): Hooks for ignoring network errors

Bah, v2 had a test failure in a test that's disabled on OSX. Tiny fix.

Comment 30

[Philipp von Weitershausen [:philikon]]

Landed part 1 in fx-sync: https://hg.mozilla.org/services/fx-sync/rev/5ac42eb61c13

Comment 31

[Philipp von Weitershausen [:philikon]]

Landed part in places:

Part 1: http://hg.mozilla.org/projects/places/rev/a073422b2246
Part 2: https://hg.mozilla.org/projects/places/rev/ddd847f6fe89

Comment 32

[Philipp von Weitershausen [:philikon]]

Landed on m-c:

Part 1: http://hg.mozilla.org/mozilla-central/rev/a073422b2246
Part 2: http://hg.mozilla.org/mozilla-central/rev/ddd847f6fe89