Closed Bug 1401700 Opened 2 years ago Closed 2 years ago

Firefox desktop keeps reopening tabs synced from Firefox Android

Categories

(Firefox :: Sync, defect, P2)

Product:
Firefox
Component:
Sync
Version:
57 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox58 --- fixed

People

(Reporter: andrew, Assigned: eoger)

Details

Attachments

(2 files)

same set of three tabs reopening
478.95 KB, image/png
Details
Bug 1401700 - Prevent incoming tabs from opening multiple times if client sync fails.
59 bytes, text/x-review-board-request
tcsc
: review+
Details 
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.
Component: Untriaged → Sync
This should be resolved now, thanks for the bug report.
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1401686
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 → ---
Priority: -- → P2
Assignee: nobody → eoger
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 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 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)
> 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 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)
Thanks!
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 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+
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+
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)
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
https://hg.mozilla.org/mozilla-central/rev/7b8f1bcaf50d
Status: REOPENED → RESOLVED
Closed: 2 years ago → 2 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Flags: needinfo?(eoger)
You need to log in before you can comment on or make changes to this bug.