1212431 - Make Firefox for iOS respond to LocalCommands coming out of the SyncStateMachine

Status: RESOLVED FIXED

(Firefox for iOS :: Sync, defect)

Description

[Nick Alexander :nalexander [he/him]]

Bug 1167010 surfaces LocalCommands out of the SyncStateMachine.  We need to take actions in response: resetting timestamps, wiping mirror collections, updating checkbox displays.

Comment 1

[Richard Newman [:rnewman]]

I'll work on this when Nick polishes and lands his state machine work.

Comment 2

[Richard Newman [:rnewman]]

> Huh, this surprises me. You're going to drop the mirror when you see a reset?
> That is different than other clients. You're saying that the mirror is "not
> us", but the Sync device constellation assumes all devices will eventually try
> to merge all the content they've ever seen back into the storage server.
> 
> I think this is clarified by considering three clients and explicit reset
> commands. Suppose client A uploads a complete collection, client B is iOS and
> healthily synced, and client C has not yet synced bookmarks. If client A is
> removed from the constellation, client C could send a reset command and expect
> B to upload what client A had previously provided. But B sees the reset and
> drops its mirror, so it can't update the storage server and client C is frozen
> out.
> 
> You're going to explain that the mirror is a scratchpad that gets merged into
> the local store, so that dropping the mirror only drops incomplete data. I'd
> like to understand that flow.

Indeed. Although for the moment we're exposing your server bookmarks in the UI, the mirroring strategy we have now is very much a half-way house: we have no facility for upload, merging, or anything else we'd need to do something sane with a locally persisted mirror.

The mirror is a replica of the server, nothing more. The correct behavior in almost any circumstance is to throw it away and start again: that way we'll always end up with an accurate snapshot of the server. (syncID and key changes are accompanied by server DELETEs, and of course we can't figure out what's been deleted, so we start from zero.)

Imagine that a user turns off bookmark sync on their desktop (which wipes server storage and uploads a new meta/global), reimports a backup, then turns it back on.

On iOS 1.1 we'd process the disablement as a reset, and reset again on enablement. Right now that throws away the mirror, and the new data will arrive when desktop turns bookmark sync back on.

But what if we keep the data across those transitions? We'd have a broken jumble of half-overwritten data in the mirror -- we wouldn't ever throw anything away, and the user can't delete it. Even worse, when we implement two-way sync we wouldn't be able to trust our mirror!

It would be accurate to say that we do not yet sync bookmarks in either direction, because we neither upload our own, nor merge downloaded data into real persistent storage.

The mirror will eventually turn into either a shared mirror like logins (which is flushed to local storage on reset, with content-based reconciling used to reconstruct a shared parent on reconnection), or a temporary scratchpad that feeds into a richer local structure. Right now it's just a mirror.

Comment 3

[Richard Newman [:rnewman]]

Attachment: Pull req.

Comment 4

[Richard Newman [:rnewman]]

Manual testing. Disabled passwords remotely; verified that loginsM was wiped and loginsL contained all passwords.

Re-enabled:

[Profile.swift:643] syncLoginsWithDelegate(_:prefs:ready:) > Syncing logins to storage.
[Synchronizer.swift:151] init(scratchpad:delegate:basePrefs:collection:) > Synchronizer configured with prefs 'profile.sync.synchronizer.passwords...'
[LoginsSynchronizer.swift:158] synchronizeLocalLogins(_:withServer:info:) > Synchronizing passwords. Last fetched: 0.
[StorageClient.swift:697] getSince(_:sort:limit:offset:) > Issuing GET with newer = 0.
[StorageClient.swift:328] failFromResponse > Status code: 200.
[LoginsSynchronizer.swift:163] synchronizeLocalLogins(_:withServer:info:) > Applying incoming password records from response timestamped 1444858265700, last modified 1444858255300.
[LoginsSynchronizer.swift:164] synchronizeLocalLogins(_:withServer:info:) > Records header hint: Optional(4)
[SQLiteLogins.swift:613] applyChangedLogin(_:local:mirror:) > Local record with GUID vytzVPlEY_vg but no mirror. This is unusual; assuming disconnect-reconnect scenario. Smushing.
[SQLiteLogins.swift:783] resolveConflictWithoutParentBetween(local:upstream:) > Conflicting records with no shared parent. Using newer remote record.
[SQLiteLogins.swift:613] applyChangedLogin(_:local:mirror:) > Local record with GUID u0FbeIHDbGif but no mirror. This is unusual; assuming disconnect-reconnect scenario. Smushing.
[SQLiteLogins.swift:783] resolveConflictWithoutParentBetween(local:upstream:) > Conflicting records with no shared parent. Using newer remote record.
[SQLiteLogins.swift:613] applyChangedLogin(_:local:mirror:) > Local record with GUID B7QQXwgfiFG5 but no mirror. This is unusual; assuming disconnect-reconnect scenario. Smushing.
[SQLiteLogins.swift:783] resolveConflictWithoutParentBetween(local:upstream:) > Conflicting records with no shared parent. Using newer remote record.
[SQLiteLogins.swift:613] applyChangedLogin(_:local:mirror:) > Local record with GUID 3p6dhcHfd2Tb but no mirror. This is unusual; assuming disconnect-reconnect scenario. Smushing.
[SQLiteLogins.swift:783] resolveConflictWithoutParentBetween(local:upstream:) > Conflicting records with no shared parent. Using newer remote record.
[IndependentRecordSynchronizer.swift:41] done() > Bumping fetch timestamp to 1444858255300.
[IndependentRecordSynchronizer.swift:73] uploadRecords(_:by:lastTimestamp:storageClient:onUpload:) > No modified records to upload.
[IndependentRecordSynchronizer.swift:73] uploadRecords(_:by:lastTimestamp:storageClient:onUpload:) > No modified records to upload.
[LoginsSynchronizer.swift:142] uploadOutgoingFromStorage(_:lastTimestamp:withServer:) > Done syncing.


Those log lines aren't as scary as they look: we hit this block:

        // Do the best we can. Either the local wins and will be
        // uploaded, or the remote wins and we delete our overlay.
        if local.timePasswordChanged > upstream.timePasswordChanged {
            log.debug("Conflicting records with no shared parent. Using newer local record.")
            return self.insertNewMirror(upstream, isOverridden: 1)
        }

and the timePasswordChanged values are the same, so we're simply taking the mirror value. If we'd locally changed the password and the other client re-uploaded, we'd take the local record.

Verified that after re-merge all logins are in loginsM and none are in loginsL, but values are unchanged.

Comment 5

[Richard Newman [:rnewman]]

r+ over IRC.

4155dea