1319433 - Devices differ in Synced Tab and Send to Device menus

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Ryan Feeley [:rfeeley]]

Attachment: devices-mismatch.png

Unless the devices without open tabs are hidden (and I didn't think we were doing this), the two devices lists do not appear to be matching. These two lists should match the Sync devices in FxA/Settings.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

I can't work out how this could happen:

* "Send Page to Device" uses the clients engine's list of tabs, and excludes devices that haven't synced in a week. It seems strange that Ryan would have so many dupe devices that did sync within a week though.

* SyncedTabs uses the list of clients known to the "tabs" engine - ie, clients which have uploaded their tabs. In practice, this should be all devices. These tab records have a TTL of 7 days.

So somehow, Ryan has many devices that have synced in the last 7 days, but haven't uploaded tabs. Tab syncing must be enabled or (a) we wouldn't be showing the sidebar and (b) no devices would be syncing tabs.

So I can't reproduce it locally and nor can I explain it. I guess seeing what about-sync shows for both those collections might be interesting...

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Actually, one scenario is where a device has started and synced within the last 7 days, but hasn't actually opened any new tabs (or navigated to new pages in existing tabs) once Sync is initialized - for example, a new tab may have been opened within the first 10 seconds of the browser starting (so Sync missed it), or new tabs were only opened in private-browsing mode (in which case we will ignore it)

If this is actually the case here, I doubt we should treat it as a high priority as it seems unusual, but I guess the solution would be for SyncedTabs.jsm to use the list of clients in the clients-engine as canonical.

Comment 3

[Edouard Oger [:eoger]]

Attachment: Bug 1319433 - Bump tabs engine TTL to 21 days.

Review commit: https://reviewboard.mozilla.org/r/105896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105896/

Comment 4

[Edouard Oger [:eoger]]

I'll send a build to rfeeley to confirm this does not happen anymore (works on my machine).

Comment 5

[Edouard Oger [:eoger]]

Comment on attachment 8828540 [details]
Bug 1319433 - Bump tabs engine TTL to 21 days.

After thinking about it I think I should add some tests, clearing review flag for now.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

/me in IRC:

* I think this patch is suggesting there's a client record without a corresponding tabs record? I guess that must be when a client hasn't synced for 7 days, after which the tabs engine TTL causes the tab record to die, but the client to remain? Is there another scenario you can think of?

* If not, I'm wondering if we should consider having the tabs TTL match the clients TTL? Otherwise, and IIUC, the user will still see the client record shown, but without any tabs, which may still cause users to report a bug.

to which rnewman replied:

* user turned off tab sync? Even if the TTL is the same, remember that sending a tab to a client bumps its TTL. All kinds of ways you could get a mismatch here.

to which I replied:

* That's an excellent point - if a user *does* turn tab syncing off, I doubt they would expect the client to appear in the synced tabs list, even with no tabs.

and I didn't write in IRC but am writing here:

* Even though a user sending a tab to a device bumps its TTL, IMO using the same TTL would make this mismatch enough of an edge-case that we could probably just live with it - it's not as though we are doing anything "wrong" here, it's just that the user's expectations aren't being met - I think the same TTL would make it far less likely we wouldn't meet those expectations.

So as usual, things get murky quickly :(

Edouard, are you able to explain these situations to rfeeley and see what he suggests?

Comment 7

[Edouard Oger [:eoger]]

This is a good step forward towards having identical clients lists (minus the edge case), let's go for it.

Comment 8

[Edouard Oger [:eoger]]

Comment on attachment 8828540 [details]
Bug 1319433 - Bump tabs engine TTL to 21 days.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105896/diff/1-2/

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8828540 [details]
Bug 1319433 - Bump tabs engine TTL to 21 days.

You should also do this for Android (it's in m-c), and file a bug to do so on Android.

Comment 10

[Edouard Oger [:eoger]]

I also checked firefox-ios, but it seems we're good: https://github.com/mozilla-mobile/firefox-ios/blob/13186294f11b9bfc3279f94d7497ffcf6691a7a8/Sync/Synchronizers/TabsSynchronizer.swift#L46

Comment 11

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8828540 [details]
Bug 1319433 - Bump tabs engine TTL to 21 days.

https://reviewboard.mozilla.org/r/105896/#review108040

nice :)

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a151ee7bc049
Bump tabs engine TTL to 21 days. r=markh

Comment 13

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a151ee7bc049