1058298 - Ensure that the local tabs database contains only tabs for active remote clients

Status: RESOLVED INCOMPLETE

(Firefox for Android Graveyard :: Android Sync, defect, P3)

Description

[Richard Newman [:rnewman]]

We clean up client records implicitly, by always wiping and re-downloading.

We don't do that for remote tabs. If a device goes AWOL, its tab and client records will be TTLed, and quietly disappear from the server.

Your remaining clients will never download a deleted record for the AWOL device, and we never ordinarily wipe the tabs DB, so those tabs will stick around in the local DB forever.

We should clean this up.

Probably the easiest thing to do is to, at the end of each sync, ask the server for the IDs of records in the collection, and then delete from the DB any records with a non-null GUID that isn't in that set.

We'd do that in FennecTabsRepositorySession.

Comment 1

[Dirkjan Ochtman (:djc)]

I looked at FennecTabsRepositorySession and FirefoxAccounts, but it's not quite clear to me where "at the end of each sync" would live. Also, are there any good examples of interactions with the server/DB cleanup that I could crib from?

Comment 2

[Richard Newman [:rnewman]]

Let me think about this a little more. The repository session doesn't have a direct line to the server, so this needs to be handled in an enclosing scope.

Comment 3

[Richard Newman [:rnewman]]

This bug is, at its root, a philosophical one.

Sync operates under the open world assumption: a server record exists ('yes', 'apply', 'preserve'), exists as a deleted tombstone ('no', 'delete'), or isn't known ('reupload', 'ignore'). The absence of a record on the server is not an assertion that an entity is deleted.

This is true for all non-client-linked data types: bookmarks, history, etc.

Client-linked data types -- right now that's clients and tabs -- are closed-world: absence means 'no', not 'maybe', even if only for a while.

For example, you can't (mustn't) send a command to a client that doesn't have a record on the server, even if you have an old copy of its client record and could upload one.

If there's no tabs record for a particular client on the server, we assume that the client went away, and we should locally delete or hide the data we have for that client, rather than showing the tabs they uploaded two weeks ago.

(So far these data types have been modeled as collections where the key is a client ID, and the object is a canonical blob.)


The open-world default is why Sync doesn't have a channel for a client to treat TTLed records as deleted -- because in that model that are not deleted -- and for obvious reasons this interacts poorly with the closed-world data types.


I think what we need here to close the gap is the notion of a synthetic deletion. The synchronizer or stage should detect when a closed-world record has disappeared from the server without the presence of a server wipe -- i.e., via an incremental DELETE or TTL expiry -- and treat it as an explicitly deleted open-world record, notifying the local repository session accordingly.

It can only do this by having local bookkeeping for the IDs that are being kept locally, and fetching a list of IDs from the server at the start of a sync.

Comment 4

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #3)
> This bug is, at its root, a philosophical one.

This analysis is correct, but possibly at a higher level than is needed to address this ticket in the short term.

In TabsProvider, we JOIN the tabs and clients to expose client information to each tab.  (This is perhaps not optimal, but won't change for at least a little while.)  I thought the join was such that if the corresponding client didn't exist, we'd drop the tab record entirely.  Perhaps I am wrong.  But this would be a low tech way to filter tab records whose corresponding client record has been deleted.

Comment 5

[Richard Newman [:rnewman]]

Yup, we can take a display-filtering approach (and we probably should, at least only to improve the UX). We should still delete all those old tab rows, though.

I think there's an elegant way to make this work simply by inserting logic at just the right place, for which we can reserve this bug.

Comment 6

[Nick Alexander :nalexander [he/him]]

We talked, but perhaps not on this bug, about the fact that the clients engine was supposed to wipe the set of clients.  I think it wipes *Sync's* clients DB, not *Fennec's*.  In fact, there's a subtle bug hidden in Fennec's TabsProvider: you can't do anything by CLIENT_GUID on the clients table.  That was part of the motivation for Bug 1025128.

Comment 8

[Michael Comella (:mcomella) [needinfo or I won't see it]]

https://mail.mozilla.org/pipermail/mobile-firefox-dev/2015-February/001090.html

Comment 9

[BMO Automation]

We have completed our launch of our new Firefox on Android. The development of the new versions use GitHub for issue tracking. If the bug report still reproduces in a current version of [Firefox on Android nightly](https://play.google.com/store/apps/details?id=org.mozilla.fenix) an issue can be reported at the [Fenix GitHub project](https://github.com/mozilla-mobile/fenix/). If you want to discuss your report please use [Mozilla's chat](https://wiki.mozilla.org/Matrix#Connect_to_Matrix) server https://chat.mozilla.org and join the [#fenix](https://chat.mozilla.org/#/room/#fenix:mozilla.org) channel.