Closed Bug 1453887 Opened 2 years ago Closed 2 years ago

Sync attempts to sync as the network link goes down

Categories

(Firefox :: Sync, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox59 --- wontfix
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: markh, Assigned: tcsc)

References

Details

(Keywords: regression)

Attachments

(1 file)

Thom noticed a spike in sync errors, which appear to be mostly network related. After running a few notebooks [1] we came up with the following theory:

* At [2] we observe "network:link-status-changed", and if this.offline is false we sync now.
* In bug 1420802 we changed the offline getter to ignore the link status.
* End result is that we try to sync on notifications of the link going *down*.

The fix appears easy - check .offline *and* NetworkLinkService.isLinkUp before syncing (although we should also consider if the |this.shouldSyncWhenLinkComesUp| logic removed by that bug has value.)


[1] https://gist.github.com/mhammond/92f8ff67ecc13e1d4bd7590336f13619
[2] https://searchfox.org/mozilla-central/rev/9f3da81290054c5b8955bb67ff98cae66676f745/services/sync/modules/policies.js#204
This graph is sufficiently convincing that this is the cause of the errors: https://sql.telemetry.mozilla.org/queries/52529/source#139886

I don't really see why we'd want `this.shouldSyncWhenLinkComesUp`. Did that logic ever land anyway?
Assignee: nobody → tchiovoloni
Comment on attachment 8967841 [details]
Bug 1453887 - Avoid syncing as the network link changes to down

https://reviewboard.mozilla.org/r/236550/#review242378

Looks good, thanks!
Attachment #8967841 - Flags: review?(eoger) → review+
Pushed by tchiovoloni@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d10dc4dc4162
Avoid syncing as the network link changes to down r=eoger
https://hg.mozilla.org/mozilla-central/rev/d10dc4dc4162
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Needinfoing myself to follow up and check if this does indeed resolve the issue.
Flags: needinfo?(tchiovoloni)
Please also request uplift to beta after you have verified things and are confident that nothing has been made worse here.

Also, FWIW, I think that fallthrough was intended, but now that .offline doesn't take the link status into account, I think the patch is probably correct. The scheduler is a mess :(
Yeah I'll request uplift after confirming it fixes it. I think the fallthrough only made sense if there was a break after scheduleNextSync, since otherwise we'll sync twice in the cases we call that.
https://sql.telemetry.mozilla.org/queries/52561#140010 indicates that builds after this landed have substantially fewer unknown_host errors. We still expect some, since, well, networks ¯\_(ツ)_/¯.
Flags: needinfo?(tchiovoloni)
Comment on attachment 8967841 [details]
Bug 1453887 - Avoid syncing as the network link changes to down

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1420802
[User impact if declined]: Many attempted syncs in cases where sync will certainly fail (we just got a notification that the network went down).
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes, as well as via sync error telemetry.
[Needs manual test from QE? If yes, steps to reproduce]: No, verified via automated tests, and we have telemetry saying this fixes the issue. (It wouldn't be hard to test manually, but doesn't seem necessary).
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: Low risk.
[Why is the change risky/not risky?]: This part of the sync scheduler is completely optional for sync working correctly (it's an opportunistic sync when the network comes online), and we'll be syncing strictly less often this way anyway.
[String changes made/needed]: N/A
Attachment #8967841 - Flags: approval-mozilla-beta?
Comment on attachment 8967841 [details]
Bug 1453887 - Avoid syncing as the network link changes to down

avoid useless sync on link down, approved for 60.0b14
Attachment #8967841 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.