1313967 - Modifying bookmarks during a sync can cause inconsistency

Status: RESOLVED DUPLICATE of bug 1359200

(Firefox :: Sync, defect, P3)

Description

[Lina Butler [:lina]]

`_modified` only contains the GUIDs of changed bookmarks...but we don't actually fetch the bookmarks from the database until we're in `_uploadOutgoing`. At that point, we call `_createRecord` for every bookmark in `_modified`. This leads to a race if the user moves the bookmark during the upload. Consider:

1. At the start of the sync, `_modified` contains A, B, and some other bookmarks. A is a new folder. B is a child of A.
2. We create a record for A. A.children == ["B"]
3. User moves B into C. We bump the change counter for A, B, and C.
4. We create a record for B. B.parentid = "C", but we won't upload C during this sync, because it's not in `_modified`. Oops.

Another variation: if (3) happens before (2), at least A's children will be correct...but B will still be orphaned.

With the new tracker, this isn't catastrophic, because we still bumped the change counter. (With the old tracker, we hopefully persisted the change to the JSON file). On the next sync, we'll upload A, B, and C again, and the orphaning logic can move B into the right folder.

And, if A already exists on other devices (rather than a new folder), we'll do the right thing anyway. Though A's children will still be wrong on the server until the next sync.

Comment 1

[Lina Butler [:lina]]

Mulling over two-phase application, I realized we can use the same schema to buffer outgoing bookmarks, too. We can apply, reconcile, build an outgoing changeset, and insert it into the outgoing buffer in a transaction. Our uploader would then read from that outgoing buffer, not from Places directly. We might upload stale state, but at least it'll be consistent.

Open questions: are there things we can do to mitigate this, and is it worth fixing in the interim?

Comment 2

[Lina Butler [:lina]]

The simplest thing to do is fetch all bookmark records from Places and store them in memory at the start of the sync, of course. That seems like it would be bad for the first sync, though: if you have 5k bookmarks, that's 5k heavyweight JS objects that need to be retained for the entire sync, and GCed at the end.

Then again, maybe 5k isn't much next to the entire Firefox front-end. We'd definitely want to profile this.

Comment 3

[Lina Butler [:lina]]

Attachment: Bug 1313967 - Buffer outgoing Sync records in memory.

The current design of `Engine::_uploadOutgoing` makes it hard to upload
records that aren't in `Engine::_modified`. This commit extracts the
`_createRecord` calls into a `fetchOutgoingRecords` method that returns
an array of encrypted records for upload. Engines can override this
method to include additional records.

This is used in bug 1303679 to add a cleanup stage that writes
tombstones for records that shouldn't be on the server.

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

Comment 4

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

Comment on attachment 8806573 [details]
Bug 1313967 - Buffer outgoing Sync records in memory.

https://reviewboard.mozilla.org/r/89964/#review89486

I'm fine with this if is necesary for bug 1303679, although it will increase the memory usage of Sync, and IIUC doesn't really solve the problem, but instead makes the window a little smaller (ie, given all the yielding we do, ISTM a user might still manage to modify the bookmark as we are looping over the list). Is there any scope for somehow enforcing consistency while we are looping (eg, using a transaction)? I think the memory  is probably not a real issue in practice (although it is something we should be wary of) so yeah, go for it!

Comment 5

[Lina Butler [:lina]]

Thanks, Mark! I'm going to hold off on landing until we figure out bug 1303679. A fix that's not needed is worse than no fix at all. You're right; it just makes the window smaller; we still end up fetching the bookmarks separately. I think there is scope for fetching everything in a single statement, to close that window.

Comment 6

[Lina Butler [:lina]]

Comment on attachment 8806573 [details]
Bug 1313967 - Buffer outgoing Sync records in memory.

Obsoleting this because I think we're going to go with a different approach for bug 1303679. Let's either wait for two-phase, or read everything in a single query.