1296714 - PlacesSyncUtils methods aren't atomic

Status: RESOLVED WONTFIX

(Firefox :: Sync, defect)

Description

[Lina Butler [:lina]]

The methods in PlacesSyncUtils are meant to be atomic, but aren't necessarily so. For example, `insert` and `update` call several public methods, all of which can throw.

It's possible that an uncaught exception will leave a bookmark partially applied. On the one hand, maybe that's not so bad; we'll mark it as failed to apply and try again. On the other, I'm not sure what would happen if the user tried to fix this, and Sync "helpfully" reconciled their changes.

Another concern is shutdown. `PlacesUtils.withConnectionWrapper` blocks shutdown until the connection is finished, but it's possible for us to shut down in between two such calls:

  yield PlacesUtils.bookmarks.insert({ source: SOURCE_SYNC, ... });
  // --- Shutdown begins here ---
  yield insertBookmarkMetadata(newId, item, insertInfo);
  // Oops, now insertBookmarkMetadata fails, and we have a partially applied record.

Comment 1

[Lina Butler [:lina]]

On further reflection, this doesn't seem actionable.

ISTM we only fast-forward `lastSync` once we've processed all records (though it's not clear to me what "process any backlog of GUIDs" is doing: http://searchfox.org/mozilla-central/rev/a2812fa126be538f73efed589e78d6973f23df2f/services/sync/modules/engines.js#1183-1221), so we'll end up downloading the records again on the next sync. At that point, they'll already exist in the database, so the engine will call `PlacesSyncUtils.bookmarks.update` and add missing fields.

There will be some data loss if the user updates the partially applied bookmark before Sync runs again...but, short of rewriting bookmarks to use download-then-merge a la iOS, that's inevitable in some circumstances.

Also, I don't think the shutdown blocker will do much. Making `PlacesSyncUtils` truly transactional would require duplicating the `INSERT/UPDATE` queries for bookmarks, annos, and tags, and wrapping them up in a `db.executeTransaction` call. That extra complexity doesn't seem worth the trouble.

Mark, does this sound right to you?

Comment 2

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

(In reply to Kit Cambridge [:kitcambridge] from comment #1)
> Mark, does this sound right to you?

Indeed it does, thanks!