From 14accfe33d4e1272becaa3e728fcdac558ae3682 Mon Sep 17 00:00:00 2001 From: Kit Cambridge Date: Tue, 6 Sep 2016 08:36:21 -0700 Subject: [PATCH] Bug 1299338 - Replace `ignoreAll` with change source checks in the Sync bookmarks engine. r=markh MozReview-Commit-ID: DAsTlQbFanK --- services/sync/modules/engines/bookmarks.js | 66 ++++++++----- .../sync/tests/unit/test_bookmark_engine.js | 97 +++++++++++++++++++ .../components/places/nsNavHistoryResult.cpp | 4 +- 3 files changed, 139 insertions(+), 28 deletions(-) diff --git a/services/sync/modules/engines/bookmarks.js b/services/sync/modules/engines/bookmarks.js index 7a2c779b83ddc..6366afd992d3c 100644 --- a/services/sync/modules/engines/bookmarks.js +++ b/services/sync/modules/engines/bookmarks.js @@ -27,7 +27,11 @@ const ANNOS_TO_TRACK = [BookmarkAnnos.DESCRIPTION_ANNO, BookmarkAnnos.SIDEBAR_AN const SERVICE_NOT_SUPPORTED = "Service not supported on this platform"; const FOLDER_SORTINDEX = 1000000; -const { SOURCE_SYNC } = Ci.nsINavBookmarksService; +const { + SOURCE_SYNC, + SOURCE_IMPORT, + SOURCE_IMPORT_REPLACE, +} = Ci.nsINavBookmarksService; // Maps Sync record property names to `PlacesSyncUtils` bookmark properties. const RECORD_PROPS_TO_BOOKMARK_PROPS = { @@ -42,6 +46,12 @@ const RECORD_PROPS_TO_BOOKMARK_PROPS = { feedUri: "feed", }; +// The tracker ignores changes made by bookmark import and restore, and +// changes made by Sync. We don't need to exclude `SOURCE_IMPORT`, but both +// import and restore fire `bookmarks-restore-*` observer notifications, and +// the tracker doesn't currently distinguish between the two. +const IGNORED_SOURCES = [SOURCE_SYNC, SOURCE_IMPORT, SOURCE_IMPORT_REPLACE]; + this.PlacesItem = function PlacesItem(collection, id, type) { CryptoWrapper.call(this, collection, id); this.type = type || "item"; @@ -377,9 +387,7 @@ BookmarksEngine.prototype = { SyncEngine.prototype._processIncoming.call(this, newitems); } finally { // Reorder children. - this._tracker.ignoreAll = true; this._store._orderChildren(); - this._tracker.ignoreAll = false; delete this._store._childrenToOrder; } }, @@ -905,6 +913,16 @@ function BookmarksTracker(name, engine) { BookmarksTracker.prototype = { __proto__: Tracker.prototype, + //`_ignore` checks the change source for each observer notification, so we + // don't want to let the engine ignore all changes during a sync. + get ignoreAll() { + return false; + }, + + // Define an empty setter so that the engine doesn't throw a `TypeError` + // setting a read-only property. + set ignoreAll(value) {}, + startTracking: function() { PlacesUtils.bookmarks.addObserver(this, true); Svc.Obs.add("bookmarks-restore-begin", this); @@ -925,11 +943,9 @@ BookmarksTracker.prototype = { switch (topic) { case "bookmarks-restore-begin": this._log.debug("Ignoring changes from importing bookmarks."); - this.ignoreAll = true; break; case "bookmarks-restore-success": this._log.debug("Tracking all items on successful import."); - this.ignoreAll = false; this._log.debug("Restore succeeded: wiping server and other clients."); this.engine.service.resetClient([this.name]); @@ -938,7 +954,6 @@ BookmarksTracker.prototype = { break; case "bookmarks-restore-failed": this._log.debug("Tracking all items on failed import."); - this.ignoreAll = false; break; } }, @@ -978,11 +993,15 @@ BookmarksTracker.prototype = { * Item under consideration to ignore * @param folder (optional) * Folder of the item being changed + * @param guid + * Places GUID of the item being changed + * @param source + * A change source constant from `nsINavBookmarksService::SOURCE_*`. */ - _ignore: function BMT__ignore(itemId, folder, guid) { - // Ignore unconditionally if the engine tells us to. - if (this.ignoreAll) + _ignore: function BMT__ignore(itemId, folder, guid, source) { + if (IGNORED_SOURCES.includes(source)) { return true; + } // Get the folder id if we weren't given one. if (folder == null) { @@ -1018,9 +1037,10 @@ BookmarksTracker.prototype = { onItemAdded: function BMT_onItemAdded(itemId, folder, index, itemType, uri, title, dateAdded, - guid, parentGuid) { - if (this._ignore(itemId, folder, guid)) + guid, parentGuid, source) { + if (this._ignore(itemId, folder, guid, source)) { return; + } this._log.trace("onItemAdded: " + itemId); this._add(itemId, guid); @@ -1028,8 +1048,8 @@ BookmarksTracker.prototype = { }, onItemRemoved: function (itemId, parentId, index, type, uri, - guid, parentGuid) { - if (this._ignore(itemId, parentId, guid)) { + guid, parentGuid, source) { + if (this._ignore(itemId, parentId, guid, source)) { return; } @@ -1049,9 +1069,6 @@ BookmarksTracker.prototype = { if (all.length == 0) return; - // Disable handling of notifications while changing the mobile query - this.ignoreAll = true; - let mobile = find(BookmarkAnnos.MOBILE_ANNO); let queryURI = Utils.makeURI("place:folder=" + BookmarkSpecialIds.mobile); let title = Str.sync.get("mobile.label"); @@ -1073,8 +1090,6 @@ BookmarksTracker.prototype = { else if (PlacesUtils.bookmarks.getItemTitle(mobile[0]) != title) { PlacesUtils.bookmarks.setItemTitle(mobile[0], title, SOURCE_SYNC); } - - this.ignoreAll = false; }, // This method is oddly structured, but the idea is to return as quickly as @@ -1082,11 +1097,7 @@ BookmarksTracker.prototype = { // *each change*. onItemChanged: function BMT_onItemChanged(itemId, property, isAnno, value, lastModified, itemType, parentId, - guid, parentGuid) { - // Quicker checks first. - if (this.ignoreAll) - return; - + guid, parentGuid, source) { if (isAnno && (ANNOS_TO_TRACK.indexOf(property) == -1)) // Ignore annotations except for the ones that we sync. return; @@ -1095,8 +1106,9 @@ BookmarksTracker.prototype = { if (property == "favicon") return; - if (this._ignore(itemId, parentId, guid)) + if (this._ignore(itemId, parentId, guid, source)) { return; + } this._log.trace("onItemChanged: " + itemId + (", " + property + (isAnno? " (anno)" : "")) + @@ -1106,9 +1118,11 @@ BookmarksTracker.prototype = { onItemMoved: function BMT_onItemMoved(itemId, oldParent, oldIndex, newParent, newIndex, itemType, - guid, oldParentGuid, newParentGuid) { - if (this._ignore(itemId, newParent, guid)) + guid, oldParentGuid, newParentGuid, + source) { + if (this._ignore(itemId, newParent, guid, source)) { return; + } this._log.trace("onItemMoved: " + itemId); this._add(oldParent, oldParentGuid); diff --git a/services/sync/tests/unit/test_bookmark_engine.js b/services/sync/tests/unit/test_bookmark_engine.js index 07d26be7f27f9..e9dd2eb68fd72 100644 --- a/services/sync/tests/unit/test_bookmark_engine.js +++ b/services/sync/tests/unit/test_bookmark_engine.js @@ -17,6 +17,103 @@ initTestLogging("Trace"); Service.engineManager.register(BookmarksEngine); +add_task(function* test_change_during_sync() { + _("Ensure that we track changes made during a sync."); + + let engine = new BookmarksEngine(Service); + let store = engine._store; + let server = serverForFoo(engine); + new SyncTestingInfrastructure(server.server); + + let collection = server.user("foo").collection("bookmarks"); + + Svc.Obs.notify("weave:engine:start-tracking"); + + try { + let folder1_id = PlacesUtils.bookmarks.createFolder( + PlacesUtils.bookmarks.toolbarFolder, "Folder 1", 0); + let folder1_guid = store.GUIDForId(folder1_id); + _(`Folder GUID: ${folder1_guid}`); + + let bmk1_id = PlacesUtils.bookmarks.insertBookmark( + folder1_id, Utils.makeURI("http://getthunderbird.com/"), + PlacesUtils.bookmarks.DEFAULT_INDEX, "Get Thunderbird!"); + let bmk1_guid = store.GUIDForId(bmk1_id); + _(`Thunderbird GUID: ${bmk1_guid}`); + + // Sync is synchronous, so, to simulate a bookmark change made during a + // sync, we create a server record that adds a bookmark as a side effect. + let bmk2_guid = "get-firefox1"; + let bmk3_id = -1; + { + let localRecord = new Bookmark("bookmarks", bmk2_guid); + localRecord.bmkUri = "http://getfirefox.com/"; + localRecord.description = "Firefox is awesome."; + localRecord.title = "Get Firefox!"; + localRecord.tags = ["firefox", "awesome", "browser"]; + localRecord.keyword = "awesome"; + localRecord.loadInSidebar = false; + localRecord.parentName = "Folder 1"; + localRecord.parentid = folder1_guid; + + let remoteRecord = collection.insert(bmk2_guid, encryptPayload(localRecord.cleartext)); + remoteRecord.get = function get() { + _("Inserting bookmark into local store"); + bmk3_id = PlacesUtils.bookmarks.insertBookmark( + folder1_id, Utils.makeURI("https://mozilla.org/"), + PlacesUtils.bookmarks.DEFAULT_INDEX, "Mozilla"); + + return ServerWBO.prototype.get.apply(this, arguments); + }; + } + + { + let tree = yield PlacesUtils.promiseBookmarksTree(folder1_guid); + let childGuids = tree.children.map(child => child.guid); + deepEqual(childGuids, [bmk1_guid], "Folder should have 1 child before first sync"); + } + + _("Perform first sync"); + yield sync_engine_and_validate_telem(engine, false); + + let bmk2_id = store.idForGUID(bmk2_guid); + let bmk3_guid = store.GUIDForId(bmk3_id); + _(`Mozilla GUID: ${bmk3_guid}`); + { + equal(store.GUIDForId(bmk2_id), bmk2_guid, + "Remote bookmark should be applied during first sync"); + ok(bmk3_id > -1, + "Bookmark created during first sync should exist locally"); + ok(!collection.wbo(bmk3_guid), + "Bookmark created during first sync shouldn't be uploaded yet"); + + let tree = yield PlacesUtils.promiseBookmarksTree(folder1_guid); + let childGuids = tree.children.map(child => child.guid); + deepEqual(childGuids, [bmk1_guid, bmk3_guid, bmk2_guid], + "Folder should have 3 children after first sync"); + } + + _("Perform second sync"); + yield sync_engine_and_validate_telem(engine, false); + + { + ok(collection.wbo(bmk3_guid), + "Bookmark created during first sync should be uploaded during second sync"); + + let tree = yield PlacesUtils.promiseBookmarksTree(folder1_guid); + let childGuids = tree.children.map(child => child.guid); + deepEqual(childGuids, [bmk1_guid, bmk3_guid, bmk2_guid], + "Folder should have same children after second sync"); + } + } finally { + store.wipe(); + Svc.Prefs.resetBranch(""); + Service.recordManager.clearCache(); + yield new Promise(resolve => server.stop(resolve)); + Svc.Obs.notify("weave:engine:stop-tracking"); + } +}); + add_task(function* bad_record_allIDs() { let server = new SyncServer(); server.start(); diff --git a/toolkit/components/places/nsNavHistoryResult.cpp b/toolkit/components/places/nsNavHistoryResult.cpp index 8382404d70ba3..2f3d47e5f4145 100644 --- a/toolkit/components/places/nsNavHistoryResult.cpp +++ b/toolkit/components/places/nsNavHistoryResult.cpp @@ -3965,12 +3965,12 @@ nsNavHistoryFolderResultNode::OnItemMoved(int64_t aItemId, } if (aOldParent == mTargetFolderItemId) { OnItemRemoved(aItemId, aOldParent, aOldIndex, aItemType, itemURI, - aGUID, aOldParentGUID, nsINavBookmarksService::SOURCE_DEFAULT); + aGUID, aOldParentGUID, aSource); } if (aNewParent == mTargetFolderItemId) { OnItemAdded(aItemId, aNewParent, aNewIndex, aItemType, itemURI, itemTitle, RoundedPRNow(), // This is a dummy dateAdded, not the real value. - aGUID, aNewParentGUID, nsINavBookmarksService::SOURCE_DEFAULT); + aGUID, aNewParentGUID, aSource); } } return NS_OK;