From f0b3559610eb1aa6b00c5f0e07b166309638f9f7 Mon Sep 17 00:00:00 2001 From: Kit Cambridge Date: Fri, 25 Aug 2017 12:04:22 -0700 Subject: [PATCH] Bug 1393904 - Ensure `insertTree` removes Sync tombstones for restored bookmarks. r=mak MozReview-Commit-ID: EbGybRbhWKJ --- services/sync/tests/unit/test_telemetry.js | 1 - toolkit/components/places/Bookmarks.jsm | 21 +++++ toolkit/components/places/PlacesSyncUtils.jsm | 11 ++- .../places/tests/unit/test_sync_utils.js | 77 ++++++++++++++++++- 4 files changed, 105 insertions(+), 5 deletions(-) diff --git a/services/sync/tests/unit/test_telemetry.js b/services/sync/tests/unit/test_telemetry.js index da75d765ca01d..c3dd1a0bbfdd1 100644 --- a/services/sync/tests/unit/test_telemetry.js +++ b/services/sync/tests/unit/test_telemetry.js @@ -181,7 +181,6 @@ add_task(async function test_uploading() { PlacesUtils.bookmarks.setItemTitle(bmk_id, "New Title"); - await store.wipe(); await engine.resetClient(); ping = await sync_engine_and_validate_telem(engine, false); diff --git a/toolkit/components/places/Bookmarks.jsm b/toolkit/components/places/Bookmarks.jsm index b8091e83dff9e..693d6cbadc531 100644 --- a/toolkit/components/places/Bookmarks.jsm +++ b/toolkit/components/places/Bookmarks.jsm @@ -95,6 +95,7 @@ async function promiseTagsFolderId() { const MATCH_ANYWHERE_UNMODIFIED = Ci.mozIPlacesAutoComplete.MATCH_ANYWHERE_UNMODIFIED; const BEHAVIOR_BOOKMARK = Ci.mozIPlacesAutoComplete.BEHAVIOR_BOOKMARK; +const SQLITE_MAX_VARIABLE_NUMBER = 999; var Bookmarks = Object.freeze({ /** @@ -1438,6 +1439,15 @@ function insertBookmarkTree(items, source, parent, urls, lastAddedForParent) { NULLIF(:title, ""), :date_added, :last_modified, :guid, :syncChangeCounter, :syncStatus)`, items); + // Remove stale tombstones for new items. + for (let chunk of chunkArray(items, SQLITE_MAX_VARIABLE_NUMBER)) { + await db.executeCached( + `DELETE FROM moz_bookmarks_deleted WHERE guid IN (${ + new Array(chunk.length).fill("?").join(",")})`, + chunk.map(item => item.guid) + ); + } + await setAncestorsLastModified(db, parent.guid, lastAddedForParent, syncChangeDelta); }); @@ -2384,3 +2394,14 @@ function adjustSeparatorsSyncCounter(db, parentId, startIndex, syncChangeDelta) item_type: Bookmarks.TYPE_SEPARATOR }); } + +function* chunkArray(array, chunkLength) { + if (array.length <= chunkLength) { + yield array; + return; + } + let startIndex = 0; + while (startIndex < array.length) { + yield array.slice(startIndex, startIndex += chunkLength); + } +} diff --git a/toolkit/components/places/PlacesSyncUtils.jsm b/toolkit/components/places/PlacesSyncUtils.jsm index 7bacecaa5bb62..76c21a13971e4 100644 --- a/toolkit/components/places/PlacesSyncUtils.jsm +++ b/toolkit/components/places/PlacesSyncUtils.jsm @@ -1947,6 +1947,9 @@ async function fetchQueryItem(db, bookmarkItem) { function addRowToChangeRecords(row, changeRecords) { let syncId = BookmarkSyncUtils.guidToSyncId(row.getResultByName("guid")); + if (syncId in changeRecords) { + throw new Error(`Duplicate entry for ${syncId} in changeset`); + } let modifiedAsPRTime = row.getResultByName("modified"); let modified = modifiedAsPRTime / MICROSECONDS_PER_SECOND; if (Number.isNaN(modified) || modified <= 0) { @@ -1976,7 +1979,7 @@ function addRowToChangeRecords(row, changeRecords) { var pullSyncChanges = async function(db) { let changeRecords = {}; - await db.executeCached(` + let rows = await db.executeCached(` WITH RECURSIVE syncedItems(id, guid, modified, syncChangeCounter, syncStatus) AS ( SELECT b.id, b.guid, b.lastModified, b.syncChangeCounter, b.syncStatus @@ -1995,8 +1998,10 @@ var pullSyncChanges = async function(db) { SELECT guid, dateRemoved AS modified, 1 AS syncChangeCounter, :deletedSyncStatus, 1 AS tombstone FROM moz_bookmarks_deleted`, - { deletedSyncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NORMAL }, - row => addRowToChangeRecords(row, changeRecords)); + { deletedSyncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NORMAL }); + for (let row of rows) { + addRowToChangeRecords(row, changeRecords); + } return changeRecords; }; diff --git a/toolkit/components/places/tests/unit/test_sync_utils.js b/toolkit/components/places/tests/unit/test_sync_utils.js index 00ee4f7b00092..c1a08c383f801 100644 --- a/toolkit/components/places/tests/unit/test_sync_utils.js +++ b/toolkit/components/places/tests/unit/test_sync_utils.js @@ -2230,7 +2230,7 @@ add_task(async function test_pullChanges_restore_json_tracked() { ], "Pulling items restored from JSON backup should not mark them as syncing"); let tombstones = await PlacesTestUtils.fetchSyncTombstones(); - ok(tombstones.map(({ guid }) => guid), [syncedFolder.guid], + deepEqual(tombstones.map(({ guid }) => guid), [syncedFolder.guid], "Tombstones should exist after restoring from JSON backup"); await PlacesSyncUtils.bookmarks.markChangesAsSyncing(changes); @@ -2909,3 +2909,78 @@ add_task(async function test_ensureMobileQuery() { await PlacesUtils.bookmarks.eraseEverything(); await PlacesSyncUtils.bookmarks.reset(); }); + +add_task(async function test_remove_stale_tombstones() { + do_print("Insert and delete synced bookmark"); + { + await PlacesUtils.bookmarks.insert({ + guid: "bookmarkAAAA", + parentGuid: PlacesUtils.bookmarks.toolbarGuid, + url: "http://example.com/a", + title: "A", + source: PlacesUtils.bookmarks.SOURCES.SYNC, + }); + await PlacesUtils.bookmarks.remove("bookmarkAAAA"); + let tombstones = await PlacesTestUtils.fetchSyncTombstones(); + deepEqual(tombstones.map(({ guid }) => guid), ["bookmarkAAAA"], + "Should store tombstone for deleted synced bookmark"); + } + + do_print("Reinsert deleted bookmark"); + { + // Different parent, URL, and title, but same GUID. + await PlacesUtils.bookmarks.insert({ + guid: "bookmarkAAAA", + parentGuid: PlacesUtils.bookmarks.unfiledGuid, + url: "http://example.com/a-restored", + title: "A (Restored)", + }); + + let tombstones = await PlacesTestUtils.fetchSyncTombstones(); + deepEqual(tombstones, [], + "Should remove tombstone for reinserted bookmark"); + } + + do_print("Insert tree and erase everything"); + { + await PlacesUtils.bookmarks.insertTree({ + guid: PlacesUtils.bookmarks.menuGuid, + source: PlacesUtils.bookmarks.SOURCES.SYNC, + children: [{ + guid: "bookmarkBBBB", + url: "http://example.com/b", + title: "B", + }, { + guid: "bookmarkCCCC", + url: "http://example.com/c", + title: "C", + }], + }); + await PlacesUtils.bookmarks.eraseEverything(); + let tombstones = await PlacesTestUtils.fetchSyncTombstones(); + deepEqual(tombstones.map(({ guid }) => guid).sort(), ["bookmarkBBBB", + "bookmarkCCCC"], "Should store tombstones after erasing everything"); + } + + do_print("Reinsert tree"); + { + await PlacesUtils.bookmarks.insertTree({ + guid: PlacesUtils.bookmarks.mobileGuid, + children: [{ + guid: "bookmarkBBBB", + url: "http://example.com/b", + title: "B", + }, { + guid: "bookmarkCCCC", + url: "http://example.com/c", + title: "C", + }], + }); + let tombstones = await PlacesTestUtils.fetchSyncTombstones(); + deepEqual(tombstones.map(({ guid }) => guid).sort(), [], + "Should remove tombstones after reinserting tree"); + } + + await PlacesUtils.bookmarks.eraseEverything(); + await PlacesSyncUtils.bookmarks.reset(); +});