Bug 1297941 - eraseEverything may send notifications in the wrong order, causing an invalid GuidHelper cache. r=markh draft
authorMarco Bonardo <mbonardo@mozilla.com>
Thu, 25 Aug 2016 10:41:36 +1000
changeset 406080 89fcc1ee1b2b1aec2105b5022087df6d77530b0a
parent 405457 7963ebdd52b93f96b812eff2eab8d94097147b9c
child 529575 24cabafc04d4460e36272788c76d2d1ac30110d6
push id27637
push usermak77@bonardo.net
push dateFri, 26 Aug 2016 11:06:10 +0000
reviewersmarkh
bugs1297941
milestone51.0a1
Bug 1297941 - eraseEverything may send notifications in the wrong order, causing an invalid GuidHelper cache. r=markh MozReview-Commit-ID: EolOzGkLbg9
toolkit/components/places/Bookmarks.jsm
toolkit/components/places/tests/bookmarks/test_bookmarks_eraseEverything.js
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
@@ -1429,20 +1429,20 @@ Task.async(function* (db, folderGuids, o
          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
-       FROM moz_bookmarks b
+       FROM descendants
+       JOIN moz_bookmarks b ON did = b.id
        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 });
+       LEFT JOIN moz_places h ON b.fk = h.id`, { 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
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_eraseEverything.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_eraseEverything.js
@@ -76,11 +76,37 @@ add_task(function* test_eraseEverything_
   // Ensure the roots have not been removed.
   Assert.ok(yield PlacesUtils.bookmarks.fetch(PlacesUtils.bookmarks.unfiledGuid));
   Assert.ok(yield PlacesUtils.bookmarks.fetch(PlacesUtils.bookmarks.toolbarGuid));
   Assert.ok(yield PlacesUtils.bookmarks.fetch(PlacesUtils.bookmarks.menuGuid));
   Assert.ok(yield PlacesUtils.bookmarks.fetch(PlacesUtils.bookmarks.tagsGuid));
   Assert.ok(yield PlacesUtils.bookmarks.fetch(PlacesUtils.bookmarks.rootGuid));
 });
 
-function run_test() {
-  run_next_test();
-}
+add_task(function* test_eraseEverything_reparented() {
+  // Create a folder with 1 bookmark in it...
+  let folder1 = yield PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+    type: PlacesUtils.bookmarks.TYPE_FOLDER
+  });
+  let bookmark1 = yield PlacesUtils.bookmarks.insert({
+    parentGuid: folder1.guid,
+    url: "http://example.com/"
+  });
+  // ...and a second folder.
+  let folder2 = yield PlacesUtils.bookmarks.insert({
+    parentGuid: PlacesUtils.bookmarks.toolbarGuid,
+    type: PlacesUtils.bookmarks.TYPE_FOLDER
+  });
+
+  // Reparent the bookmark to the 2nd folder.
+  bookmark1.parentGuid = folder2.guid;
+  yield PlacesUtils.bookmarks.update(bookmark1);
+
+  // Erase everything.
+  yield PlacesUtils.bookmarks.eraseEverything();
+
+  // All the above items should no longer be in the GUIDHelper cache.
+  for (let guid of [folder1.guid, bookmark1.guid, folder2.guid]) {
+    yield Assert.rejects(PlacesUtils.promiseItemId(guid),
+                         /no item found for the given GUID/);
+  }
+});
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js
@@ -358,42 +358,87 @@ add_task(function* eraseEverything_notif
                                                     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();
   yield PlacesUtils.bookmarks.eraseEverything();
 
+  // Bookmarks should always be notified before their parents.
   observer.check([ { name: "onItemRemoved",
+                     arguments: [ itemId, parentId, bm.index, bm.type,
+                                  bm.url, bm.guid, bm.parentGuid,
+                                  Ci.nsINavBookmarksService.SOURCE_DEFAULT ] },
+                   { name: "onItemRemoved",
                      arguments: [ folder2Id, folder2ParentId, folder2.index,
                                   folder2.type, null, folder2.guid,
                                   folder2.parentGuid,
                                   Ci.nsINavBookmarksService.SOURCE_DEFAULT ] },
                    { name: "onItemRemoved",
-                     arguments: [ itemId, parentId, bm.index, bm.type,
-                                  bm.url, bm.guid, bm.parentGuid,
-                                  Ci.nsINavBookmarksService.SOURCE_DEFAULT ] },
-                   { name: "onItemRemoved",
                      arguments: [ folder1Id, folder1ParentId, folder1.index,
                                   folder1.type, null, folder1.guid,
                                   folder1.parentGuid,
                                   Ci.nsINavBookmarksService.SOURCE_DEFAULT ] },
                    { name: "onItemRemoved",
                      arguments: [ menuBmId, menuBmParentId,
                                   menuBm.index, menuBm.type,
                                   menuBm.url, menuBm.guid,
                                   menuBm.parentGuid,
                                   Ci.nsINavBookmarksService.SOURCE_DEFAULT ] },
                     { name: "onItemRemoved",
                      arguments: [ toolbarBmId, toolbarBmParentId,
                                   toolbarBm.index, toolbarBm.type,
                                   toolbarBm.url, toolbarBm.guid,
                                   toolbarBm.parentGuid,
-                                  Ci.nsINavBookmarksService.SOURCE_DEFAULT ] }
+                                  Ci.nsINavBookmarksService.SOURCE_DEFAULT ] },
+                 ]);
+});
+
+add_task(function* eraseEverything_reparented_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);
+
+  let bm = yield PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
+                                                parentGuid: folder1.guid,
+                                                url: new URL("http://example.com/") });
+  let itemId = yield PlacesUtils.promiseItemId(bm.guid);
+
+  let folder2 = yield PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_FOLDER,
+                                                     parentGuid: PlacesUtils.bookmarks.unfiledGuid });
+  let folder2Id = yield PlacesUtils.promiseItemId(folder2.guid);
+  let folder2ParentId = yield PlacesUtils.promiseItemId(folder2.parentGuid);
+
+  bm.parentGuid = folder2.guid;
+  bm = yield PlacesUtils.bookmarks.update(bm);
+  let parentId = yield PlacesUtils.promiseItemId(bm.parentGuid);
+
+  let observer = expectNotifications();
+  yield PlacesUtils.bookmarks.eraseEverything();
+
+  // Bookmarks should always be notified before their parents.
+  observer.check([ { name: "onItemRemoved",
+                     arguments: [ itemId, parentId, bm.index, bm.type,
+                                  bm.url, bm.guid, bm.parentGuid,
+                                  Ci.nsINavBookmarksService.SOURCE_DEFAULT ] },
+                   { name: "onItemRemoved",
+                     arguments: [ folder2Id, folder2ParentId, folder2.index,
+                                  folder2.type, null, folder2.guid,
+                                  folder2.parentGuid,
+                                  Ci.nsINavBookmarksService.SOURCE_DEFAULT ] },
+                   { name: "onItemRemoved",
+                     arguments: [ folder1Id, folder1ParentId, folder1.index,
+                                  folder1.type, null, folder1.guid,
+                                  folder1.parentGuid,
+                                  Ci.nsINavBookmarksService.SOURCE_DEFAULT ] },
                  ]);
 });
 
 add_task(function* reorder_notification() {
   let bookmarks = [
     { type: PlacesUtils.bookmarks.TYPE_BOOKMARK,
       url: "http://example1.com/",
       parentGuid: PlacesUtils.bookmarks.unfiledGuid
--- a/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
+++ b/toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js
@@ -185,15 +185,15 @@ add_task(function* remove_separator() {
 
 add_task(function* test_nested_content_fails_when_not_allowed() {
   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" });
-  Assert.rejects(PlacesUtils.bookmarks.remove(folder1, {preventRemovalOfNonEmptyFolders: true}),
-                 /Cannot remove a non-empty folder./);
+  yield Assert.rejects(PlacesUtils.bookmarks.remove(folder1, {preventRemovalOfNonEmptyFolders: true}),
+                       /Cannot remove a non-empty folder./);
 });
 
 function run_test() {
   run_next_test();
 }