Working on bug 1095425, I found out the a code path that does something like this: Task.spawn(function* () { let folder1 = yield PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_FOLDER, title: "Folder 1", parentGuid: PlacesUtils.bookmarks.unfiledGuid }); let folder2 = yield PlacesUtils.bookmarks.insert({ type: PlacesUtils.bookmarks.TYPE_FOLDER, title: "Folder 2", parentGuid: folder1.guid }); yield PlacesUtils.bookmarks.remove(folder1.guid); yield PlacesUtils.bookmarks.insert(folder1); // Restoring guid for parent folder (this works) try { yield PlacesUtils.bookmarks.insert(folder2); // Restoring guid for nested folder (this fails) } catch(ex) { alert(ex.errors[0].message); } }); fails with the sqlite error "UNIQUE constraint failed: moz_bookmarks.guid". So it seems Bookmarks.remove doesn't actually remove folder1's contents.

Comment on attachment 8535085 [details] [diff] [review] patch Review of attachment 8535085 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/Bookmarks.jsm @@ +1344,5 @@ > time: toPRTime(time) }); > }); > + > +/** > + * Remove all decadents of one or more bookmark folders. ehr, descendants :) @@ +1349,5 @@ > + * > + * @param db > + * the Sqlite.jsm connection handle. > + * @param folders > + * array of folders specified by guids. "array of folder guids" @@ +1350,5 @@ > + * @param db > + * the Sqlite.jsm connection handle. > + * @param folders > + * array of folders specified by guids. > + * @param [optional] updateAncestorsLastModified I think it should be duty of the caller to do the update when needed and on the expected ancestors. why do we need to do this inside this helper? it looks fragile. @@ +1355,5 @@ > + * whether or not to update the last modified value for the emptied > + * folders and their ancestors. > + */ > +let removeFoldersContents = > +Task.async(function* (db, folders, updateAncestorsLastModified = true) { s/folders/folderGuids/ @@ +1357,5 @@ > + */ > +let removeFoldersContents = > +Task.async(function* (db, folders, updateAncestorsLastModified = true) { > + let itemsRemoved = null; > + for (let folder of folders) { s/folder/folderGuid @@ +1363,5 @@ > + `WITH RECURSIVE > + descendants(did) AS ( > + SELECT b.id FROM moz_bookmarks b > + JOIN moz_bookmarks p ON b.parent = p.id > + WHERE p.guid=:folder add whitespaces around = and s/:folder/:folderGuid/ @@ +1380,5 @@ > + let items = rowsToItemsArray(rows); > + if (itemsRemoved) > + itemsRemoved.push(...items); > + else > + itemsRemoved = items; I guess you could use .concat to simplify this? @@ +1387,5 @@ > + `WITH RECURSIVE > + descendants(did) AS ( > + SELECT b.id FROM moz_bookmarks b > + JOIN moz_bookmarks p ON b.parent = p.id > + WHERE p.guid=:folder ditto ::: toolkit/components/places/tests/bookmarks/test_bookmarks_remove.js @@ +158,5 @@ > + 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); could you please also add a notifications test to test_bookmarks_notifications? I know it's duping eraseEverything testing, but in future they may differ.

Comment on attachment 8535501 [details] [diff] [review] patch Review of attachment 8535501 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/places/tests/bookmarks/test_bookmarks_notifications.js @@ +327,5 @@ > + 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); please insert a bookmark in folder2 too, and check it is notified before it, just in case.