--- a/toolkit/components/places/SyncedBookmarksMirror.jsm
+++ b/toolkit/components/places/SyncedBookmarksMirror.jsm
@@ -573,25 +573,23 @@ class SyncedBookmarksMirror {
"Merged tree doesn't mention all items from local tree");
}
if (!await merger.subsumes(remoteTree)) {
throw new SyncedBookmarksMirror.ConsistencyError(
"Merged tree doesn't mention all items from remote tree");
}
MirrorLog.debug("Applying merged tree");
- let localDeletions = Array.from(merger.deleteLocally).map(guid => {
- let localNode = localTree.nodeForGuid(guid);
- return { guid, level: localNode ? localNode.level : -1 };
- });
- let remoteDeletions = Array.from(merger.deleteRemotely);
+ let deletions = [];
+ for await (let deletion of yieldingIterator(merger.deletions())) {
+ deletions.push(deletion);
+ }
let { time: updateTiming } = await withTiming(
"Apply merged tree",
- () => this.updateLocalItemsInPlaces(mergedRoot, localDeletions,
- remoteDeletions)
+ () => this.updateLocalItemsInPlaces(mergedRoot, deletions)
);
applyStats.update = { time: updateTiming };
// At this point, the database is consistent, and we can fetch info to
// pass to observers. Note that we can't fetch observer info in the
// triggers above, because the structure might not be complete yet. An
// incomplete structure might cause us to miss or record wrong parents and
// positions.
@@ -1292,23 +1290,21 @@ class SyncedBookmarksMirror {
* implementation, though we lose idempotent merges. If upload is interrupted,
* the next sync won't distinguish between new merge states from the previous
* sync, and local changes. Since this is how Desktop behaved before
* structured application, that's OK. In the future, we can make this more
* like iOS.
*
* @param {MergedBookmarkNode} mergedRoot
* The root of the merged bookmark tree.
- * @param {Object[]} localDeletions
- * `{ guid, level }` tuples for items to remove from Places and flag as
- * merged.
- * @param {String[]} remoteDeletions
- * Remotely deleted GUIDs that should be flagged as merged.
+ * @param {Object[]} deletions
+ * `{ guid, localLevel, shouldUploadTombstone }` tuples for items to
+ * remove from Places, write tombstones, and flag as merged.
*/
- async updateLocalItemsInPlaces(mergedRoot, localDeletions, remoteDeletions) {
+ async updateLocalItemsInPlaces(mergedRoot, deletions) {
MirrorLog.trace("Setting up merge states table");
let mergeStatesParams = [];
for await (let param of yieldingIterator(mergedRoot.mergeStatesParams())) {
mergeStatesParams.push(param);
}
if (mergeStatesParams.length) {
await this.db.execute(`
INSERT INTO mergeStates(localGuid, mergedGuid, parentGuid, level,
@@ -1330,97 +1326,47 @@ class SyncedBookmarksMirror {
FROM items v
JOIN urls u ON u.id = v.urlId
JOIN mergeStates r ON r.mergedGuid = v.guid
WHERE r.valueState = :valueState`,
{ queryKind: SyncedBookmarksMirror.KIND.QUERY,
valueState: BookmarkMergeState.TYPE.REMOTE });
await this.db.execute(`DELETE FROM moz_updatehostsinsert_temp`);
+ MirrorLog.trace("Setting up deletions table");
+ for (let chunk of PlacesSyncUtils.chunkArray(deletions,
+ SQLITE_MAX_VARIABLE_NUMBER)) {
+
+ // This fires the `noteItemRemoved` trigger, which records observer infos
+ // for deletions. It's important we do this before updating the structure,
+ // so that the trigger captures the old parent and position.
+ await this.db.execute(`
+ INSERT INTO itemsToRemove(guid, localLevel, shouldUploadTombstone)
+ VALUES ${chunk.map(({ localLevel, shouldUploadTombstone }) =>
+ `(?, ${localLevel}, ${shouldUploadTombstone})`
+ ).join(",")}`,
+ chunk.map(({ guid }) => guid));
+ }
+
// Deleting from `itemsToMerge` fires the `insertNewLocalItems` and
// `updateExistingLocalItems` triggers.
MirrorLog.trace("Updating value states for local bookmarks");
await this.db.execute(`DELETE FROM itemsToMerge`);
// Update the structure. The mirror stores structure info in a separate
// table, like iOS, while Places stores structure info on children. We don't
// check the parent's merge state here because our merged tree might
// diverge from the server if we're missing children, or moved children
// without parents to "unfiled". In that case, we *don't* want to reupload
// the new local structure to the server.
MirrorLog.trace("Updating structure states for local bookmarks");
await this.db.execute(`DELETE FROM structureToMerge`);
MirrorLog.trace("Removing remotely deleted items from Places");
- for (let chunk of PlacesSyncUtils.chunkArray(localDeletions,
- SQLITE_MAX_VARIABLE_NUMBER)) {
-
- let guids = chunk.map(({ guid }) => guid);
-
- // Record item removed notifications.
- await this.db.execute(`
- WITH
- guidsWithLevelsToDelete(guid, level) AS (
- VALUES ${chunk.map(({ level }) => `(?, ${level})`).join(",")}
- )
- INSERT INTO itemsRemoved(itemId, parentId, position, type, placeId,
- guid, parentGuid, level)
- SELECT b.id, b.parent, b.position, b.type, b.fk, b.guid, p.guid,
- o.level
- FROM moz_bookmarks b
- JOIN moz_bookmarks p ON p.id = b.parent
- JOIN guidsWithLevelsToDelete o ON o.guid = b.guid`,
- guids);
-
- // Recalculate frecencies. The `isUntagging` check is a formality, since
- // tags shouldn't have annos or tombstones, should not appear in local
- // deletions, and should not affect frecency. This can go away with
- // bug 1293445.
- await this.db.execute(`
- UPDATE moz_places SET
- frecency = -1
- WHERE id IN (SELECT placeId FROM itemsRemoved
- WHERE NOT isUntagging)`);
-
- // Remove annos for the deleted items.
- await this.db.execute(`
- DELETE FROM moz_items_annos
- WHERE item_id IN (SELECT itemId FROM itemsRemoved
- WHERE NOT isUntagging)`);
-
- // Remove any local tombstones for deleted items.
- await this.db.execute(`
- DELETE FROM moz_bookmarks_deleted
- WHERE guid IN (SELECT guid FROM itemsRemoved)`);
-
- await this.db.execute(`
- DELETE FROM moz_bookmarks
- WHERE id IN (SELECT itemId FROM itemsRemoved
- WHERE NOT isUntagging)`);
-
- // Flag locally deleted items as merged.
- await this.db.execute(`
- UPDATE items SET
- needsMerge = 0
- WHERE needsMerge AND
- guid IN (SELECT guid FROM itemsRemoved
- WHERE NOT isUntagging)`);
- }
-
- MirrorLog.trace("Flagging remotely deleted items as merged");
- for (let chunk of PlacesSyncUtils.chunkArray(remoteDeletions,
- SQLITE_MAX_VARIABLE_NUMBER)) {
-
- await this.db.execute(`
- UPDATE items SET
- needsMerge = 0
- WHERE needsMerge AND
- guid IN (${new Array(chunk.length).fill("?").join(",")})`,
- chunk);
- }
+ await this.db.execute(`DELETE FROM itemsToRemove`);
}
/**
* Records Places observer notifications for removed, added, moved, and
* changed items.
*
* @param {BookmarkObserverRecorder} observersToNotify
*/
@@ -1687,19 +1633,18 @@ class SyncedBookmarksMirror {
await this.db.execute(`
INSERT INTO structureToUpload(guid, parentId, position)
SELECT b.guid, b.parent, b.position FROM moz_bookmarks b
JOIN itemsToUpload o ON o.id = b.parent`);
// Finally, stage tombstones for deleted items. Ignore conflicts if we have
// tombstones for undeleted items; Places Maintenance should clean these up.
await this.db.execute(`
- INSERT OR IGNORE INTO itemsToUpload(guid, syncChangeCounter, isDeleted,
- dateAdded)
- SELECT guid, 1, 1, dateRemoved / 1000 FROM moz_bookmarks_deleted`);
+ INSERT OR IGNORE INTO itemsToUpload(guid, syncChangeCounter, isDeleted)
+ SELECT guid, 1, 1 FROM moz_bookmarks_deleted`);
}
/**
* Inflates Sync records for all staged outgoing items.
*
* @return {Object.<String, BookmarkChangeRecord>}
* A changeset containing Sync record cleartexts for outgoing items
* and tombstones, keyed by their Sync record IDs.
@@ -2191,16 +2136,88 @@ async function initializeTempMirrorEntit
parentGuid TEXT NOT NULL,
level INTEGER NOT NULL,
position INTEGER NOT NULL,
valueState INTEGER NOT NULL,
structureState INTEGER NOT NULL,
PRIMARY KEY(localGuid, mergedGuid)
) WITHOUT ROWID`);
+ // Stages all items to delete locally and remotely. Items to delete locally
+ // don't need tombstones: since we took the remote deletion, the tombstone
+ // already exists on the server. Items to delete remotely, or non-syncable
+ // items to delete on both sides, need tombstones.
+ await db.execute(`CREATE TEMP TABLE itemsToRemove(
+ guid TEXT PRIMARY KEY,
+ localLevel INTEGER NOT NULL,
+ shouldUploadTombstone BOOLEAN NOT NULL
+ ) WITHOUT ROWID`);
+
+ await db.execute(`
+ CREATE TEMP TRIGGER noteItemRemoved
+ AFTER INSERT ON itemsToRemove
+ BEGIN
+ /* Note that we can't record item removed notifications in the
+ "removeLocalItems" trigger, because SQLite can delete rows in any
+ order, and might fire the trigger for a removed parent before its
+ children. */
+ INSERT INTO itemsRemoved(itemId, parentId, position, type, placeId,
+ guid, parentGuid, level)
+ SELECT b.id, b.parent, b.position, b.type, b.fk, b.guid, p.guid,
+ NEW.localLevel
+ FROM moz_bookmarks b
+ JOIN moz_bookmarks p ON p.id = b.parent
+ WHERE b.guid = NEW.guid;
+ END`);
+
+ // Removes items that are deleted on one or both sides from Places, and
+ // inserts new tombstones for non-syncable items to delete remotely.
+ await db.execute(`
+ CREATE TEMP TRIGGER removeLocalItems
+ AFTER DELETE ON itemsToRemove
+ BEGIN
+ /* Recalculate frecencies. */
+ UPDATE moz_places SET
+ frecency = -1
+ WHERE id = (SELECT fk FROM moz_bookmarks
+ WHERE guid = OLD.guid);
+
+ /* Remove annos for the deleted items. */
+ DELETE FROM moz_items_annos
+ WHERE item_id = (SELECT id FROM moz_bookmarks
+ WHERE guid = OLD.guid);
+
+ /* Don't reupload tombstones for items that are already deleted on the
+ server. */
+ DELETE FROM moz_bookmarks_deleted
+ WHERE NOT OLD.shouldUploadTombstone AND
+ guid = OLD.guid;
+
+ /* Upload tombstones for non-syncable items. We can remove the
+ "shouldUploadTombstone" check and persist tombstones unconditionally
+ in bug 1343103. */
+ INSERT OR IGNORE INTO moz_bookmarks_deleted(guid, dateRemoved)
+ SELECT OLD.guid, STRFTIME('%s', 'now', 'localtime', 'utc')
+ WHERE OLD.shouldUploadTombstone;
+
+ /* Remove the item from Places. */
+ DELETE FROM moz_bookmarks
+ WHERE guid = OLD.guid;
+
+ /* Flag applied deletions as merged. */
+ UPDATE items SET
+ needsMerge = 0
+ WHERE needsMerge AND
+ guid = OLD.guid AND
+ /* Don't flag tombstones for items that don't exist in the local
+ tree. This can be removed once we persist tombstones in bug
+ 1343103. */
+ (NOT isDeleted OR OLD.localLevel > -1);
+ END`);
+
// A view of the value states for all bookmarks in the mirror. We use triggers
// on this view to update Places. Note that we can't just `REPLACE INTO
// moz_bookmarks`, because `REPLACE` doesn't fire the `AFTER DELETE` triggers
// that Places uses to maintain schema coherency.
await db.execute(`
CREATE TEMP VIEW itemsToMerge(localId, remoteId, hasRemoteValue, newLevel,
oldGuid, newGuid, newType,
newDateAddedMicroseconds, newTitle,
@@ -3413,16 +3430,36 @@ class BookmarkMerger {
return true;
}
mentions(guid) {
return this.mergedGuids.has(guid) || this.deleteLocally.has(guid) ||
this.deleteRemotely.has(guid);
}
+ * deletions() {
+ // Items that should be deleted locally already have tombstones on the
+ // server, so we don't need to upload tombstones for these deletions.
+ for (let guid of this.deleteLocally) {
+ if (this.deleteRemotely.has(guid)) {
+ continue;
+ }
+ let localNode = this.localTree.nodeForGuid(guid);
+ yield { guid, localLevel: localNode ? localNode.level : -1,
+ shouldUploadTombstone: false };
+ }
+
+ // Items that should be deleted remotely, or on both sides, need tombstones.
+ for (let guid of this.deleteRemotely) {
+ let localNode = this.localTree.nodeForGuid(guid);
+ yield { guid, localLevel: localNode ? localNode.level : -1,
+ shouldUploadTombstone: true };
+ }
+ }
+
/**
* Merges two nodes, recursively walking folders.
*
* @param {String} guid
* The GUID to use for the merged node.
* @param {BookmarkNode?} localNode
* The local node. May be `null` if the node only exists remotely.
* @param {BookmarkNode?} remoteNode
--- a/toolkit/components/places/tests/sync/test_bookmark_corruption.js
+++ b/toolkit/components/places/tests/sync/test_bookmark_corruption.js
@@ -1122,16 +1122,97 @@ add_task(async function test_non_syncabl
}]);
let changesToUpload = await buf.apply();
deepEqual(await buf.fetchUnmergedGuids(), [], "Should merge all items");
let datesAdded = await promiseManyDatesAdded([PlacesUtils.bookmarks.menuGuid,
PlacesUtils.bookmarks.unfiledGuid, "bookmarkBBBB", "bookmarkJJJJ"]);
deepEqual(changesToUpload, {
+ folderAAAAAA: {
+ tombstone: true,
+ counter: 1,
+ synced: false,
+ cleartext: {
+ id: "folderAAAAAA",
+ deleted: true,
+ },
+ },
+ folderDDDDDD: {
+ tombstone: true,
+ counter: 1,
+ synced: false,
+ cleartext: {
+ id: "folderDDDDDD",
+ deleted: true,
+ },
+ },
+ folderLEFTPQ: {
+ tombstone: true,
+ counter: 1,
+ synced: false,
+ cleartext: {
+ id: "folderLEFTPQ",
+ deleted: true,
+ },
+ },
+ folderLEFTPC: {
+ tombstone: true,
+ counter: 1,
+ synced: false,
+ cleartext: {
+ id: "folderLEFTPC",
+ deleted: true,
+ },
+ },
+ folderLEFTPR: {
+ tombstone: true,
+ counter: 1,
+ synced: false,
+ cleartext: {
+ id: "folderLEFTPR",
+ deleted: true,
+ },
+ },
+ folderLEFTPF: {
+ tombstone: true,
+ counter: 1,
+ synced: false,
+ cleartext: {
+ id: "folderLEFTPF",
+ deleted: true,
+ },
+ },
+ rootHHHHHHHH: {
+ tombstone: true,
+ counter: 1,
+ synced: false,
+ cleartext: {
+ id: "rootHHHHHHHH",
+ deleted: true,
+ },
+ },
+ bookmarkFFFF: {
+ tombstone: true,
+ counter: 1,
+ synced: false,
+ cleartext: {
+ id: "bookmarkFFFF",
+ deleted: true,
+ },
+ },
+ bookmarkIIII: {
+ tombstone: true,
+ counter: 1,
+ synced: false,
+ cleartext: {
+ id: "bookmarkIIII",
+ deleted: true,
+ },
+ },
bookmarkBBBB: {
tombstone: false,
counter: 1,
synced: false,
cleartext: {
id: "bookmarkBBBB",
type: "bookmark",
parentid: "menu",
@@ -1230,17 +1311,17 @@ add_task(async function test_non_syncabl
url: "http://example.com/g",
}],
}, {
guid: PlacesUtils.bookmarks.mobileGuid,
type: PlacesUtils.bookmarks.TYPE_FOLDER,
index: 4,
title: MobileBookmarksTitle,
}],
- }, "Should upload new structure excluding non-syncable items");
+ }, "Should exclude non-syncable items from new local structure");
await buf.finalize();
await PlacesUtils.bookmarks.eraseEverything();
await PlacesSyncUtils.bookmarks.reset();
});
// See what happens when a left-pane root and a left-pane query are on the server
add_task(async function test_left_pane_root() {