Bug 655722 - Rewrite _buildGUIDMap in the sync bookmark engine to use PlacesUtils.promiseBookmarksTree r?mak draft
authorThom Chiovoloni <tchiovoloni@mozilla.com>
Tue, 05 Apr 2016 16:42:10 -0700
changeset 352092 21e410196ef3866da5c82875e7718ef84b582fd4
parent 349439 d6ba2370b0726f39d103a197349a2f6be8f6bc97
child 352093 a0efae36a179a34556afd8156d99c758dd0965d6
push id15611
push userbmo:tchiovoloni@mozilla.com
push dateFri, 15 Apr 2016 17:02:45 +0000
reviewersmak
bugs655722
milestone48.0a1
Bug 655722 - Rewrite _buildGUIDMap in the sync bookmark engine to use PlacesUtils.promiseBookmarksTree r?mak MozReview-Commit-ID: BfcVQEldK6M
services/common/async.js
services/sync/modules/engines/bookmarks.js
services/sync/tests/unit/test_bookmark_engine.js
services/sync/tests/unit/test_bookmark_invalid.js
--- a/services/common/async.js
+++ b/services/common/async.js
@@ -202,9 +202,19 @@ this.Async = {
   querySpinningly: function querySpinningly(query, names) {
     // 'Synchronously' asyncExecute, fetching all results by name.
     let storageCallback = Object.create(Async._storageCallbackPrototype);
     storageCallback.names = names;
     storageCallback.syncCb = Async.makeSyncCallback();
     query.executeAsync(storageCallback);
     return Async.waitForSyncCallback(storageCallback.syncCb);
   },
+
+  promiseSpinningly(promise) {
+    let cb = Async.makeSpinningCallback();
+    promise.then(result =>  {
+      cb(null, result);
+    }, err => {
+      cb(err || new Error("Promise rejected without explicit error"));
+    });
+    return cb.wait();
+  },
 };
--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -267,79 +267,79 @@ BookmarksEngine.prototype = {
       }
     }
     return url;
   },
 
   _guidMapFailed: false,
   _buildGUIDMap: function _buildGUIDMap() {
     let guidMap = {};
-    for (let guid in this._store.getAllIDs()) {
-      // Figure out with which key to store the mapping.
-      let key;
-      let id = this._store.idForGUID(guid);
-      let itemType;
-      try {
-        itemType = PlacesUtils.bookmarks.getItemType(id);
-      } catch (ex) {
-        this._log.warn("Deleting invalid bookmark record with id", id);
-        try {
-          PlacesUtils.bookmarks.removeItem(id);
-        } catch (ex) {
-          this._log.warn("Failed to delete invalid bookmark", ex);
+    let tree = Async.promiseSpinningly(PlacesUtils.promiseBookmarksTree("", {
+      includeItemIds: true
+    }));
+    function* walkBookmarksTree(tree, parent=null) {
+      if (tree) {
+        // Skip root node
+        if (parent) {
+          yield [tree, parent];
         }
-        continue;
+        if (tree.children) {
+          for (let child of tree.children) {
+            yield* walkBookmarksTree(child, tree);
+          }
+        }
       }
-      switch (itemType) {
-        case PlacesUtils.bookmarks.TYPE_BOOKMARK:
+    }
 
-          // Smart bookmarks map to their annotation value.
-          let queryId;
-          try {
-            queryId = PlacesUtils.annotations.getItemAnnotation(
-              id, SMART_BOOKMARKS_ANNO);
-          } catch(ex) {}
+    function* walkBookmarksRoots(tree, rootGUIDs) {
+      for (let guid of rootGUIDs) {
+        let id = kSpecialIds.specialIdForGUID(guid, false);
+        let bookmarkRoot = id === null ? null :
+          tree.children.find(child => child.id === id);
+        if (bookmarkRoot === null) {
+          continue;
+        }
+        yield* walkBookmarksTree(bookmarkRoot, tree);
+      }
+    }
 
-          if (queryId) {
-            key = "q" + queryId;
+    let rootsToWalk = kSpecialIds.guids.filter(guid =>
+      guid !== 'places' && guid !== 'tags');
+
+    for (let [node, parent] of walkBookmarksRoots(tree, rootsToWalk)) {
+      let {guid, id, type: placeType} = node;
+      guid = kSpecialIds.specialGUIDForId(id) || guid;
+      let key;
+      switch (placeType) {
+        case PlacesUtils.TYPE_X_MOZ_PLACE:
+          // Bookmark
+          let query = null;
+          if (node.annos && node.uri.startsWith("place:")) {
+            query = node.annos.find(({name}) => name === SMART_BOOKMARKS_ANNO);
+          }
+          if (query && query.value) {
+            key = "q" + query.value;
           } else {
-            let uri;
-            try {
-              uri = PlacesUtils.bookmarks.getBookmarkURI(id);
-            } catch (ex) {
-              // Bug 1182366 - NS_ERROR_MALFORMED_URI here stops bookmarks sync.
-              // Try and get the string value of the URL for diagnostic purposes.
-              let url = this._getStringUrlForId(id);
-              this._log.warn(`Deleting bookmark with invalid URI. url="${url}", id=${id}`);
-              try {
-                PlacesUtils.bookmarks.removeItem(id);
-              } catch (ex) {
-                this._log.warn("Failed to delete invalid bookmark", ex);
-              }
-              continue;
-            }
-            key = "b" + uri.spec + ":" + PlacesUtils.bookmarks.getItemTitle(id);
+            key = "b" + node.uri + ":" + node.title;
           }
           break;
-        case PlacesUtils.bookmarks.TYPE_FOLDER:
-          key = "f" + PlacesUtils.bookmarks.getItemTitle(id);
+        case PlacesUtils.TYPE_X_MOZ_PLACE_CONTAINER:
+          // Folder
+          key = "f" + node.title;
           break;
-        case PlacesUtils.bookmarks.TYPE_SEPARATOR:
-          key = "s" + PlacesUtils.bookmarks.getItemIndex(id);
+        case PlacesUtils.TYPE_X_MOZ_PLACE_SEPARATOR:
+          // Separator
+          key = "s" + node.index;
           break;
         default:
+          this._log.error("Unknown place type: '"+placeType+"'");
           continue;
       }
 
-      // The mapping is on a per parent-folder-name basis.
-      let parent = PlacesUtils.bookmarks.getFolderIdForItem(id);
-      if (parent <= 0)
-        continue;
-
-      let parentName = PlacesUtils.bookmarks.getItemTitle(parent);
+      let parentName = parent.title;
       if (guidMap[parentName] == null)
         guidMap[parentName] = {};
 
       // If the entry already exists, remember that there are explicit dupes.
       let entry = new String(guid);
       entry.hasDupe = guidMap[parentName][key] != null;
 
       // Remember this item's GUID for its parent-name/key pair.
--- a/services/sync/tests/unit/test_bookmark_engine.js
+++ b/services/sync/tests/unit/test_bookmark_engine.js
@@ -391,17 +391,19 @@ add_test(function test_bookmark_guidMap_
     PlacesUtils.bookmarks.toolbarFolder, "Folder 1", 0);
   let itemGUID    = store.GUIDForId(itemID);
   let itemPayload = store.createRecord(itemGUID).cleartext;
   coll.insert(itemGUID, encryptPayload(itemPayload));
 
   engine.lastSync = 1;   // So we don't back up.
 
   // Make building the GUID map fail.
-  store.getAllIDs = function () { throw "Nooo"; };
+
+  let pbt = PlacesUtils.promiseBookmarksTree;
+  PlacesUtils.promiseBookmarksTree = function() { return Promise.reject("Nooo"); };
 
   // Ensure that we throw when accessing _guidMap.
   engine._syncStartup();
   _("No error.");
   do_check_false(engine._guidMapFailed);
 
   _("We get an error if building _guidMap fails in use.");
   let err;
@@ -417,16 +419,17 @@ add_test(function test_bookmark_guidMap_
   err = undefined;
   try {
     engine._processIncoming();
   } catch (ex) {
     err = ex;
   }
   do_check_eq(err, "Nooo");
 
+  PlacesUtils.promiseBookmarksTree = pbt;
   server.stop(run_next_test);
 });
 
 add_test(function test_bookmark_is_taggable() {
   let engine = new BookmarksEngine(Service);
   let store = engine._store;
 
   do_check_true(store.isTaggable("bookmark"));
--- a/services/sync/tests/unit/test_bookmark_invalid.js
+++ b/services/sync/tests/unit/test_bookmark_invalid.js
@@ -7,56 +7,16 @@ Cu.import("resource://services-sync/serv
 Cu.import("resource://services-sync/util.js");
 
 Service.engineManager.register(BookmarksEngine);
 
 var engine = Service.engineManager.get("bookmarks");
 var store = engine._store;
 var tracker = engine._tracker;
 
-// Return a promise resolved when the specified message is written to the
-// bookmarks engine log.
-function promiseLogMessage(messagePortion) {
-  return new Promise(resolve => {
-    let appender;
-    let log = Log.repository.getLogger("Sync.Engine.Bookmarks");
-
-    function TestAppender() {
-      Log.Appender.call(this);
-    }
-    TestAppender.prototype = Object.create(Log.Appender.prototype);
-    TestAppender.prototype.doAppend = function(message) {
-      if (message.indexOf(messagePortion) >= 0) {
-        log.removeAppender(appender);
-        resolve();
-      }
-    };
-    TestAppender.prototype.level = Log.Level.Debug;
-    appender = new TestAppender();
-    log.addAppender(appender);
-  });
-}
-
-// Returns a promise that resolves if the specified ID does *not* exist and
-// rejects if it does.
-function promiseNoItem(itemId) {
-  return new Promise((resolve, reject) => {
-    try {
-      PlacesUtils.bookmarks.getFolderIdForItem(itemId);
-      reject("fetching the item didn't fail");
-    } catch (ex) {
-      if (ex.result == Cr.NS_ERROR_ILLEGAL_VALUE) {
-        resolve("item doesn't exist");
-      } else {
-        reject("unexpected exception: " + ex);
-      }
-    }
-  });
-}
-
 add_task(function* test_ignore_invalid_uri() {
   _("Ensure that we don't die with invalid bookmarks.");
 
   // First create a valid bookmark.
   let bmid = PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId,
                                                   Services.io.newURI("http://example.com/", null, null),
                                                   PlacesUtils.bookmarks.DEFAULT_INDEX,
                                                   "the title");
@@ -65,23 +25,19 @@ add_task(function* test_ignore_invalid_u
   yield PlacesUtils.withConnectionWrapper("test_ignore_invalid_uri", Task.async(function* (db) {
     yield db.execute(
       `UPDATE moz_places SET url = :url
        WHERE id = (SELECT b.fk FROM moz_bookmarks b
        WHERE b.id = :id LIMIT 1)`,
       { id: bmid, url: "<invalid url>" });
   }));
 
-  // DB is now "corrupt" - setup a log appender to capture what we log.
-  let promiseMessage = promiseLogMessage('Deleting bookmark with invalid URI. url="<invalid url>"');
-  // This should work and log our invalid id.
+  // Ensure that this doesn't throw even though the DB is now in a bad state (a
+  // bookmark has an illegal url).
   engine._buildGUIDMap();
-  yield promiseMessage;
-  // And we should have deleted the item.
-  yield promiseNoItem(bmid);
 });
 
 add_task(function* test_ignore_missing_uri() {
   _("Ensure that we don't die with a bookmark referencing an invalid bookmark id.");
 
   // First create a valid bookmark.
   let bmid = PlacesUtils.bookmarks.insertBookmark(PlacesUtils.unfiledBookmarksFolderId,
                                                   Services.io.newURI("http://example.com/", null, null),
@@ -91,21 +47,17 @@ add_task(function* test_ignore_missing_u
   // Now update moz_bookmarks to reference a non-existing places ID
   yield PlacesUtils.withConnectionWrapper("test_ignore_missing_uri", Task.async(function* (db) {
     yield db.execute(
       `UPDATE moz_bookmarks SET fk = 999999
        WHERE id = :id`
       , { id: bmid });
   }));
 
-  // DB is now "corrupt" - bookmarks will fail to locate a string url to log
-  // and use "<not found>" as a placeholder.
-  let promiseMessage = promiseLogMessage('Deleting bookmark with invalid URI. url="<not found>"');
+  // Ensure that this doesn't throw even though the DB is now in a bad state (a
+  // bookmark has an illegal url).
   engine._buildGUIDMap();
-  yield promiseMessage;
-  // And we should have deleted the item.
-  yield promiseNoItem(bmid);
 });
 
 function run_test() {
   initTestLogging('Trace');
   run_next_test();
 }