Bug 1393904 - Ensure `insertTree` removes Sync tombstones for restored bookmarks. r=mak
MozReview-Commit-ID: EbGybRbhWKJ
--- a/services/sync/tests/unit/test_telemetry.js
+++ b/services/sync/tests/unit/test_telemetry.js
@@ -176,17 +176,16 @@ add_task(async function test_uploading()
equal(ping.engines.length, 1);
equal(ping.engines[0].name, "bookmarks");
ok(!!ping.engines[0].outgoing);
greater(ping.engines[0].outgoing[0].sent, 0)
ok(!ping.engines[0].incoming);
PlacesUtils.bookmarks.setItemTitle(bmk_id, "New Title");
- await store.wipe();
await engine.resetClient();
ping = await sync_engine_and_validate_telem(engine, false);
equal(ping.engines.length, 1);
equal(ping.engines[0].name, "bookmarks");
equal(ping.engines[0].outgoing.length, 1);
ok(!!ping.engines[0].incoming);
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -90,16 +90,17 @@ async function promiseTagsFolderId() {
"SELECT id FROM moz_bookmarks WHERE guid = :guid",
{ guid: Bookmarks.tagsGuid }
);
return gTagsFolderId = rows[0].getResultByName("id");
}
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({
/**
* Item's type constants.
* These should stay consistent with nsINavBookmarksService.idl
*/
TYPE_BOOKMARK: 1,
TYPE_FOLDER: 2,
@@ -1433,16 +1434,25 @@ function insertBookmarkTree(items, sourc
dateAdded, lastModified, guid,
syncChangeCounter, syncStatus)
VALUES (CASE WHEN :url ISNULL THEN NULL ELSE (SELECT id FROM moz_places WHERE url_hash = hash(:url) AND url = :url) END, :type,
(SELECT id FROM moz_bookmarks WHERE guid = :parentGuid),
IFNULL(:index, (SELECT count(*) FROM moz_bookmarks WHERE parent = :rootId)),
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);
});
// We don't wait for the frecency calculation.
updateFrecency(db, urls, true).catch(Cu.reportError);
return items;
@@ -2379,8 +2389,19 @@ function adjustSeparatorsSyncCounter(db,
`,
{
delta: syncChangeDelta,
parent: parentId,
start_index: startIndex,
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);
+ }
+}
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -1942,16 +1942,19 @@ async function fetchQueryItem(db, bookma
item.query = query;
}
return item;
}
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) {
BookmarkSyncLog.error("addRowToChangeRecords: Invalid modified date for " +
syncId, modifiedAsPRTime);
modified = 0;
}
changeRecords[syncId] = {
@@ -1971,17 +1974,17 @@ function addRowToChangeRecords(row, chan
* The Sqlite.jsm connection handle.
* @return {Promise} resolved once all items have been fetched.
* @resolves to an object containing records for changed bookmarks, keyed by
* the sync ID.
*/
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
FROM moz_bookmarks b
WHERE b.guid IN ('menu________', 'toolbar_____', 'unfiled_____',
'mobile______')
UNION ALL
SELECT b.id, b.guid, b.lastModified, b.syncChangeCounter, b.syncStatus
@@ -1990,18 +1993,20 @@ var pullSyncChanges = async function(db)
)
SELECT guid, modified, syncChangeCounter, syncStatus, 0 AS tombstone
FROM syncedItems
WHERE syncChangeCounter >= 1
UNION ALL
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;
};
var touchSyncBookmark = async function(db, bookmarkItem) {
if (BookmarkSyncLog.level <= Log.Level.Trace) {
BookmarkSyncLog.trace(
`touch: Reviving item "${bookmarkItem.guid}" and marking parent ` +
--- a/toolkit/components/places/tests/unit/test_sync_utils.js
+++ b/toolkit/components/places/tests/unit/test_sync_utils.js
@@ -2225,17 +2225,17 @@ add_task(async function test_pullChanges
PlacesUtils.bookmarks.SYNC_STATUS.NORMAL,
PlacesUtils.bookmarks.SYNC_STATUS.NORMAL,
PlacesUtils.bookmarks.SYNC_STATUS.NORMAL,
PlacesUtils.bookmarks.SYNC_STATUS.UNKNOWN,
PlacesUtils.bookmarks.SYNC_STATUS.UNKNOWN,
], "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);
let normalFields = await PlacesTestUtils.fetchBookmarkSyncFields(
PlacesUtils.bookmarks.menuGuid, PlacesUtils.bookmarks.toolbarGuid,
PlacesUtils.bookmarks.unfiledGuid, "NnvGl3CRA4hC", "APzP8MupzA8l");
ok(normalFields.every(field =>
field.syncStatus == PlacesUtils.bookmarks.SYNC_STATUS.NORMAL
@@ -2904,8 +2904,83 @@ add_task(async function test_ensureMobil
do_print("We shouldn't track the query or the left pane root");
let changes = await PlacesSyncUtils.bookmarks.pullChanges();
ok(!(queryGuid in changes), "Should not track mobile query");
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();
+});