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();