Closed Bug 600261 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: vlad, Assigned: philikon)

References

Details

(Whiteboard: [captive portal][softblocker])

Attachments

(2 files, 2 obsolete files)

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.
We need to revisit the timing for notifications.  We should show this once per outage for automatic syncs + show a notification on manual syncs.
blocking2.0: ? → beta8+
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.
(In reply to comment #2)
> "my laptop just unsuspended and is trying to connect to the network"

That's bug 591102.
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.
blocking2.0: beta8+ → betaN+
blocking2.0: betaN+ → beta9+
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?
See Also: → 562917
Whiteboard: [captive portal]
Duplicate of this bug: 622938
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)
blocking2.0: beta9+ → betaN+
See Also: 562917
Fixing fields my automated script accidentally blanked. Apologies for the bugspam
See Also: → 562917
Whiteboard: [captive portal] → [captive portal][softblocker]
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.
Whiteboard: [captive portal][softblocker] → [captive portal]
Duplicate of this bug: 625382
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.
Whiteboard: [captive portal] → [captive portal][softblocker]
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.
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.
(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!
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!
Assignee: nobody → dwitte
(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.
Depends on: 591102
YOINK
Assignee: dwitte → philipp
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.
Status: NEW → ASSIGNED
(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.)
(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.
Status: ASSIGNED → NEW
Status: NEW → ASSIGNED
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.
Attachment #508993 - Flags: review?(mconnor)
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().
Attachment #508994 - Flags: review?(mconnor)
Whiteboard: [captive portal][softblocker] → [captive portal][softblocker][has patch][needs review mconnor]
Try build: http://tbpl.mozilla.org/?tree=MozillaTry&rev=4ccc85c16c6e
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Sorry, didn't mean to RESOLVE FIX this. Driver asleep at the wheel...
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Status: REOPENED → ASSIGNED
I was really excited for a second :)
(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 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.
Attachment #508993 - Flags: review?(mconnor) → review+
Attachment #508994 - Flags: review?(mconnor) → review+
Addresses mconnor's review comments.
Attachment #508993 - Attachment is obsolete: true
Whiteboard: [captive portal][softblocker][has patch][needs review mconnor] → [captive portal][softblocker][has patch][has review]
Bah, v2 had a test failure in a test that's disabled on OSX. Tiny fix.
Attachment #509454 - Attachment is obsolete: true
Landed part in places:

Part 1: http://hg.mozilla.org/projects/places/rev/a073422b2246
Part 2: https://hg.mozilla.org/projects/places/rev/ddd847f6fe89
Whiteboard: [captive portal][softblocker][has patch][has review] → [captive portal][fixed in places]
Whiteboard: [captive portal][fixed in places] → [captive portal][fixed in places][softblocker]
Landed on m-c:

Part 1: http://hg.mozilla.org/mozilla-central/rev/a073422b2246
Part 2: http://hg.mozilla.org/mozilla-central/rev/ddd847f6fe89
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Whiteboard: [captive portal][fixed in places][softblocker] → [captive portal][softblocker]
Blocks: 684160
No longer blocks: 615950
Component: Firefox Sync: UI → Sync
Product: Cloud Services → Firefox
You need to log in before you can comment on or make changes to this bug.