Closed
Bug 1453887
Opened 6 years ago
Closed 6 years ago
Sync attempts to sync as the network link goes down
Categories
(Firefox :: Sync, defect)
Firefox
Sync
Tracking
()
RESOLVED
FIXED
Firefox 61
People
(Reporter: markh, Assigned: tcsc)
References
Details
(Keywords: regression)
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
eoger
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
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
Assignee | ||
Comment 1•6 years ago
|
||
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 | ||
Updated•6 years ago
|
Assignee: nobody → tchiovoloni
Comment hidden (mozreview-request) |
Comment 3•6 years ago
|
||
mozreview-review |
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
Comment 5•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/d10dc4dc4162
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Assignee | ||
Comment 6•6 years ago
|
||
Needinfoing myself to follow up and check if this does indeed resolve the issue.
Flags: needinfo?(tchiovoloni)
Reporter | ||
Comment 7•6 years ago
|
||
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 :(
Assignee | ||
Comment 8•6 years ago
|
||
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.
Assignee | ||
Comment 9•6 years ago
|
||
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)
Assignee | ||
Comment 10•6 years ago
|
||
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 11•6 years ago
|
||
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+
Comment 12•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/dad20eed22b9
Updated•6 years ago
|
Flags: in-testsuite+
Updated•6 years ago
|
Keywords: regression
You need to log in
before you can comment on or make changes to this bug.
Description
•