Bug 1380606 - Add a Places maintenance task to fix up items with invalid GUIDs. r=mak
authorKit Cambridge <kit@yakshaving.ninja>
Thu, 05 Oct 2017 11:33:54 -0700
changeset 385679 479c9304b70343d8ede784bc8079513a6fab04ca
parent 385678 455609e45fe7b0e59bcb874e86a3a88c002c0968
child 385680 a97ced54c8398a23b48e6b61d3dae4e8112acff8
push id32664
push userarchaeopteryx@coole-files.de
push dateThu, 12 Oct 2017 09:34:55 +0000
treeherdermozilla-central@a32c32d9631c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak
bugs1380606
milestone58.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
toolkit/components/places/PlacesDBUtils.jsm
toolkit/components/places/tests/head_common.js
toolkit/components/places/tests/unit/test_preventive_maintenance.js
toolkit/components/places/tests/unit/test_preventive_maintenance_checkAndFixDatabase.js
--- 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");
 });