Bug 739339 - make desktop Sync more robust against malformed bookmark records. r=gps

services/sync/modules/engines/bookmarks.js

@@ -852,8 +852,9 @@ BookmarksStore.prototype = {
         PlacesUtils.bookmarks.changeBookmarkURI(itemId, Utils.makeURI(val));
         break;
       case "tags":
-        if (Array.isArray(val)) {
-          this._tagURI(PlacesUtils.bookmarks.getBookmarkURI(itemId), val);
+        if (Array.isArray(val) &&
+            (remoteRecordType in ["bookmark", "microsummary", "query"])) {
+          this._tagID(itemId, val);
         }
         break;
       case "keyword":
@@ -1217,15 +1218,43 @@ BookmarksStore.prototype = {
     return items;
   },
 
-  _tagURI: function BStore_tagURI(bmkURI, tags) {
+  /**
+   * Associates the URI of the item with the provided ID with the
+   * provided array of tags.
+   * If the provided ID does not identify an item with a URI,
+   * returns immediately.
+   */
+  _tagID: function _tagID(itemID, tags) {
+    if (!itemID || !tags) {
+      return;
+    }
+
+    try {
+      let u = PlacesUtils.bookmarks.getBookmarkURI(itemId);
+      _tagURI(u, tags);
+    } catch (e) {
+      // I guess it doesn't have a URI. Don't try to tag it.
+      return;
+    }
+  },
+
+  /**
+   * Associate the provided URI with the provided array of tags.
+   * If the provided URI is falsy, returns immediately.
+   */
+  _tagURI: function _tagURI(bookmarkURI, tags) {
+    if (!bookmarkURI || !tags) {
+      return;
+    }
+
     // Filter out any null/undefined/empty tags.
     tags = tags.filter(function(t) t);
 
     // Temporarily tag a dummy URI to preserve tag ids when untagging.
     let dummyURI = Utils.makeURI("about:weave#BStore_tagURI");
     PlacesUtils.tagging.tagURI(dummyURI, tags);
-    PlacesUtils.tagging.untagURI(bmkURI, null);
-    PlacesUtils.tagging.tagURI(bmkURI, tags);
+    PlacesUtils.tagging.untagURI(bookmarkURI, null);
+    PlacesUtils.tagging.tagURI(bookmarkURI, tags);
     PlacesUtils.tagging.untagURI(dummyURI, null);
   },
 

services/sync/tests/unit/test_bookmark_engine.js

@@ -418,10 +418,43 @@ add_test(function test_bookmark_guidMap_fail() {
   server.stop(run_next_test);
 });
 
+add_test(function test_bookmark_tag_but_no_uri() {
+  _("Ensure that a bookmark record with tags, but no URI, doesn't throw an exception.");
+
+  let engine = new BookmarksEngine();
+  let store = engine._store;
+
+  // We're simply checking that no exception is thrown, so
+  // no actual checks in this test.
+ 
+  store._tagURI(null, ["foo"]);
+  store._tagURI(null, null);
+  store._tagURI(Utils.makeURI("about:fake"), null);
+
+  let record = {
+    _parent:     PlacesUtils.bookmarks.toolbarFolder,
+    id:          Utils.makeGUID(),
+    description: "",
+    tags:        ["foo"],
+    title:       "Taggy tag",
+    type:        "folder"
+  };
+
+  // Because update() walks the cleartext.
+  record.cleartext = record;
+
+  store.create(record);
+  record.tags = ["bar"];
+  store.update(record);
+
+  run_next_test();
+});
 
 function run_test() {
   initTestLogging("Trace");
   Log4Moz.repository.getLogger("Sync.Engine.Bookmarks").level  = Log4Moz.Level.Trace;
+  Log4Moz.repository.getLogger("Sync.Store.Bookmarks").level   = Log4Moz.Level.Trace;
+  Log4Moz.repository.getLogger("Sync.Tracker.Bookmarks").level = Log4Moz.Level.Trace;
 
   generateNewKeys();