Bug 1293365 - PlacesUtils.bookmarks.reorder can introduce holes when reordering. r=kitcambridge
MozReview-Commit-ID: Gt1UVykwvQz
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -1178,24 +1178,25 @@ function reorderChildren(parent, ordered
// when no more existing GUIDs have been provided.
let valuesTable = children.map((child, i) => `("${child.guid}", ${i})`)
.join();
yield db.execute(
`WITH sorting(g, p) AS (
VALUES ${valuesTable}
)
UPDATE moz_bookmarks SET position = (
- SELECT CASE count(a.g) WHEN 0 THEN -position
- ELSE count(a.g) - 1
+ SELECT CASE count(*) WHEN 0 THEN -position
+ ELSE count(*) - 1
END
FROM sorting a
JOIN sorting b ON b.p <= a.p
WHERE a.g = guid
- AND parent = :parentId
- )`, { parentId: parent._id});
+ )
+ WHERE parent = :parentId
+ `, { parentId: parent._id});
// Update position of items that could have been inserted in the meanwhile.
// Since this can happen rarely and it's only done for schema coherence
// resonds, we won't notify about these changes.
yield db.executeCached(
`CREATE TEMP TRIGGER moz_bookmarks_reorder_trigger
AFTER UPDATE OF position ON moz_bookmarks
WHEN NEW.position = -1
--- a/toolkit/components/places/PlacesSyncUtils.jsm
+++ b/toolkit/components/places/PlacesSyncUtils.jsm
@@ -96,75 +96,44 @@ const BookmarkSyncUtils = PlacesSyncUtil
if (parentGuid == PlacesUtils.bookmarks.rootGuid) {
// Reordering roots doesn't make sense, but Sync will do this on the
// first sync.
return Promise.resolve();
}
return PlacesUtils.withConnectionWrapper("BookmarkSyncUtils: order",
Task.async(function* (db) {
- let children;
-
- yield db.executeTransaction(function* () {
- children = yield fetchAllChildren(db, parentGuid);
- if (!children.length) {
- return;
- }
- for (let child of children) {
- // Note the current index for notifying observers. This can
- // be removed once we switch to `reorder`.
- child.oldIndex = child.index;
- }
-
- // Reorder the list, ignoring missing children.
- let delta = 0;
- for (let i = 0; i < childGuids.length; ++i) {
- let guid = childGuids[i];
- let child = findChildByGuid(children, guid);
- if (!child) {
- delta++;
- BookmarkSyncLog.trace(`order: Ignoring missing child ${guid}`);
- continue;
- }
- let newIndex = i - delta;
- updateChildIndex(children, child, newIndex);
- }
- children.sort((a, b) => a.index - b.index);
+ let children = yield fetchAllChildren(db, parentGuid);
+ if (!children.length) {
+ return;
+ }
+ for (let child of children) {
+ // Note the current index for notifying observers. This can
+ // be removed once we switch to `reorder`.
+ child.oldIndex = child.index;
+ }
- // Update positions. We use a custom query instead of
- // `PlacesUtils.bookmarks.reorder` because `reorder` introduces holes
- // (bug 1293365). Once it's fixed, we can uncomment this code and
- // remove the transaction, query, and observer notification code.
-
- /*
- let orderedChildrenGuids = children.map(({ guid }) => guid);
- yield PlacesUtils.bookmarks.reorder(parentGuid, orderedChildrenGuids);
- */
+ // Reorder the list, ignoring missing children.
+ let delta = 0;
+ for (let i = 0; i < childGuids.length; ++i) {
+ let guid = childGuids[i];
+ let child = findChildByGuid(children, guid);
+ if (!child) {
+ delta++;
+ BookmarkSyncLog.trace(`order: Ignoring missing child ${guid}`);
+ continue;
+ }
+ let newIndex = i - delta;
+ updateChildIndex(children, child, newIndex);
+ }
+ children.sort((a, b) => a.index - b.index);
- yield db.executeCached(`WITH sorting(g, p) AS (
- VALUES ${children.map(
- (child, i) => `("${child.guid}", ${i})`
- ).join()}
- ) UPDATE moz_bookmarks SET position = (
- SELECT p FROM sorting WHERE g = guid
- ) WHERE parent = (
- SELECT id FROM moz_bookmarks WHERE guid = :parentGuid
- )`,
- { parentGuid });
- });
-
- // Notify observers.
- let observers = PlacesUtils.bookmarks.getObservers();
- for (let child of children) {
- notify(observers, "onItemMoved", [ child.id, child.parentId,
- child.oldIndex, child.parentId,
- child.index, child.type,
- child.guid, parentGuid,
- parentGuid, SOURCE_SYNC ]);
- }
+ // Update positions.
+ let orderedChildrenGuids = children.map(({ guid }) => guid);
+ yield PlacesUtils.bookmarks.reorder(parentGuid, orderedChildrenGuids);
})
);
}),
/**
* Removes an item from the database.
*/
remove: Task.async(function* (guid) {
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_reorder.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_reorder.js
@@ -37,32 +37,29 @@ add_task(function* invalid_input_throws(
add_task(function* reorder_nonexistent_guid() {
yield Assert.rejects(PlacesUtils.bookmarks.reorder("123456789012", [ "012345678901" ]),
/No folder found for the provided GUID/,
"Should throw for nonexisting guid");
});
add_task(function* reorder() {
let bookmarks = [
- { type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
- url: "http://example1.com/",
+ { url: "http://example1.com/",
parentGuid: PlacesUtils.bookmarks.unfiledGuid
},
{ type: PlacesUtils.bookmarks.TYPE_FOLDER,
parentGuid: PlacesUtils.bookmarks.unfiledGuid
},
{ type: PlacesUtils.bookmarks.TYPE_SEPARATOR,
parentGuid: PlacesUtils.bookmarks.unfiledGuid
},
- { type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
- url: "http://example2.com/",
+ { url: "http://example2.com/",
parentGuid: PlacesUtils.bookmarks.unfiledGuid
},
- { type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
- url: "http://example3.com/",
+ { url: "http://example3.com/",
parentGuid: PlacesUtils.bookmarks.unfiledGuid
}
];
let sorted = [];
for (let bm of bookmarks) {
sorted.push(yield PlacesUtils.bookmarks.insert(bm));
}
@@ -104,11 +101,77 @@ add_task(function* reorder() {
let rows = yield db.execute(
`SELECT parent
FROM moz_bookmarks
GROUP BY parent
HAVING (SUM(DISTINCT position + 1) - (count(*) * (count(*) + 1) / 2)) <> 0`);
Assert.equal(rows.length, 0, "All the bookmarks should have consistent positions");
});
-function run_test() {
- run_next_test();
-}
+add_task(function* move_and_reorder() {
+ // Start clean.
+ yield PlacesUtils.bookmarks.eraseEverything();
+
+ let bm1 = yield PlacesUtils.bookmarks.insert({
+ url: "http://example1.com/",
+ parentGuid: PlacesUtils.bookmarks.unfiledGuid
+ });
+ let f1 = yield PlacesUtils.bookmarks.insert({
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ parentGuid: PlacesUtils.bookmarks.unfiledGuid
+ });
+ let bm2 = yield PlacesUtils.bookmarks.insert({
+ url: "http://example2.com/",
+ parentGuid: f1.guid
+ });
+ let f2 = yield PlacesUtils.bookmarks.insert({
+ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+ parentGuid: PlacesUtils.bookmarks.unfiledGuid
+ });
+ let bm3 = yield PlacesUtils.bookmarks.insert({
+ url: "http://example3.com/",
+ parentGuid: f2.guid
+ });
+ let bm4 = yield PlacesUtils.bookmarks.insert({
+ url: "http://example4.com/",
+ parentGuid: f2.guid
+ });
+ let bm5 = yield PlacesUtils.bookmarks.insert({
+ url: "http://example5.com/",
+ parentGuid: f2.guid
+ });
+
+ // Invert f2 children.
+ // This is critical to reproduce the bug, cause it inverts the position
+ // compared to the natural insertion order.
+ yield PlacesUtils.bookmarks.reorder(f2.guid, [bm5.guid, bm4.guid, bm3.guid]);
+
+ bm1.parentGuid = f1.guid;
+ bm1.index = 0;
+ yield PlacesUtils.bookmarks.update(bm1);
+
+ bm1 = yield PlacesUtils.bookmarks.fetch(bm1.guid);
+ Assert.equal(bm1.index, 0);
+ bm2 = yield PlacesUtils.bookmarks.fetch(bm2.guid);
+ Assert.equal(bm2.index, 1);
+ bm3 = yield PlacesUtils.bookmarks.fetch(bm3.guid);
+ Assert.equal(bm3.index, 2);
+ bm4 = yield PlacesUtils.bookmarks.fetch(bm4.guid);
+ Assert.equal(bm4.index, 1);
+ bm5 = yield PlacesUtils.bookmarks.fetch(bm5.guid);
+ Assert.equal(bm5.index, 0);
+
+ // No-op reorder on f1 children.
+ // Nothing should change. Though, due to bug 1293365 this was causing children
+ // of other folders to get messed up.
+ yield PlacesUtils.bookmarks.reorder(f1.guid, [bm1.guid, bm2.guid]);
+
+ bm1 = yield PlacesUtils.bookmarks.fetch(bm1.guid);
+ Assert.equal(bm1.index, 0);
+ bm2 = yield PlacesUtils.bookmarks.fetch(bm2.guid);
+ Assert.equal(bm2.index, 1);
+ bm3 = yield PlacesUtils.bookmarks.fetch(bm3.guid);
+ Assert.equal(bm3.index, 2);
+ bm4 = yield PlacesUtils.bookmarks.fetch(bm4.guid);
+ Assert.equal(bm4.index, 1);
+ bm5 = yield PlacesUtils.bookmarks.fetch(bm5.guid);
+ Assert.equal(bm5.index, 0);
+});