Bug 1380606 - Add a Places maintenance task to fix up items with invalid GUIDs. r=mak
This task assigns new GUIDs to items with missing or invalid ones, and
bumps the Sync change counter for the affected items and their parents.
We also invalidate the GUID cache for the affected items, and write
tombstones for invalid GUIDs that have already been synced.
MozReview-Commit-ID: 7Jm3enYchAz
--- a/toolkit/components/places/PlacesDBUtils.jsm
+++ b/toolkit/components/places/PlacesDBUtils.jsm
@@ -53,16 +53,17 @@ this.PlacesDBUtils = {
*
* @return a Map[taskName(String) -> Object]. The Object has the following properties:
* - succeeded: boolean
* - logs: an array of strings containing the messages logged by the task.
*/
async maintenanceOnIdle() {
let tasks = [
this.checkIntegrity,
+ this.invalidateCaches,
this.checkCoherence,
this._refreshUI
];
let telemetryStartTime = Date.now();
let taskStatusMap = await PlacesDBUtils.runTasks(tasks);
Services.prefs.setIntPref("places.database.lastMaintenance",
parseInt(Date.now() / 1000));
@@ -82,16 +83,17 @@ this.PlacesDBUtils = {
* A promise that resolves with a Map[taskName(String) -> Object].
* The Object has the following properties:
* - succeeded: boolean
* - logs: an array of strings containing the messages logged by the task.
*/
async checkAndFixDatabase() {
let tasks = [
this.checkIntegrity,
+ this.invalidateCaches,
this.checkCoherence,
this.expire,
this.vacuum,
this.stats,
this._refreshUI,
];
return PlacesDBUtils.runTasks(tasks);
},
@@ -197,16 +199,40 @@ this.PlacesDBUtils = {
// There was some other error, so throw.
PlacesDBUtils.clearPendingTasks();
throw new Error("Unable to check database integrity");
}
}
return logs;
},
+ async invalidateCaches() {
+ let logs = [];
+ try {
+ await PlacesUtils.withConnectionWrapper(
+ "PlacesDBUtils: invalidate caches",
+ async (db) => {
+ let idsWithInvalidGuidsRows = await db.execute(`
+ SELECT id FROM moz_bookmarks
+ WHERE guid IS NULL OR
+ NOT IS_VALID_GUID(guid)`);
+ for (let row of idsWithInvalidGuidsRows) {
+ let id = row.getResultByName("id");
+ PlacesUtils.invalidateCachedGuidFor(id);
+ }
+ }
+ );
+ logs.push("The caches have been invalidated");
+ } catch (ex) {
+ PlacesDBUtils.clearPendingTasks();
+ throw new Error("Unable to invalidate caches");
+ }
+ return logs;
+ },
+
/**
* Checks data coherence and tries to fix most common errors.
*
* @return {Promise} resolves when coherence is checked.
* @resolves to an array of logs for this task.
* @rejects if database is not coherent.
*/
async checkCoherence() {
@@ -754,16 +780,66 @@ this.PlacesDBUtils = {
cleanupStatements.push(fixForeignCount);
// L.5 recalculate missing hashes.
let fixMissingHashes = {
query: `UPDATE moz_places SET url_hash = hash(url) WHERE url_hash = 0`
};
cleanupStatements.push(fixMissingHashes);
+ // MOZ_BOOKMARKS
+ // S.1 fix invalid GUIDs for synced bookmarks.
+ // This requires multiple related statements.
+ // First, we insert tombstones for all synced bookmarks with invalid
+ // GUIDs, so that we can delete them on the server. Second, we add a
+ // temporary trigger to bump the change counter for the parents of any
+ // items we update, since Sync stores the list of child GUIDs on the
+ // parent. Finally, we assign new GUIDs for all items with missing and
+ // invalid GUIDs, bump their change counters, and reset their sync
+ // statuses to NEW so that they're considered for deduping.
+ cleanupStatements.push({
+ query:
+ `INSERT OR IGNORE INTO moz_bookmarks_deleted(guid, dateRemoved)
+ SELECT guid, :dateRemoved
+ FROM moz_bookmarks
+ WHERE syncStatus <> :syncStatus AND
+ guid NOT NULL AND
+ NOT IS_VALID_GUID(guid)`,
+ params: {
+ dateRemoved: PlacesUtils.toPRTime(new Date()),
+ syncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NEW,
+ },
+ });
+ cleanupStatements.push({
+ query:
+ `CREATE TEMP TRIGGER IF NOT EXISTS moz_bm_fix_guids_temp_trigger
+ AFTER UPDATE of guid ON moz_bookmarks
+ FOR EACH ROW
+ BEGIN
+ UPDATE moz_bookmarks
+ SET syncChangeCounter = syncChangeCounter + 1
+ WHERE id = NEW.parent;
+ END`,
+ });
+ cleanupStatements.push({
+ query:
+ `UPDATE moz_bookmarks
+ SET guid = GENERATE_GUID(),
+ syncChangeCounter = syncChangeCounter + 1,
+ syncStatus = :syncStatus
+ WHERE guid IS NULL OR
+ NOT IS_VALID_GUID(guid)`,
+ params: {
+ syncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NEW,
+ },
+ });
+ cleanupStatements.push({
+ query: "DROP TRIGGER moz_bm_fix_guids_temp_trigger",
+ });
+
// MAINTENANCE STATEMENTS SHOULD GO ABOVE THIS POINT!
return cleanupStatements;
},
/**
* Tries to vacuum the database.
*
--- a/toolkit/components/places/tests/head_common.js
+++ b/toolkit/components/places/tests/head_common.js
@@ -37,16 +37,18 @@ XPCOMUtils.defineLazyModuleGetter(this,
XPCOMUtils.defineLazyModuleGetter(this, "Services",
"resource://gre/modules/Services.jsm");
XPCOMUtils.defineLazyModuleGetter(this, "BookmarkJSONUtils",
"resource://gre/modules/BookmarkJSONUtils.jsm");
XPCOMUtils.defineLazyModuleGetter(this, "BookmarkHTMLUtils",
"resource://gre/modules/BookmarkHTMLUtils.jsm");
XPCOMUtils.defineLazyModuleGetter(this, "PlacesBackups",
"resource://gre/modules/PlacesBackups.jsm");
+XPCOMUtils.defineLazyModuleGetter(this, "PlacesSyncUtils",
+ "resource://gre/modules/PlacesSyncUtils.jsm");
XPCOMUtils.defineLazyModuleGetter(this, "PlacesTestUtils",
"resource://testing-common/PlacesTestUtils.jsm");
XPCOMUtils.defineLazyModuleGetter(this, "PlacesTransactions",
"resource://gre/modules/PlacesTransactions.jsm");
XPCOMUtils.defineLazyModuleGetter(this, "OS",
"resource://gre/modules/osfile.jsm");
XPCOMUtils.defineLazyModuleGetter(this, "Sqlite",
"resource://gre/modules/Sqlite.jsm");
--- a/toolkit/components/places/tests/unit/test_preventive_maintenance.js
+++ b/toolkit/components/places/tests/unit/test_preventive_maintenance.js
@@ -59,28 +59,34 @@ function addPlace(aUrl, aFavicon) {
stmt.params.url = href;
stmt.params.favicon = aFavicon;
stmt.execute();
stmt.finalize();
}
return id;
}
-function addBookmark(aPlaceId, aType, aParent, aKeywordId, aFolderType, aTitle) {
+function addBookmark(aPlaceId, aType, aParent, aKeywordId, aFolderType, aTitle,
+ aGuid = PlacesUtils.history.makeGuid(),
+ aSyncStatus = PlacesUtils.bookmarks.SYNC_STATUS.NEW,
+ aSyncChangeCounter = 0) {
let stmt = mDBConn.createStatement(
`INSERT INTO moz_bookmarks (fk, type, parent, keyword_id, folder_type,
- title, guid)
+ title, guid, syncStatus, syncChangeCounter)
VALUES (:place_id, :type, :parent, :keyword_id, :folder_type, :title,
- GENERATE_GUID())`);
+ :guid, :sync_status, :change_counter)`);
stmt.params.place_id = aPlaceId || null;
stmt.params.type = aType || bs.TYPE_BOOKMARK;
stmt.params.parent = aParent || bs.unfiledBookmarksFolder;
stmt.params.keyword_id = aKeywordId || null;
stmt.params.folder_type = aFolderType || null;
stmt.params.title = typeof(aTitle) == "string" ? aTitle : null;
+ stmt.params.guid = aGuid;
+ stmt.params.sync_status = aSyncStatus;
+ stmt.params.change_counter = aSyncChangeCounter;
stmt.execute();
stmt.finalize();
return mDBConn.lastInsertRowID;
}
// ------------------------------------------------------------------------------
// Tests
@@ -1230,16 +1236,109 @@ tests.push({
async check() {
Assert.ok((await this._getHash()) > 0);
}
});
// ------------------------------------------------------------------------------
tests.push({
+ name: "S.1",
+ desc: "fix invalid GUIDs for synced bookmarks",
+ _bookmarkInfos: [],
+
+ async setup() {
+ await PlacesTestUtils.markBookmarksAsSynced();
+
+ let folderWithInvalidGuid = addBookmark(
+ null, PlacesUtils.bookmarks.TYPE_FOLDER,
+ PlacesUtils.bookmarks.bookmarksMenuFolder, /* aKeywordId */ null,
+ /* aFolderType */ null, "NORMAL folder with invalid GUID",
+ "{123456}", PlacesUtils.bookmarks.SYNC_STATUS.NORMAL);
+
+ let placeIdForBookmarkWithoutGuid = addPlace();
+ let bookmarkWithoutGuid = addBookmark(
+ placeIdForBookmarkWithoutGuid, PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ folderWithInvalidGuid, /* aKeywordId */ null,
+ /* aFolderType */ null, "NEW bookmark without GUID",
+ /* aGuid */ null);
+
+ let placeIdForBookmarkWithInvalidGuid = addPlace();
+ let bookmarkWithInvalidGuid = addBookmark(
+ placeIdForBookmarkWithInvalidGuid, PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ folderWithInvalidGuid, /* aKeywordId */ null,
+ /* aFolderType */ null, "NORMAL bookmark with invalid GUID",
+ "bookmarkAAAA\n", PlacesUtils.bookmarks.SYNC_STATUS.NORMAL);
+
+ let placeIdForBookmarkWithValidGuid = addPlace();
+ let bookmarkWithValidGuid = addBookmark(
+ placeIdForBookmarkWithValidGuid, PlacesUtils.bookmarks.TYPE_BOOKMARK,
+ folderWithInvalidGuid, /* aKeywordId */ null,
+ /* aFolderType */ null, "NORMAL bookmark with valid GUID",
+ "bookmarkBBBB", PlacesUtils.bookmarks.SYNC_STATUS.NORMAL);
+
+ this._bookmarkInfos.push({
+ id: PlacesUtils.bookmarks.bookmarksMenuFolder,
+ syncChangeCounter: 1,
+ syncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NORMAL,
+ }, {
+ id: folderWithInvalidGuid,
+ syncChangeCounter: 3,
+ syncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NEW,
+ }, {
+ id: bookmarkWithoutGuid,
+ syncChangeCounter: 1,
+ syncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NEW,
+ }, {
+ id: bookmarkWithInvalidGuid,
+ syncChangeCounter: 1,
+ syncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NEW,
+ }, {
+ id: bookmarkWithValidGuid,
+ syncChangeCounter: 0,
+ syncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NORMAL,
+ });
+ },
+
+ async check() {
+ let db = await PlacesUtils.promiseDBConnection();
+ let updatedRows = await db.execute(`
+ SELECT id, guid, syncChangeCounter, syncStatus
+ FROM moz_bookmarks
+ WHERE id IN (?, ?, ?, ?, ?)`,
+ this._bookmarkInfos.map(info => info.id));
+
+ for (let row of updatedRows) {
+ let id = row.getResultByName("id");
+ let guid = row.getResultByName("guid");
+ do_check_true(PlacesUtils.isValidGuid(guid));
+
+ let cachedGuid = await PlacesUtils.promiseItemGuid(id);
+ do_check_eq(cachedGuid, guid);
+
+ let expectedInfo = this._bookmarkInfos.find(info => info.id == id);
+
+ let syncChangeCounter = row.getResultByName("syncChangeCounter");
+ do_check_eq(syncChangeCounter, expectedInfo.syncChangeCounter);
+
+ let syncStatus = row.getResultByName("syncStatus");
+ do_check_eq(syncStatus, expectedInfo.syncStatus);
+ }
+
+ let tombstones = await PlacesTestUtils.fetchSyncTombstones();
+ do_check_matches(tombstones.map(info => info.guid),
+ ["bookmarkAAAA\n", "{123456}"]);
+
+ await PlacesSyncUtils.bookmarks.reset();
+ },
+});
+
+// ------------------------------------------------------------------------------
+
+tests.push({
name: "Z",
desc: "Sanity: Preventive maintenance does not touch valid items",
_uri1: uri("http://www1.mozilla.org"),
_uri2: uri("http://www2.mozilla.org"),
_folder: null,
_bookmark: null,
_bookmarkId: null,
--- a/toolkit/components/places/tests/unit/test_preventive_maintenance_checkAndFixDatabase.js
+++ b/toolkit/components/places/tests/unit/test_preventive_maintenance_checkAndFixDatabase.js
@@ -18,12 +18,12 @@ add_task(async function() {
let failedTasks = [];
tasksStatusMap.forEach(val => {
if (val.succeeded) {
successfulTasks.push(val);
} else {
failedTasks.push(val);
}
});
- Assert.equal(numberOfTasksRun, 6, "Check that we have run all tasks.");
- Assert.equal(successfulTasks.length, 6, "Check that we have run all tasks successfully");
+ Assert.equal(numberOfTasksRun, 7, "Check that we have run all tasks.");
+ Assert.equal(successfulTasks.length, 7, "Check that we have run all tasks successfully");
Assert.equal(failedTasks.length, 0, "Check that no task is failing");
});