From bc4794f1bfc9f0c15558c0880f559b53f2c92edc Mon Sep 17 00:00:00 2001
From: Richard Newman <rnewman@mozilla.com>
Date: Mon, 26 Mar 2012 18:40:28 -0700
Subject: [PATCH] Bug 739339 - make desktop Sync more robust against malformed
 bookmark records. r=gps

---
 services/sync/modules/engines/bookmarks.js    | 39 ++++++++++++++++---
 .../sync/tests/unit/test_bookmark_engine.js   | 35 ++++++++++++++++-
 2 files changed, 68 insertions(+), 6 deletions(-)

diff --git a/services/sync/modules/engines/bookmarks.js b/services/sync/modules/engines/bookmarks.js
index c9f75138273..2ddd4cf5297 100644
--- a/services/sync/modules/engines/bookmarks.js
+++ b/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);
   },
 
diff --git a/services/sync/tests/unit/test_bookmark_engine.js b/services/sync/tests/unit/test_bookmark_engine.js
index e048ad892a9..1eae5643209 100644
--- a/services/sync/tests/unit/test_bookmark_engine.js
+++ b/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.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();