Closed
Bug 1401700
Opened 3 years ago
Closed 3 years ago
Firefox desktop keeps reopening tabs synced from Firefox Android
Categories
(Firefox :: Sync, defect, P2)
Tracking
()
RESOLVED
FIXED
Firefox 58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: andrew, Assigned: eoger)
Details
Attachments
(2 files)
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:57.0) Gecko/20100101 Firefox/57.0 Build ID: 20170920100426 Steps to reproduce: I sent three tabs from Firefox Android to my Firefox desktop. Actual results: All three tabs opened up, repeatedly every few minutes. Expected results: All three tabs should open only once.
Reporter | ||
Comment 1•3 years ago
|
||
Assignee | ||
Comment 2•3 years ago
|
||
This should be resolved now, thanks for the bug report.
Status: UNCONFIRMED → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1401686
Comment 3•3 years ago
|
||
While this was resolved by bug 1401686, I think we should keep it open and try to understand what kinds of errors can cause this behaviour and see what we can do to prevent them - we don't want transient errors to cause this either (eg, will 500 errors from the server due to operational issues also cause this?)
Status: RESOLVED → REOPENED
Ever confirmed: true
Resolution: DUPLICATE → ---
Updated•3 years ago
|
Priority: -- → P2
Updated•3 years ago
|
Assignee: nobody → eoger
Comment 4•3 years ago
|
||
I think this will happen whenever we fail to update the clients collection (after applying the tab) but succeed in fetching it. I think it would also happen if your clients collection got stuck due to having an oversized record. GET would succeed, but POST would always fail. I don't think we have a lot of protection in place for preventing oversized client records either, and sync will almost certainly be broken in a lot of ways. rnewman pointed out in irc that this is unlikely, but that it probably could happen (a small, fixed number of times) if we fail the XIUS check because there's a midair collision: <@rnewman> tcsc: but the replacement record should always be smaller than the one with the commands, no? <@rnewman> unless a client wanted to write a new junk client name <tcsc> yeah i think usually it should succeed <@rnewman> but it feels very edge-casey <@rnewman> yeah, there are failure cases around replacing the record on the server <tcsc> like there might be a case where they send a huge tab at exactly the right mid-sync moment also <@rnewman> e.g., receive tab, go to write the empty record, another client has uploaded a second tab <@rnewman> so the clear fails, and we download the same tab again <tcsc> oh, yeah, that too <@rnewman> everything about the Sync commands channel is sad-making
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•3 years ago
|
||
Comment on attachment 8914870 [details] Bug 1401700 - Prevent incoming tabs from opening multiple times if client sync fails. This feels more like a band-aid than a real long-lasting lasting solution.
Attachment #8914870 -
Flags: review?(markh) → feedback?(markh)
Comment 7•3 years ago
|
||
Comment on attachment 8914870 [details] Bug 1401700 - Prevent incoming tabs from opening multiple times if client sync fails. I suspect we want to do this for all commands and not just tabs. Without thinking too much about this, is there any scope for keeping the last-modified for the commands once we process them and use that as a "flag" to indicate the incoming record isn't new?
Attachment #8914870 -
Flags: feedback?(markh)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•3 years ago
|
||
> Without thinking too much about this, is there any scope for keeping the last-modified for the commands once we process them and use that as a "flag" to indicate the incoming record isn't new?
That's a good idea, what do you think of this?
Comment 10•3 years ago
|
||
mozreview-review |
Comment on attachment 8914870 [details] Bug 1401700 - Prevent incoming tabs from opening multiple times if client sync fails. https://reviewboard.mozilla.org/r/186134/#review191682 This looks good, thanks. It does seem like an edge-case, but should we store `lastModifiedOnProcessCommands` in a pref? Otherwise it seems we will still hit this if there is a browser restart in between? We should also have tests.
Attachment #8914870 -
Flags: review?(markh)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 12•3 years ago
|
||
Thanks!
Comment 13•3 years ago
|
||
mozreview-review |
Comment on attachment 8914870 [details] Bug 1401700 - Prevent incoming tabs from opening multiple times if client sync fails. https://reviewboard.mozilla.org/r/186134/#review192612 Looks great, but the test change concerns me... ::: services/sync/modules/engines/clients.js:53 (Diff revision 3) > const CLIENTS_TTL = 1814400; // 21 days > const CLIENTS_TTL_REFRESH = 604800; // 7 days > const STALE_CLIENT_REMOTE_AGE = 604800; // 7 days > > const SUPPORTED_PROTOCOL_VERSIONS = [SYNC_API_VERSION]; > +const LAST_MODIFIED_ON_PROCESS_COMMAND_PREF = "services.sync.clients._lastModifiedOnProcessCommands"; leading underscores are somewhat unusual on prefs - they are *all* internal ;) I don't really care if you prefer it this way though. ::: services/sync/tests/unit/test_clients_engine.js:1298 (Diff revision 3) > }), now - 10)); > > // Simulate reboot > SyncEngine.prototype._uploadOutgoing = oldUploadOutgoing; > engine = Service.clientsEngine = new ClientEngine(Service); > + Services.prefs.clearUserPref("services.sync.clients._lastModifiedOnProcessCommands"); Why is this change needed? A reboot is not going to reset that pref.
Attachment #8914870 -
Flags: review?(markh)
Comment hidden (mozreview-request) |
Comment 15•3 years ago
|
||
mozreview-review |
Comment on attachment 8914870 [details] Bug 1401700 - Prevent incoming tabs from opening multiple times if client sync fails. https://reviewboard.mozilla.org/r/186134/#review193670 As I mentioned in IRC, this still has a race condition where we'll end up applying records more times. This is unlikely though, so this is probably good enough. (We could avoid that by remembering which records we fail to mark as applied, and doing a deep compare against them -- but given that this is an edge case of an edge case, I don't think it's worth it). I've filed a followup bug 1407689, for getting an ID into the commands so that we can skip the deep compare in that case, and fix robustness issues.
Attachment #8914870 -
Flags: review?(tchiovoloni) → review+
Assignee | ||
Comment 16•3 years ago
|
||
mozreview-review |
Comment on attachment 8914870 [details] Bug 1401700 - Prevent incoming tabs from opening multiple times if client sync fails. https://reviewboard.mozilla.org/r/186134/#review193676
Attachment #8914870 -
Flags: review+
Comment 17•3 years ago
|
||
Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8d5cc47b248f Prevent incoming tabs from opening multiple times if client sync fails. r=tcsc
Backed out for failing xpcshell services/sync/tests/unit/test_bookmark_repair.js on OS X 10.10 opt: https://hg.mozilla.org/integration/autoland/rev/b9dd71f3f20f077171eddb8f20d1683701ef24e5 Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=8d5cc47b248f5b6c995fc836b076a3c0d1655313&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=136318316&repo=autoland 14:35:20 INFO - TEST-PASS | services/sync/tests/unit/test_bookmark_repair.js | test_bookmark_repair_integration - [test_bookmark_repair_integration : 80] [] deepEqual [] 14:35:20 INFO - PID 6560 | 1507757719535 Sync.Doctor INFO didn't start a repair of bookmarks with flowID fake-guid-08 14:35:20 INFO - PID 6560 | 1507757719536 Sync.Status DEBUG Status.sync: success.sync => success.sync 14:35:20 INFO - PID 6560 | 1507757719536 Sync.Status DEBUG Status.service: success.status_ok => success.status_ok 14:35:20 INFO - PID 6560 | 1507757719536 Sync.Synchronizer INFO Sync completed at 2017-10-11 14:35:19 after 0.29 secs. 14:35:20 INFO - PID 6560 | 1507757719536 Sync.Declined DEBUG Handling remote declined: [] 14:35:20 INFO - PID 6560 | 1507757719536 Sync.Declined DEBUG Handling local declined: [] 14:35:20 INFO - PID 6560 | 1507757719537 Sync.Declined DEBUG Declined changed? false 14:35:20 INFO - PID 6560 | 1507757719537 Sync.Service INFO No change to declined engines. Not reuploading meta/global. 14:35:20 INFO - PID 6560 | 1507757719538 Sync.SyncScheduler DEBUG Next sync in 90000 ms. 14:35:20 INFO - PID 6560 | 1507757719538 Sync.Telemetry TRACE observed weave:service:sync:finish 14:35:20 INFO - PID 6560 | 1507757719539 Sync.LogManager DEBUG Flushing file log 14:35:20 INFO - TEST-PASS | services/sync/tests/unit/test_bookmark_repair.js | test_bookmark_repair_integration - [test_bookmark_repair_integration : 278] Missing bookmark should be downloaded to initial client - {"guid":"_WeDUOg4kOOe","index":0,"type":1,"title":"Get Firefox!","dateAdded":"2017-10-11T21:35:17.856Z","lastModified":"2017-10- == true 14:35:20 WARNING - TEST-UNEXPECTED-FAIL | services/sync/tests/unit/test_bookmark_repair.js | test_bookmark_repair_integration - [test_bookmark_repair_integration : 51] [] deepEqual [{"object":"processcommand","method":"repairResponse","value":"undefined","extra":{"flowID":"fake-guid-04"}},{"object":"repair", 14:35:20 INFO - /Users/cltbld/tasks/task_1507756194/build/tests/xpcshell/tests/services/sync/tests/unit/test_bookmark_repair.js:checkRecordedEvents:51 14:35:20 INFO - /Users/cltbld/tasks/task_1507756194/build/tests/xpcshell/tests/services/sync/tests/unit/test_bookmark_repair.js:test_bookmark_repair_integration:279 14:35:20 INFO - exiting test
Flags: needinfo?(eoger)
Comment hidden (mozreview-request) |
Comment 20•3 years ago
|
||
Pushed by eoger@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/7b8f1bcaf50d Prevent incoming tabs from opening multiple times if client sync fails. r=tcsc
![]() |
||
Comment 21•3 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/7b8f1bcaf50d
Status: REOPENED → RESOLVED
Closed: 3 years ago → 3 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Assignee | ||
Updated•3 years ago
|
Flags: needinfo?(eoger)
You need to log in
before you can comment on or make changes to this bug.
Description
•