Bug 1293365 - PlacesUtils.bookmarks.reorder can introduce holes when reordering. r=kitcambridge draft
authorMarco Bonardo <mbonardo@mozilla.com>
Wed, 31 Aug 2016 11:37:09 +0200
changeset 408682 db13745bd66d2a10c6c8ccde589a3b786de68a8b
parent 408681 212b8ad12f3cc173e4f9b03d8e38909fd31277b9
child 530162 767c158e698af5d93973a2d24b4840fcda984af9
push id28270
push usermak77@bonardo.net
push dateThu, 01 Sep 2016 14:22:14 +0000
reviewerskitcambridge
bugs1293365
milestone51.0a1
Bug 1293365 - PlacesUtils.bookmarks.reorder can introduce holes when reordering. r=kitcambridge MozReview-Commit-ID: Gt1UVykwvQz
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/PlacesSyncUtils.jsm
toolkit/components/places/tests/bookmarks/test_bookmarks_reorder.js
--- 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);
+});