--- a/services/sync/modules/engines/bookmarks.js
+++ b/services/sync/modules/engines/bookmarks.js
@@ -33,16 +33,17 @@ const ANNOS_TO_TRACK = [PlacesSyncUtils.
PlacesUtils.LMANNO_FEEDURI, PlacesUtils.LMANNO_SITEURI];
const SERVICE_NOT_SUPPORTED = "Service not supported on this platform";
const FOLDER_SORTINDEX = 1000000;
const {
SOURCE_SYNC,
SOURCE_IMPORT,
SOURCE_IMPORT_REPLACE,
+ SOURCE_SYNC_REPARENT_REMOVED_FOLDER_CHILDREN,
} = Ci.nsINavBookmarksService;
const ORGANIZERQUERY_ANNO = "PlacesOrganizer/OrganizerQuery";
const ALLBOOKMARKS_ANNO = "AllBookmarks";
const MOBILE_ANNO = "MobileBookmarks";
// Roots that should be deleted from the server, instead of applied locally.
// This matches `AndroidBrowserBookmarksRepositorySession::forbiddenGUID`,
@@ -54,17 +55,18 @@ const FORBIDDEN_INCOMING_IDS = ["pinned"
// children of the Places root, to avoid orphaning left pane queries and other
// descendants of custom roots.
const FORBIDDEN_INCOMING_PARENT_IDS = ["pinned", "readinglist"];
// 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];
+const IGNORED_SOURCES = [SOURCE_SYNC, SOURCE_IMPORT, SOURCE_IMPORT_REPLACE,
+ SOURCE_SYNC_REPARENT_REMOVED_FOLDER_CHILDREN];
function isSyncedRootNode(node) {
return node.root == "bookmarksMenuFolder" ||
node.root == "unfiledBookmarksFolder" ||
node.root == "toolbarFolder" ||
node.root == "mobileFolder";
}
@@ -486,62 +488,41 @@ BookmarksEngine.prototype = {
this._store._childrenToOrder = {};
this._store.clearPendingDeletions();
},
_deletePending() {
// Delete pending items -- See the comment above BookmarkStore's deletePending
let newlyModified = Async.promiseSpinningly(this._store.deletePending());
- let now = this._tracker._now();
- this._log.debug("Deleted pending items", newlyModified);
- for (let modifiedSyncID of newlyModified) {
- if (!this._modified.has(modifiedSyncID)) {
- this._modified.set(modifiedSyncID, { timestamp: now, deleted: false });
- }
+ if (newlyModified) {
+ this._log.debug("Deleted pending items", newlyModified);
+ this._modified.insert(newlyModified);
}
},
- // We avoid reviving folders since reviving them properly would require
- // reviving their children as well. Unfortunately, this is the wrong choice
- // in the case of a bookmark restore where wipeServer failed -- if the
- // server has the folder as deleted, we *would* want to reupload this folder.
- // This is mitigated by the fact that we move any undeleted children to the
- // grandparent when deleting the parent.
_shouldReviveRemotelyDeletedRecord(item) {
- let kind = Async.promiseSpinningly(
- PlacesSyncUtils.bookmarks.getKindForSyncId(item.id));
- if (kind === PlacesSyncUtils.bookmarks.KINDS.FOLDER) {
- return false;
- }
-
- // In addition to preventing the deletion of this record (handled by the caller),
- // we need to mark the parent of this record for uploading next sync, in order
- // to ensure its children array is accurate.
let modifiedTimestamp = this._modified.getModifiedTimestamp(item.id);
if (!modifiedTimestamp) {
// We only expect this to be called with items locally modified, so
// something strange is going on - play it safe and don't revive it.
this._log.error("_shouldReviveRemotelyDeletedRecord called on unmodified item: " + item.id);
return false;
}
- let localID = this._store.idForGUID(item.id);
- let localParentID = PlacesUtils.bookmarks.getFolderIdForItem(localID);
- let localParentSyncID = this._store.GUIDForId(localParentID);
-
- this._log.trace(`Reviving item "${item.id}" and marking parent ${localParentSyncID} as modified.`);
-
- if (!this._modified.has(localParentSyncID)) {
- this._modified.set(localParentSyncID, {
- timestamp: modifiedTimestamp,
- deleted: false
- });
+ // In addition to preventing the deletion of this record (handled by the caller),
+ // we use `touch` to mark the parent of this record for uploading next sync, in order
+ // to ensure its children array is accurate. If `touch` returns new change records,
+ // we revive the item and insert the changes into the current changeset.
+ let newChanges = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.touch(item.id));
+ if (newChanges) {
+ this._modified.insert(newChanges);
+ return true;
}
- return true
+ return false;
},
_processIncoming: function (newitems) {
try {
SyncEngine.prototype._processIncoming.call(this, newitems);
} finally {
try {
this._deletePending();
@@ -634,18 +615,17 @@ BookmarksEngine.prototype = {
getValidator() {
return new BookmarkValidator();
}
};
function BookmarksStore(name, engine) {
Store.call(this, name, engine);
- this._foldersToDelete = new Set();
- this._atomsToDelete = new Set();
+ this._itemsToDelete = new Set();
// Explicitly nullify our references to our cached services so we don't leak
Svc.Obs.add("places-shutdown", function() {
for (let query in this._stmts) {
let stmt = this._stmts[query];
stmt.finalize();
}
this._stmts = {};
}, this);
@@ -710,29 +690,18 @@ BookmarksStore.prototype = {
let item = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.insert(info));
if (item) {
this._log.debug(`Created ${item.kind} ${item.syncId} under ${
item.parentSyncId}`, item);
}
},
remove: function BStore_remove(record) {
- if (PlacesSyncUtils.bookmarks.isRootSyncID(record.id)) {
- this._log.warn("Refusing to remove special folder " + record.id);
- return;
- }
- let recordKind = Async.promiseSpinningly(
- PlacesSyncUtils.bookmarks.getKindForSyncId(record.id));
- let isFolder = recordKind === PlacesSyncUtils.bookmarks.KINDS.FOLDER;
- this._log.trace(`Buffering removal of item "${record.id}" of type "${recordKind}".`);
- if (isFolder) {
- this._foldersToDelete.add(record.id);
- } else {
- this._atomsToDelete.add(record.id);
- }
+ this._log.trace(`Buffering removal of item "${record.id}".`);
+ this._itemsToDelete.add(record.id);
},
update: function BStore_update(record) {
let info = record.toSyncBookmark();
let item = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.update(info));
if (item) {
this._log.debug(`Updated ${item.kind} ${item.syncId} under ${
item.parentSyncId}`, item);
@@ -755,131 +724,47 @@ BookmarksStore.prototype = {
// (This includes any bookmark that was not a child of the folder at the
// time the deletion was recorded, and also bookmarks restored from a backup).
// - Don't undelete any bookmark without ensuring the server structure
// includes it (see `BookmarkEngine.prototype._shouldReviveRemotelyDeletedRecord`)
//
// This leads the following approach:
//
// - Additions, moves, and updates are processed before deletions.
- // - To do this, all deletion operations are buffered during a sync. Folders
- // we plan on deleting have their sync id's stored in `this._foldersToDelete`,
- // and non-folders we plan on deleting have their sync id's stored in
- // `this._atomsToDelete`.
+ // - To do this, all deletion operations are buffered in `this._itemsToDelete`
+ // during a sync.
// - The exception to this is the moves that occur to fix the order of bookmark
// children, which are performed after we process deletions.
// - Non-folders are deleted before folder deletions, so that when we process
// folder deletions we know the correct state.
// - Remote deletions always win for folders, but do not result in recursive
// deletion of children. This is a hack because we're not able to distinguish
// between value changes and structural changes to folders, and we don't even
// have the old server record to compare to. See `BookmarkEngine`'s
// `_shouldReviveRemotelyDeletedRecord` method.
// - When a folder is deleted, its remaining children are moved in order to
// their closest living ancestor. If this is interrupted (unlikely, but
// possible given that we don't perform this operation in a transaction),
// we revive the folder.
// - Remote deletions can lose for non-folders, but only until we handle
// bookmark restores correctly (removing stale state from the server -- this
// is to say, if bug 1230011 is fixed, we should never revive bookmarks).
+ //
+ // See `PlacesSyncUtils.bookmarks.remove` for the implementation.
deletePending: Task.async(function* deletePending() {
- yield this._deletePendingAtoms();
- let guidsToUpdate = yield this._deletePendingFolders();
+ let guidsToUpdate = yield PlacesSyncUtils.bookmarks.remove([...this._itemsToDelete]);
this.clearPendingDeletions();
return guidsToUpdate;
}),
clearPendingDeletions() {
- this._foldersToDelete.clear();
- this._atomsToDelete.clear();
- },
-
- _deleteAtom: Task.async(function* _deleteAtom(syncID) {
- try {
- let info = yield PlacesSyncUtils.bookmarks.remove(syncID, {
- preventRemovalOfNonEmptyFolders: true
- });
- this._log.trace(`Removed item ${syncID} with type ${info.type}`);
- } catch (ex) {
- // Likely already removed.
- this._log.trace(`Error removing ${syncID}`, ex);
- }
- }),
-
- _deletePendingAtoms() {
- return Promise.all(
- [...this._atomsToDelete.values()]
- .map(syncID => this._deleteAtom(syncID)));
+ this._itemsToDelete.clear();
},
- // Returns an array of sync ids that need updates.
- _deletePendingFolders: Task.async(function* _deletePendingFolders() {
- // To avoid data loss, we don't want to just delete the folder outright,
- // so we buffer folder deletions and process them at the end (now).
- //
- // At this point, any member in the folder that remains is either a folder
- // pending deletion (which we'll get to in this function), or an item that
- // should not be deleted. To avoid deleting these items, we first move them
- // to the parent of the folder we're about to delete.
- let needUpdate = new Set();
- for (let syncId of this._foldersToDelete) {
- let childSyncIds = yield PlacesSyncUtils.bookmarks.fetchChildSyncIds(syncId);
- if (!childSyncIds.length) {
- // No children -- just delete the folder.
- yield this._deleteAtom(syncId)
- continue;
- }
- // We could avoid some redundant work here by finding the nearest
- // grandparent who isn't present in `this._toDelete`...
-
- let grandparentSyncId = this.GUIDForId(
- PlacesUtils.bookmarks.getFolderIdForItem(
- this.idForGUID(PlacesSyncUtils.bookmarks.syncIdToGuid(syncId))));
-
- this._log.trace(`Moving ${childSyncIds.length} children of "${syncId}" to ` +
- `grandparent "${grandparentSyncId}" before deletion.`);
-
- // Move children out of the parent and into the grandparent
- yield Promise.all(childSyncIds.map(child => PlacesSyncUtils.bookmarks.update({
- syncId: child,
- parentSyncId: grandparentSyncId
- })));
-
- // Delete the (now empty) parent
- try {
- yield PlacesSyncUtils.bookmarks.remove(syncId, {
- preventRemovalOfNonEmptyFolders: true
- });
- } catch (e) {
- // We failed, probably because someone added something to this folder
- // between when we got the children and now (or the database is corrupt,
- // or something else happened...) This is unlikely, but possible. To
- // avoid corruption in this case, we need to reupload the record to the
- // server.
- //
- // (Ideally this whole operation would be done in a transaction, and this
- // wouldn't be possible).
- needUpdate.add(syncId);
- }
-
- // Add children (for parentid) and grandparent (for children list) to set
- // of records needing an update, *unless* they're marked for deletion.
- if (!this._foldersToDelete.has(grandparentSyncId)) {
- needUpdate.add(grandparentSyncId);
- }
- for (let childSyncId of childSyncIds) {
- if (!this._foldersToDelete.has(childSyncId)) {
- needUpdate.add(childSyncId);
- }
- }
- }
- return [...needUpdate];
- }),
-
// Create a record starting from the weave id (places guid)
createRecord: function createRecord(id, collection) {
let item = Async.promiseSpinningly(PlacesSyncUtils.bookmarks.fetch(id));
if (!item) { // deleted item
let record = new PlacesItem(collection, id);
record.deleted = true;
return record;
}
--- a/services/sync/tests/unit/test_bookmark_store.js
+++ b/services/sync/tests/unit/test_bookmark_store.js
@@ -500,30 +500,30 @@ add_task(function* test_delete_buffering
equal(PlacesUtils.bookmarks.getItemType(fxRecordId),
PlacesUtils.bookmarks.TYPE_BOOKMARK);
equal(PlacesUtils.bookmarks.getItemType(folderId),
PlacesUtils.bookmarks.TYPE_FOLDER);
equal(PlacesUtils.bookmarks.getFolderIdForItem(fxRecordId), folderId);
- ok(store._foldersToDelete.has(folder.id));
- ok(store._atomsToDelete.has(fxRecord.id));
- ok(!store._atomsToDelete.has(tbRecord.id));
+ ok(store._itemsToDelete.has(folder.id));
+ ok(store._itemsToDelete.has(fxRecord.id));
+ ok(!store._itemsToDelete.has(tbRecord.id));
_("Process pending deletions and ensure that the right things are deleted.");
- let updatedGuids = yield store.deletePending();
+ let newChangeRecords = yield store.deletePending();
- deepEqual(updatedGuids.sort(), ["get-tndrbrd1", "toolbar"]);
+ deepEqual(Object.keys(newChangeRecords).sort(), ["get-tndrbrd1", "toolbar"]);
assertDeleted(fxRecordId);
assertDeleted(folderId);
- ok(!store._foldersToDelete.has(folder.id));
- ok(!store._atomsToDelete.has(fxRecord.id));
+ ok(!store._itemsToDelete.has(folder.id));
+ ok(!store._itemsToDelete.has(fxRecord.id));
equal(PlacesUtils.bookmarks.getFolderIdForItem(tbRecordId),
PlacesUtils.bookmarks.toolbarFolder);
} finally {
_("Clean up.");
store.wipe();
}
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -111,16 +111,17 @@ var Bookmarks = Object.freeze({
* Bookmark change source constants, passed as optional properties and
* forwarded to observers. See nsINavBookmarksService.idl for an explanation.
*/
SOURCES: {
DEFAULT: Ci.nsINavBookmarksService.SOURCE_DEFAULT,
SYNC: Ci.nsINavBookmarksService.SOURCE_SYNC,
IMPORT: Ci.nsINavBookmarksService.SOURCE_IMPORT,
IMPORT_REPLACE: Ci.nsINavBookmarksService.SOURCE_IMPORT_REPLACE,
+ SYNC_REPARENT_REMOVED_FOLDER_CHILDREN: Ci.nsINavBookmarksService.SOURCE_SYNC_REPARENT_REMOVED_FOLDER_CHILDREN,
},
/**
* Special GUIDs associated with bookmark roots.
* It's guaranteed that the roots will always have these guids.
*/
rootGuid: "root________",
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -215,28 +215,116 @@ const BookmarkSyncUtils = PlacesSyncUtil
{ skipped: skippedCount,
updated: syncedChanges.length,
tombstones: syncedTombstoneGuids.length });
})
);
},
/**
- * Removes an item from the database. Options are passed through to
- * PlacesUtils.bookmarks.remove.
+ * Removes items from the database. Sync buffers incoming tombstones, and
+ * calls this method to apply them at the end of each sync. Deletion
+ * happens in three steps:
+ *
+ * 1. Remove all non-folder items. Deleting a folder on a remote client
+ * uploads tombstones for the folder and its children at the time of
+ * deletion. This preserves any new children we've added locally since
+ * the last sync.
+ * 2. Reparent remaining children to the tombstoned folder's parent. This
+ * bumps the change counter for the children and their new parent.
+ * 3. Remove the tombstoned folder. Because we don't do this in a
+ * transaction, the user might move new items into the folder before we
+ * can remove it. In that case, we keep the folder and upload the new
+ * subtree to the server.
+ *
+ * See the comment above `BookmarksStore::deletePending` for the details on
+ * why delete works the way it does.
*/
- remove: Task.async(function* (syncId, options = {}) {
- let guid = BookmarkSyncUtils.syncIdToGuid(syncId);
- if (guid in ROOT_GUID_TO_SYNC_ID) {
- BookmarkSyncLog.warn(`remove: Refusing to remove root ${syncId}`);
+ remove: Task.async(function* (syncIds) {
+ if (!syncIds.length) {
return null;
}
- return PlacesUtils.bookmarks.remove(guid, Object.assign({}, options, {
- source: SOURCE_SYNC,
- }));
+
+ let folderGuids = [];
+ for (let syncId of syncIds) {
+ if (syncId in ROOT_SYNC_ID_TO_GUID) {
+ BookmarkSyncLog.warn(`remove: Refusing to remove root ${syncId}`);
+ continue;
+ }
+ let guid = BookmarkSyncUtils.syncIdToGuid(syncId);
+ let bookmarkItem = yield PlacesUtils.bookmarks.fetch(guid);
+ if (!bookmarkItem) {
+ BookmarkSyncLog.trace(`remove: Item ${guid} already removed`);
+ continue;
+ }
+ let kind = yield getKindForItem(bookmarkItem);
+ if (kind == BookmarkSyncUtils.KINDS.FOLDER) {
+ folderGuids.push(bookmarkItem.guid);
+ continue;
+ }
+ let wasRemoved = yield deleteSyncedAtom(bookmarkItem);
+ if (wasRemoved) {
+ BookmarkSyncLog.trace(`remove: Removed item ${guid} with ` +
+ `kind ${kind}`);
+ }
+ }
+
+ for (let guid of folderGuids) {
+ let bookmarkItem = yield PlacesUtils.bookmarks.fetch(guid);
+ if (!bookmarkItem) {
+ BookmarkSyncLog.trace(`remove: Folder ${guid} already removed`);
+ continue;
+ }
+ let wasRemoved = yield deleteSyncedFolder(bookmarkItem);
+ if (wasRemoved) {
+ BookmarkSyncLog.trace(`remove: Removed folder ${bookmarkItem.guid}`);
+ }
+ }
+
+ // TODO (Bug 1313890): Refactor the bookmarks engine to pull change records
+ // before uploading, instead of returning records to merge into the engine's
+ // initial changeset.
+ return PlacesUtils.withConnectionWrapper("BookmarkSyncUtils: remove",
+ db => pullSyncChanges(db));
+ }),
+
+ /**
+ * Increments the change counter of a non-folder item and its parent. Sync
+ * calls this method to override a remote deletion for an item that's changed
+ * locally.
+ *
+ * @param syncId
+ * The sync ID to revive.
+ * @return {Promise} resolved once the change counters have been updated.
+ * @resolves to `null` if the item doesn't exist or is a folder. Otherwise,
+ * resolves to an object containing new change records for the item
+ * and its parent. The bookmarks engine merges these records into
+ * the changeset for the current sync.
+ */
+ touch: Task.async(function* (syncId) {
+ PlacesUtils.SYNC_BOOKMARK_VALIDATORS.syncId(syncId);
+ let guid = BookmarkSyncUtils.syncIdToGuid(syncId);
+
+ let bookmarkItem = yield PlacesUtils.bookmarks.fetch(guid);
+ if (!bookmarkItem) {
+ return null;
+ }
+ let kind = yield getKindForItem(bookmarkItem);
+ if (kind == BookmarkSyncUtils.KINDS.FOLDER) {
+ // We avoid reviving folders since reviving them properly would require
+ // reviving their children as well. Unfortunately, this is the wrong
+ // choice in the case of a bookmark restore where the bookmarks engine
+ // fails to wipe the server. In that case, if the server has the folder
+ // as deleted, we *would* want to reupload this folder. This is mitigated
+ // by the fact that `remove` moves any undeleted children to the
+ // grandparent when deleting the parent.
+ return null;
+ }
+ return PlacesUtils.withConnectionWrapper("BookmarkSyncUtils: touch",
+ db => touchSyncBookmark(db, bookmarkItem));
}),
/**
* Returns true for sync IDs that are considered roots.
*/
isRootSyncID(syncID) {
return ROOT_SYNC_ID_TO_GUID.hasOwnProperty(syncID);
},
@@ -555,17 +643,17 @@ function validateSyncBookmarkObject(inpu
// `pushChanges`.
function validateChangeRecord(changeRecord, behavior) {
return PlacesUtils.validateItemProperties(
PlacesUtils.SYNC_CHANGE_RECORD_VALIDATORS, changeRecord, behavior);
}
// Similar to the private `fetchBookmarksByParent` implementation in
// `Bookmarks.jsm`.
-var fetchAllChildren = Task.async(function* (db, parentGuid) {
+var fetchChildGuids = Task.async(function* (db, parentGuid) {
let rows = yield db.executeCached(`
SELECT guid
FROM moz_bookmarks
WHERE parent = (
SELECT id FROM moz_bookmarks WHERE guid = :parentGuid
)
ORDER BY position`,
{ parentGuid }
@@ -1387,16 +1475,37 @@ var pullSyncChanges = Task.async(functio
{ deletedSyncStatus: PlacesUtils.bookmarks.SYNC_STATUS.NORMAL },
row => addRowToChangeRecords(row, changeRecords));
yield markChangesAsSyncing(db, changeRecords);
return changeRecords;
});
+var touchSyncBookmark = Task.async(function* (db, bookmarkItem) {
+ if (BookmarkSyncLog.level <= Log.Level.Trace) {
+ BookmarkSyncLog.trace(
+ `touch: Reviving item "${bookmarkItem.guid}" and marking parent ` +
+ BookmarkSyncUtils.guidToSyncId(bookmarkItem.parentGuid) + ` as modified`);
+ }
+
+ // Bump the change counter of the item and its parent, so that we upload
+ // both.
+ yield db.executeCached(`
+ UPDATE moz_bookmarks SET
+ syncChangeCounter = syncChangeCounter + 1
+ WHERE guid IN (:guid, :parentGuid)`,
+ { guid: bookmarkItem.guid, parentGuid: bookmarkItem.parentGuid });
+
+ // TODO (Bug 1313890): Refactor the bookmarks engine to pull change records
+ // before uploading, instead of returning records to merge into the engine's
+ // initial changeset.
+ return pullSyncChanges(db);
+});
+
var dedupeSyncBookmark = Task.async(function* (db, localGuid, remoteGuid,
remoteParentGuid) {
let rows = yield db.executeCached(`
SELECT b.id, p.guid AS parentGuid, b.syncStatus
FROM moz_bookmarks b
JOIN moz_bookmarks p ON p.id = b.parent
WHERE b.guid = :localGuid`,
{ localGuid });
@@ -1474,16 +1583,98 @@ var dedupeSyncBookmark = Task.async(func
remoteGuid + " specifies non-existing parent " +
remoteParentGuid);
}
}
return changeRecords;
});
+// Moves a synced folder's remaining children to its parent, and deletes the
+// folder if it's empty.
+var deleteSyncedFolder = Task.async(function* (bookmarkItem) {
+ // At this point, any member in the folder that remains is either a folder
+ // pending deletion (which we'll get to in this function), or an item that
+ // should not be deleted. To avoid deleting these items, we first move them
+ // to the parent of the folder we're about to delete.
+ let db = yield PlacesUtils.promiseDBConnection();
+ let childGuids = yield fetchChildGuids(db, bookmarkItem.guid);
+ if (!childGuids.length) {
+ // No children -- just delete the folder.
+ return deleteSyncedAtom(bookmarkItem);
+ }
+
+ if (BookmarkSyncLog.level <= Log.Level.Trace) {
+ BookmarkSyncLog.trace(
+ `deleteSyncedFolder: Moving ${JSON.stringify(childGuids)} children of ` +
+ `"${bookmarkItem.guid}" to grandparent
+ "${BookmarkSyncUtils.guidToSyncId(bookmarkItem.parentGuid)}" before ` +
+ `deletion`);
+ }
+
+ // Move children out of the parent and into the grandparent
+ for (let guid of childGuids) {
+ yield PlacesUtils.bookmarks.update({
+ guid,
+ parentGuid: bookmarkItem.parentGuid,
+ index: PlacesUtils.bookmarks.DEFAULT_INDEX,
+ // `SYNC_REPARENT_REMOVED_FOLDER_CHILDREN` bumps the change counter for
+ // the child and its new parent, without incrementing the bookmark
+ // tracker's score.
+ //
+ // We intentionally don't check if the child is one we'll remove later,
+ // so it's possible we'll bump the change counter of the closest living
+ // ancestor when it's not needed. This avoids inconsistency if removal
+ // is interrupted, since we don't run this operation in a transaction.
+ source: PlacesUtils.bookmarks.SOURCES.SYNC_REPARENT_REMOVED_FOLDER_CHILDREN,
+ });
+ }
+
+ // Delete the (now empty) parent
+ try {
+ yield PlacesUtils.bookmarks.remove(bookmarkItem.guid, {
+ preventRemovalOfNonEmptyFolders: true,
+ // We don't want to bump the change counter for this deletion, because
+ // a tombstone for the folder is already on the server.
+ source: SOURCE_SYNC,
+ });
+ } catch (e) {
+ // We failed, probably because someone added something to this folder
+ // between when we got the children and now (or the database is corrupt,
+ // or something else happened...) This is unlikely, but possible. To
+ // avoid corruption in this case, we need to reupload the record to the
+ // server.
+ //
+ // (Ideally this whole operation would be done in a transaction, and this
+ // wouldn't be possible).
+ BookmarkSyncLog.trace(`deleteSyncedFolder: Error removing parent ` +
+ `${bookmarkItem.guid} after reparenting children`, e);
+ return false;
+ }
+
+ return true;
+});
+
+// Removes a synced bookmark or empty folder from the database.
+var deleteSyncedAtom = Task.async(function* (bookmarkItem) {
+ try {
+ yield PlacesUtils.bookmarks.remove(bookmarkItem.guid, {
+ preventRemovalOfNonEmptyFolders: true,
+ source: SOURCE_SYNC,
+ });
+ } catch (ex) {
+ // Likely already removed.
+ BookmarkSyncLog.trace(`deleteSyncedAtom: Error removing ` +
+ bookmarkItem.guid, ex);
+ return false;
+ }
+
+ return true;
+});
+
/**
* Updates the sync status on all "NEW" and "UNKNOWN" bookmarks to "NORMAL".
*
* We do this when pulling changes instead of in `pushChanges` to make sure
* we write tombstones if a new item is deleted after an interrupted sync. (For
* example, if a "NEW" record is uploaded or reconciled, then the app is closed
* before Sync calls `pushChanges`).
*/
--- a/toolkit/components/places/nsINavBookmarksService.idl
+++ b/toolkit/components/places/nsINavBookmarksService.idl
@@ -313,16 +313,17 @@ interface nsINavBookmarksService : nsISu
// Change source constants. These are used to distinguish changes made by
// Sync and bookmarks import from other Places consumers, though they can
// be extended to support other callers. Sources are passed as optional
// parameters to methods used by Sync, and forwarded to observers.
const unsigned short SOURCE_DEFAULT = 0;
const unsigned short SOURCE_SYNC = 1;
const unsigned short SOURCE_IMPORT = 2;
const unsigned short SOURCE_IMPORT_REPLACE = 3;
+ const unsigned short SOURCE_SYNC_REPARENT_REMOVED_FOLDER_CHILDREN = 4;
/**
* Sync status flags.
*/
// "UNKNOWN" means sync tracking information was lost because the item
// was restored from a backup, and needs to be reconciled with the server.
// This is set for migrated and restored bookmarks, and changed to "NORMAL"
// before upload.
--- a/toolkit/components/places/tests/unit/test_sync_utils.js
+++ b/toolkit/components/places/tests/unit/test_sync_utils.js
@@ -2073,8 +2073,246 @@ add_task(function* test_pushChanges() {
strictEqual(info.syncChangeCounter, 1,
"Updating new bookmark after pulling changes should bump change counter");
}
}
yield PlacesUtils.bookmarks.eraseEverything();
yield PlacesSyncUtils.bookmarks.reset();
});
+
+add_task(function* test_touch() {
+ yield ignoreChangedRoots();
+
+ strictEqual(yield PlacesSyncUtils.bookmarks.touch(makeGuid()), null,
+ "Should not revive nonexistent items");
+
+ {
+ let folder = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "folder",
+ syncId: makeGuid(),
+ parentSyncId: "menu",
+ });
+ strictEqual(yield PlacesSyncUtils.bookmarks.touch(folder.syncId), null,
+ "Should not revive folders");
+ }
+
+ {
+ let bmk = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "bookmark",
+ syncId: makeGuid(),
+ parentSyncId: "menu",
+ url: "https://mozilla.org",
+ });
+
+ let changes = yield PlacesSyncUtils.bookmarks.touch(bmk.syncId);
+ deepEqual(Object.keys(changes).sort(), [bmk.syncId, "menu"].sort(),
+ "Should return change records for revived bookmark and parent");
+ equal(changes[bmk.syncId].counter, 1,
+ "Change counter for revived bookmark should be 1");
+
+ yield setChangesSynced(changes);
+ }
+
+ // Livemarks are stored as folders, but their kinds are different, so we
+ // should still bump their change counters.
+ let { site, stopServer } = makeLivemarkServer();
+ try {
+ let livemark = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "livemark",
+ syncId: makeGuid(),
+ feed: site + "/feed/1",
+ parentSyncId: "unfiled",
+ });
+
+ let changes = yield PlacesSyncUtils.bookmarks.touch(livemark.syncId);
+ deepEqual(Object.keys(changes).sort(), [livemark.syncId, "unfiled"].sort(),
+ "Should return change records for revived livemark and parent");
+ equal(changes[livemark.syncId].counter, 1,
+ "Change counter for revived livemark should be 1");
+ } finally {
+ yield stopServer();
+ }
+
+ yield PlacesUtils.bookmarks.eraseEverything();
+ yield PlacesSyncUtils.bookmarks.reset();
+});
+
+add_task(function* test_remove() {
+ yield ignoreChangedRoots();
+
+ do_print("Insert subtree for removal");
+ let parentFolder = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "folder",
+ parentSyncId: "menu",
+ syncId: makeGuid(),
+ });
+ let childBmk = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "bookmark",
+ parentSyncId: parentFolder.syncId,
+ syncId: makeGuid(),
+ url: "https://example.com",
+ });
+ let childFolder = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "folder",
+ parentSyncId: parentFolder.syncId,
+ syncId: makeGuid(),
+ });
+ let grandChildBmk = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "bookmark",
+ parentSyncId: childFolder.syncId,
+ syncId: makeGuid(),
+ url: "https://example.edu",
+ });
+
+ do_print("Remove entire subtree");
+ yield PlacesSyncUtils.bookmarks.remove([
+ parentFolder.syncId,
+ childFolder.syncId,
+ childBmk.syncId,
+ grandChildBmk.syncId,
+ ]);
+
+ /**
+ * Even though we've removed the entire subtree, we still track the menu
+ * because we 1) removed `parentFolder`, 2) reparented `childFolder` to
+ * `menu`, and 3) removed `childFolder`.
+ *
+ * This depends on the order of the folders passed to `remove`. If we
+ * removed `childFolder` *before* `parentFolder`, we wouldn't reparent
+ * anything to `menu`.
+ *
+ * `deleteSyncedFolder` could check if it's reparenting an item that will
+ * eventually be removed, and avoid bumping the new parent's change counter.
+ * Unfortunately, that introduces inconsistencies if `deleteSyncedFolder` is
+ * interrupted by shutdown. If the server changes before the next sync,
+ * we'll never upload records for the reparented item or the new parent.
+ *
+ * Another alternative: we can try to remove folders in level order, instead
+ * of the order passed to `remove`. But that means we need a recursive query
+ * to determine the order. This is already enough of an edge case that
+ * occasionally reuploading the closest living ancestor is the simplest
+ * solution.
+ */
+ let changes = yield PlacesSyncUtils.bookmarks.pullChanges();
+ deepEqual(Object.keys(changes), ["menu"],
+ "Should track closest living ancestor of removed subtree");
+
+ yield PlacesUtils.bookmarks.eraseEverything();
+ yield PlacesSyncUtils.bookmarks.reset();
+});
+
+add_task(function* test_remove_partial() {
+ yield ignoreChangedRoots();
+
+ do_print("Insert subtree for partial removal");
+ let parentFolder = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "folder",
+ parentSyncId: PlacesUtils.bookmarks.menuGuid,
+ syncId: makeGuid(),
+ });
+ let prevSiblingBmk = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "bookmark",
+ parentSyncId: parentFolder.syncId,
+ syncId: makeGuid(),
+ url: "https://example.net",
+ });
+ let childBmk = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "bookmark",
+ parentSyncId: parentFolder.syncId,
+ syncId: makeGuid(),
+ url: "https://example.com",
+ });
+ let nextSiblingBmk = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "bookmark",
+ parentSyncId: parentFolder.syncId,
+ syncId: makeGuid(),
+ url: "https://example.org",
+ });
+ let childFolder = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "folder",
+ parentSyncId: parentFolder.syncId,
+ syncId: makeGuid(),
+ });
+ let grandChildBmk = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "bookmark",
+ parentSyncId: parentFolder.syncId,
+ syncId: makeGuid(),
+ url: "https://example.edu",
+ });
+ let grandChildSiblingBmk = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "bookmark",
+ parentSyncId: parentFolder.syncId,
+ syncId: makeGuid(),
+ url: "https://mozilla.org",
+ });
+ let grandChildFolder = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "folder",
+ parentSyncId: childFolder.syncId,
+ syncId: makeGuid(),
+ });
+ let greatGrandChildPrevSiblingBmk = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "bookmark",
+ parentSyncId: grandChildFolder.syncId,
+ syncId: makeGuid(),
+ url: "http://getfirefox.com",
+ });
+ let greatGrandChildNextSiblingBmk = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "bookmark",
+ parentSyncId: grandChildFolder.syncId,
+ syncId: makeGuid(),
+ url: "http://getthunderbird.com",
+ });
+ let menuBmk = yield PlacesSyncUtils.bookmarks.insert({
+ kind: "bookmark",
+ parentSyncId: "menu",
+ syncId: makeGuid(),
+ url: "https://example.info",
+ });
+
+ do_print("Remove subset of folders and items in subtree");
+ let changes = yield PlacesSyncUtils.bookmarks.remove([
+ parentFolder.syncId,
+ childBmk.syncId,
+ grandChildFolder.syncId,
+ grandChildBmk.syncId,
+ childFolder.syncId,
+ ]);
+ deepEqual(Object.keys(changes).sort(), [
+ // Closest living ancestor.
+ "menu",
+ // Reparented bookmarks.
+ prevSiblingBmk.syncId,
+ nextSiblingBmk.syncId,
+ grandChildSiblingBmk.syncId,
+ greatGrandChildPrevSiblingBmk.syncId,
+ greatGrandChildNextSiblingBmk.syncId,
+ ].sort(), "Should track reparented bookmarks and their closest living ancestor");
+
+ /**
+ * Reparented bookmarks should maintain their order relative to their
+ * siblings: `prevSiblingBmk` (0) should precede `nextSiblingBmk` (2) in the
+ * menu, and `greatGrandChildPrevSiblingBmk` (0) should precede
+ * `greatGrandChildNextSiblingBmk` (1).
+ */
+ let menuChildren = yield PlacesSyncUtils.bookmarks.fetchChildSyncIds(
+ PlacesUtils.bookmarks.menuGuid);
+ deepEqual(menuChildren, [
+ // Existing bookmark.
+ menuBmk.syncId,
+ // 1) Moved out of `parentFolder` to `menu`.
+ prevSiblingBmk.syncId,
+ nextSiblingBmk.syncId,
+ // 3) Moved out of `childFolder` to `menu`. After this step, `childFolder`
+ // is deleted.
+ grandChildSiblingBmk.syncId,
+ // 2) Moved out of `grandChildFolder` to `childFolder`, because we remove
+ // `grandChildFolder` *before* `childFolder`. After this step,
+ // `grandChildFolder` is deleted and `childFolder`'s children are
+ // `[grandChildSiblingBmk, greatGrandChildPrevSiblingBmk,
+ // greatGrandChildNextSiblingBmk]`.
+ greatGrandChildPrevSiblingBmk.syncId,
+ greatGrandChildNextSiblingBmk.syncId,
+ ], "Should move descendants to closest living ancestor");
+
+ yield PlacesUtils.bookmarks.eraseEverything();
+ yield PlacesSyncUtils.bookmarks.reset();
+});