Bug 1110101 - Bookmarks.remove doesn't remove folder contents properly. r=mak.
authorAsaf Romano <mano@mozilla.com>
Fri, 12 Dec 2014 13:02:05 +0200
changeset 219484 a22165f1193abccaba0d3a840539b6a00a65e4b1
parent 219483 1e535a5222fb97c9d9362ffcb6da36c232768bec
child 219485 4dc5c2aa5ef8b25bb8e180a17e28b772b973bd4f
push idunknown
push userunknown
push dateunknown
reviewersmak
bugs1110101
milestone37.0a1
Bug 1110101 - Bookmarks.remove doesn't remove folder contents properly. r=mak.
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js
toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
--- a/toolkit/components/places/Bookmarks.jsm
+++ b/toolkit/components/places/Bookmarks.jsm
@@ -451,93 +451,25 @@ let Bookmarks = Object.freeze({
    *
    * Note that roots are preserved, only their children will be removed.
    *
    * @return {Promise} resolved when the removal is complete.
    * @resolves once the removal is complete.
    */
   eraseEverything: Task.async(function* () {
     let db = yield DBConnPromised;
-
     yield db.executeTransaction(function* () {
-      let rows = yield db.executeCached(
-        `WITH RECURSIVE
-         descendants(did) AS (
-           SELECT b.id FROM moz_bookmarks b
-           JOIN moz_bookmarks p ON b.parent = p.id
-           WHERE p.guid IN ( :toolbarGuid, :menuGuid, :unfiledGuid )
-           UNION ALL
-           SELECT id FROM moz_bookmarks
-           JOIN descendants ON parent = did
-         )
-         SELECT b.id AS _id, b.parent AS _parentId, b.position AS 'index',
-                b.type, url, b.guid, p.guid AS parentGuid, b.dateAdded,
-                b.lastModified, b.title, p.parent AS _grandParentId,
-                NULL AS _childCount, NULL AS keyword
-         FROM moz_bookmarks b
-         JOIN moz_bookmarks p ON p.id = b.parent
-         LEFT JOIN moz_places h ON b.fk = h.id
-         WHERE b.id IN descendants
-        `, { menuGuid: this.menuGuid, toolbarGuid: this.toolbarGuid,
-             unfiledGuid: this.unfiledGuid });
-      let items = rowsToItemsArray(rows);
-
-      yield db.executeCached(
-        `WITH RECURSIVE
-         descendants(did) AS (
-           SELECT b.id FROM moz_bookmarks b
-           JOIN moz_bookmarks p ON b.parent = p.id
-           WHERE p.guid IN ( :toolbarGuid, :menuGuid, :unfiledGuid )
-           UNION ALL
-           SELECT id FROM moz_bookmarks
-           JOIN descendants ON parent = did
-         )
-         DELETE FROM moz_bookmarks WHERE id IN descendants
-        `, { menuGuid: this.menuGuid, toolbarGuid: this.toolbarGuid,
-             unfiledGuid: this.unfiledGuid });
-
-      // Clenup orphans.
-      yield removeOrphanAnnotations(db);
-      yield removeOrphanKeywords(db);
-
-      // TODO (Bug 1087576): this may leave orphan tags behind.
-
-      // Update roots' lastModified.
-      yield db.executeCached(
-        `UPDATE moz_bookmarks SET lastModified = :time
-         WHERE id IN (SELECT id FROM moz_bookmarks
-                      WHERE guid IN ( :rootGuid, :toolbarGuid, :menuGuid, :unfiledGuid ))
-        `, { time: toPRTime(new Date()), rootGuid: this.rootGuid,
-             menuGuid: this.menuGuid, toolbarGuid: this.toolbarGuid,
-             unfiledGuid: this.unfiledGuid });
-
-      let urls = [for (item of items) if (item.url) item.url];
-      updateFrecency(db, urls).then(null, Cu.reportError);
-
-      // Send onItemRemoved notifications to listeners.
-      // TODO (Bug 1087580): this should send a single clear bookmarks
-      // notification rather than notifying for each bookmark.
-
-      // Notify listeners in reverse order to serve children before parents.
-      let observers = PlacesUtils.bookmarks.getObservers();
-      for (let item of items.reverse()) {
-        let uri = item.hasOwnProperty("url") ? toURI(item.url) : null;
-        notify(observers, "onItemRemoved", [ item._id, item._parentId,
-                                             item.index, item.type, uri,
-                                             item.guid, item.parentGuid ]);
-
-        let isUntagging = item._grandParentId == PlacesUtils.tagsFolderId;
-        if (isUntagging) {
-          for (let entry of (yield fetchBookmarksByURL(item))) {
-            notify(observers, "onItemChanged", [ entry._id, "tags", false, "",
-                                                 toPRTime(entry.lastModified),
-                                                 entry.type, entry._parentId,
-                                                 entry.guid, entry.parentGuid ]);
-          }
-        }
+      const folderGuids = [this.toolbarGuid, this.menuGuid, this.unfiledGuid];
+      yield removeFoldersContents(db, folderGuids);
+      const time = toPRTime(new Date());
+      for (let folderGuid of folderGuids) {
+        yield db.executeCached(
+          `UPDATE moz_bookmarks SET lastModified = :time
+           WHERE id IN (SELECT id FROM moz_bookmarks WHERE guid = :folderGuid )
+          `, { folderGuid, time });
       }
     }.bind(this));
   }),
 
   /**
    * Fetches information about a bookmark-item.
    *
    * REMARK: any successful call to this method resolves to a single
@@ -1021,16 +953,20 @@ function* fetchBookmarksByKeyword(info) 
 // Remove implementation.
 
 function* removeBookmark(item) {
   let db = yield DBConnPromised;
 
   let isUntagging = item._grandParentId == PlacesUtils.tagsFolderId;
 
   yield db.executeTransaction(function* transaction() {
+    // If it's a folder, remove its contents first.
+    if (item.type == Bookmarks.TYPE_FOLDER)
+      yield removeFoldersContents(db, [item.guid]);
+
     // Remove annotations first.  If it's a tag, we can avoid paying that cost.
     if (!isUntagging) {
       // We don't go through the annotations service for this cause otherwise
       // we'd get a pointless onItemChanged notification and it would also
       // set lastModified to an unexpected value.
       yield removeAnnotationsForItem(db, item._id);
     }
 
@@ -1409,8 +1345,88 @@ let setAncestorsLastModified = Task.asyn
        JOIN ancestors ON id = aid
        WHERE type = :type
      )
      UPDATE moz_bookmarks SET lastModified = :time
      WHERE id IN ancestors
     `, { guid: folderGuid, type: Bookmarks.TYPE_FOLDER,
          time: toPRTime(time) });
 });
+
+/**
+ * Remove all descendants of one or more bookmark folders.
+ *
+ * @param db
+ *        the Sqlite.jsm connection handle.
+ * @param folderGuids
+ *        array of folder guids.
+ */
+let removeFoldersContents =
+Task.async(function* (db, folderGuids) {
+  let itemsRemoved = [];
+  for (let folderGuid of folderGuids) {
+    let rows = yield db.executeCached(
+      `WITH RECURSIVE
+       descendants(did) AS (
+         SELECT b.id FROM moz_bookmarks b
+         JOIN moz_bookmarks p ON b.parent = p.id
+         WHERE p.guid = :folderGuid
+         UNION ALL
+         SELECT id FROM moz_bookmarks
+         JOIN descendants ON parent = did
+       )
+       SELECT b.id AS _id, b.parent AS _parentId, b.position AS 'index',
+              b.type, url, b.guid, p.guid AS parentGuid, b.dateAdded,
+              b.lastModified, b.title, p.parent AS _grandParentId,
+              NULL AS _childCount, NULL AS keyword
+       FROM moz_bookmarks b
+       JOIN moz_bookmarks p ON p.id = b.parent
+       LEFT JOIN moz_places h ON b.fk = h.id
+       WHERE b.id IN descendants`, { folderGuid });
+
+    itemsRemoved = itemsRemoved.concat(rowsToItemsArray(rows));
+
+    yield db.executeCached(
+      `WITH RECURSIVE
+       descendants(did) AS (
+         SELECT b.id FROM moz_bookmarks b
+         JOIN moz_bookmarks p ON b.parent = p.id
+         WHERE p.guid = :folderGuid
+         UNION ALL
+         SELECT id FROM moz_bookmarks
+         JOIN descendants ON parent = did
+       )
+       DELETE FROM moz_bookmarks WHERE id IN descendants`, { folderGuid });
+  }
+
+  // Cleanup orphans.
+  yield removeOrphanAnnotations(db);
+  yield removeOrphanKeywords(db);
+
+  // TODO (Bug 1087576): this may leave orphan tags behind.
+
+  let urls = [for (item of itemsRemoved) if (item.url) item.url];
+  updateFrecency(db, urls).then(null, Cu.reportError);
+
+  // Send onItemRemoved notifications to listeners.
+  // TODO (Bug 1087580): for the case of eraseEverything, this should send a
+  // single clear bookmarks notification rather than notifying for each
+  // bookmark.
+
+  // Notify listeners in reverse order to serve children before parents.
+  let observers = PlacesUtils.bookmarks.getObservers();
+  for (let item of itemsRemoved.reverse()) {
+    let uri = item.hasOwnProperty("url") ? toURI(item.url) : null;
+    notify(observers, "onItemRemoved", [ item._id, item._parentId,
+                                         item.index, item.type, uri,
+                                         item.guid, item.parentGuid ]);
+
+    let isUntagging = item._grandParentId == PlacesUtils.tagsFolderId;
+    if (isUntagging) {
+      for (let entry of (yield fetchBookmarksByURL(item))) {
+        notify(observers, "onItemChanged", [ entry._id, "tags", false, "",
+                                             toPRTime(entry.lastModified),
+                                             entry.type, entry._parentId,
+                                             entry.guid, entry.parentGuid ]);
+      }
+    }
+  }
+});
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js
@@ -310,16 +310,56 @@ add_task(function* remove_bookmark_tag_n
                                   tag.url, tag.guid, tag.parentGuid ] },
                    { name: "onItemChanged",
                      arguments: [ itemId, "tags", false, "",
                                   bm.lastModified, bm.type, parentId,
                                   bm.guid, bm.parentGuid ] }
                  ]);
 });
 
+add_task(function* remove_folder_notification() {
+  let folder1 = yield PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+                                                     parentGuid: PlacesUtils.bookmarks.unfiledGuid });
+  let folder1Id = yield PlacesUtils.promiseItemId(folder1.guid);
+  let folder1ParentId = yield PlacesUtils.promiseItemId(folder1.parentGuid);
+
+  let bm = yield PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+                                                parentGuid: folder1.guid,
+                                                url: new URL("http://example.com/") });
+  let bmItemId = yield PlacesUtils.promiseItemId(bm.guid);
+
+  let folder2 = yield PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+                                                     parentGuid: folder1.guid });
+  let folder2Id = yield PlacesUtils.promiseItemId(folder2.guid);
+
+  let bm2 = yield PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+                                                 parentGuid: folder2.guid,
+                                                 url: new URL("http://example.com/") });
+  let bm2ItemId = yield PlacesUtils.promiseItemId(bm2.guid);
+
+  let observer = expectNotifications();
+  yield PlacesUtils.bookmarks.remove(folder1.guid);
+
+  observer.check([ { name: "onItemRemoved",
+                     arguments: [ bm2ItemId, folder2Id, bm2.index, bm2.type,
+                                  bm2.url, bm2.guid, bm2.parentGuid ] },
+                   { name: "onItemRemoved",
+                     arguments: [ folder2Id, folder1Id, folder2.index,
+                                  folder2.type, null, folder2.guid,
+                                  folder2.parentGuid ] },
+                   { name: "onItemRemoved",
+                     arguments: [ bmItemId, folder1Id, bm.index, bm.type,
+                                  bm.url, bm.guid, bm.parentGuid ] },
+                   { name: "onItemRemoved",
+                     arguments: [ folder1Id, folder1ParentId, folder1.index,
+                                  folder1.type, null, folder1.guid,
+                                  folder1.parentGuid ] }
+                 ]);
+});
+
 add_task(function* eraseEverything_notification() {
   // Let's start from a clean situation.
   yield PlacesUtils.bookmarks.eraseEverything();
 
   let folder1 = yield PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_FOLDER,
                                                      parentGuid: PlacesUtils.bookmarks.unfiledGuid });
   let folder1Id = yield PlacesUtils.promiseItemId(folder1.guid);
   let folder1ParentId = yield PlacesUtils.promiseItemId(folder1.parentGuid);
@@ -343,39 +383,39 @@ add_task(function* eraseEverything_notif
 
   let menuBm = yield PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
                                                     parentGuid: PlacesUtils.bookmarks.menuGuid,
                                                     url: new URL("http://example.com/") });
   let menuBmId = yield PlacesUtils.promiseItemId(menuBm.guid);
   let menuBmParentId = yield PlacesUtils.promiseItemId(menuBm.parentGuid);
 
   let observer = expectNotifications();
-  let removed = yield PlacesUtils.bookmarks.eraseEverything();
+  yield PlacesUtils.bookmarks.eraseEverything();
 
   observer.check([ { name: "onItemRemoved",
-                     arguments: [ menuBmId, menuBmParentId,
-                                  menuBm.index, menuBm.type,
-                                  menuBm.url, menuBm.guid,
-                                  menuBm.parentGuid ] },
-                   { name: "onItemRemoved",
-                     arguments: [ toolbarBmId, toolbarBmParentId,
-                                  toolbarBm.index, toolbarBm.type,
-                                  toolbarBm.url, toolbarBm.guid,
-                                  toolbarBm.parentGuid ] },
-                   { name: "onItemRemoved",
                      arguments: [ folder2Id, folder2ParentId, folder2.index,
                                   folder2.type, null, folder2.guid,
                                   folder2.parentGuid ] },
                    { name: "onItemRemoved",
                      arguments: [ itemId, parentId, bm.index, bm.type,
                                   bm.url, bm.guid, bm.parentGuid ] },
                    { name: "onItemRemoved",
                      arguments: [ folder1Id, folder1ParentId, folder1.index,
                                   folder1.type, null, folder1.guid,
-                                  folder1.parentGuid ] }
+                                  folder1.parentGuid ] },
+                   { name: "onItemRemoved",
+                     arguments: [ menuBmId, menuBmParentId,
+                                  menuBm.index, menuBm.type,
+                                  menuBm.url, menuBm.guid,
+                                  menuBm.parentGuid ] },
+                    { name: "onItemRemoved",
+                     arguments: [ toolbarBmId, toolbarBmParentId,
+                                  toolbarBm.index, toolbarBm.type,
+                                  toolbarBm.url, toolbarBm.guid,
+                                  toolbarBm.parentGuid ] }
                  ]);
 });
 
 function expectNotifications() {
   let notifications = [];
   let observer = new Proxy(NavBookmarkObserver, {
     get(target, name) {
       if (name == "check") {
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
@@ -142,16 +142,30 @@ add_task(function* remove_folder() {
   Assert.equal(bm2.index, 0);
   Assert.deepEqual(bm2.dateAdded, bm2.lastModified);
   Assert.equal(bm2.type, PlacesUtils.bookmarks.TYPE_FOLDER);
   Assert.equal(bm2.title, "a folder");
   Assert.ok(!("url" in bm2));
   Assert.ok(!("keyword" in bm2));
 });
 
+add_task(function* test_nested_contents_removed() {
+  let folder1 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
+                                                     type: PlacesUtils.bookmarks.TYPE_FOLDER,
+                                                     title: "a folder" });
+  let folder2 = yield PlacesUtils.bookmarks.insert({ parentGuid: folder1.guid,
+                                                     type: PlacesUtils.bookmarks.TYPE_FOLDER,
+                                                     title: "a folder" });
+  let sep = yield PlacesUtils.bookmarks.insert({ parentGuid: folder2.guid,
+                                                 type: PlacesUtils.bookmarks.TYPE_SEPARATOR });
+  yield PlacesUtils.bookmarks.remove(folder1);
+  Assert.strictEqual((yield PlacesUtils.bookmarks.fetch(folder1.guid)), null);
+  Assert.strictEqual((yield PlacesUtils.bookmarks.fetch(folder2.guid)), null);
+  Assert.strictEqual((yield PlacesUtils.bookmarks.fetch(sep.guid)), null);
+});
 add_task(function* remove_folder_empty_title() {
   let bm1 = yield PlacesUtils.bookmarks.insert({ parentGuid: PlacesUtils.bookmarks.unfiledGuid,
                                                  type: PlacesUtils.bookmarks.TYPE_FOLDER,
                                                  title: "" });
   checkBookmarkObject(bm1);
 
   let bm2 = yield PlacesUtils.bookmarks.remove(bm1.guid);
   checkBookmarkObject(bm2);