405237 - deleting "Unfiled Bookmarks" or "bookmarks menu" folders breaks places organizer (should be undeletable)

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P1)

Description

[Pavel Cvrcek [:JasnaPaka]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007112405 Minefield/3.0b2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b2pre) Gecko/2007112405 Minefield/3.0b2pre

Firefox Trunk, clean profile.

Reproducible: Always

Steps to Reproduce:
1. Start Firefox and go to Bookmarks -> Show All Bookmarks.
2. In tree on left side try delete "Unfiled Bookmarks" via context menu option.
3. Close Places Organizer.
4. Open Places Organizer again and try to expand folder "All Bookmarks" in tree on left side.
5. Nothing happens.
Actual Results:  
Cannot expand folder "All Bookmarks". In Error Console I have sometimes errors:

Error: uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsINavHistoryService.executeQuery]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://browser/content/places/utils.js :: PU_getFolderContents :: line 876"  data: no]

Error: uncaught exception: [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE) [nsINavHistoryContainerResultNode.childCount]"  nsresult: "0x80040111 (NS_ERROR_NOT_AVAILABLE)"  location: "JS frame :: chrome://browser/content/places/utils.js :: PU_getURLsForContainerNode :: line 1526"  data: no]

Error: uncaught exception: [Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.getItemTitle]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)"  location: "JS frame :: chrome://browser/content/places/editBookmarkOverlay.js :: EIO__initFolderMenuList :: line 213"  data: no]

Expected Results:  
No error.

Comment 1

[Marco Bonardo [:mak]]

confirmed, this happens also if you try to delete the Bookmarks Menu folder, it deletes all the contents and then All bookmarks does not work anymore.
marking as dataloss and asking blocking since this can corrupt a profile and a user can lost all unfiled bookmarks/boockmark menu

Those items should not be detable, possible patch coming (don't know if it's the right place to fix this, it feels like it is)

we could also add some check to the delete functions in places (or directly into the query) to be sure that nothing can call a delete on a bookmark root.

Comment 2

[Marco Bonardo [:mak]]

Attachment: don't delete unfiled bookmarks and bookmarks menu in the places organizer

this adds check to controller.js

Comment 4

[Marco Bonardo [:mak]]

Attachment: optional additional checks in queries

an additional check can be added in deletes from moz_bookmarks, so we are sure that nothing can delete the roots

Comment 5

[Dietrich Ayala (:dietrich)]

we should fix this in M10.

Comment 6

[Marco Bonardo [:mak]]

Dietrich, do you think that the second patch (on queries) could be useful or is a bad idea to lock a delete query? 
the worst case is if something needs to delete a root to change or re-create it, but in that case it should create the new root, update the folder_id to the new and then delete the old one (it's no more a root so it will be succesfully deleted)... 
if you think that could be checked-in as an additional check, it will need your review too

Comment 7

[Dietrich Ayala (:dietrich)]

Comment on attachment 290167 [details] [diff] [review]
don't delete unfiled bookmarks and bookmarks menu in the places organizer

>Index: browser/components/places/content/controller.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/controller.js,v
>retrieving revision 1.186
>diff -u -8 -p -r1.186 controller.js
>--- browser/components/places/content/controller.js	20 Nov 2007 19:06:31 -0000	1.186
>+++ browser/components/places/content/controller.js	26 Nov 2007 01:35:11 -0000
>@@ -273,25 +273,31 @@ PlacesController.prototype = {
>    */
>   _hasRemovableSelection: function PC__hasRemovableSelection(aIsMoveCommand) {
>     if (!this._view.hasSelection)
>       return false;
> 
>     var nodes = this._view.getSelectionNodes();
>     var root = this._view.getResultNode();
> 
>+    var bmFolderId = PlacesUtils.bookmarksMenuFolderId;
>+    var ubFolderId = PlacesUtils.unfiledBookmarksFolderId;
>     var btFolderId = PlacesUtils.toolbarFolderId;
>+    

nit: whitespace

>     for (var i = 0; i < nodes.length; ++i) {
>       // Disallow removing the view's root node 
>       if (nodes[i] == root)
>         return false;
> 
>       // Disallow removing the toolbar folder
>       if (!aIsMoveCommand &&
>-          PlacesUtils.nodeIsFolder(nodes[i]) && nodes[i].itemId == btFolderId)
>+          PlacesUtils.nodeIsFolder(nodes[i]) && 
>+          (nodes[i].itemId == btFolderId || 

nit: whitespace on previous 2 lines

Comment 8

[Dietrich Ayala (:dietrich)]

(In reply to comment #6)
> Dietrich, do you think that the second patch (on queries) could be useful or is
> a bad idea to lock a delete query? 
> the worst case is if something needs to delete a root to change or re-create
> it, but in that case it should create the new root, update the folder_id to the
> new and then delete the old one (it's no more a root so it will be succesfully
> deleted)... 
> if you think that could be checked-in as an additional check, it will need your
> review too
> 

i'm not sure how tight we want to enforce this on the backend, due to non-fx toolkit consumers. lets wait on that, reconsider it for M11.

Comment 9

[Dietrich Ayala (:dietrich)]

Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  controller.js
new revision: 1.188; previous revision: 1.187
done

marking fixed. will file a followup for considering back-end restriction.

Comment 10

[Dietrich Ayala (:dietrich)]

(In reply to comment #9)
> 
> marking fixed. will file a followup for considering back-end restriction.
> 

filed bug 406886

Comment 11

[Asaf Romano (gone)]

This calls GetItemId trice for every node in the selection :-/

Comment 12

[Marco Bonardo [:mak]]

Mano, should that be fixed assigning nodes[i].itemId to a var and checking on that? I thought that itemId was a node property

Comment 13

[Marco Bonardo [:mak]]

Attachment: Supplimental perf fix

this fixes a comment and Mano's comment

Comment 14

[Asaf Romano (gone)]

Comment on attachment 291633 [details] [diff] [review]
Supplimental perf fix

>Index: browser/components/places/content/controller.js
>===================================================================
>RCS file: /cvsroot/mozilla/browser/components/places/content/controller.js,v
>retrieving revision 1.188
>diff -u -8 -p -r1.188 controller.js
>--- browser/components/places/content/controller.js	5 Dec 2007 00:11:00 -0000	1.188
>+++ browser/components/places/content/controller.js	5 Dec 2007 10:29:46 -0000
>@@ -275,29 +275,29 @@ PlacesController.prototype = {
>     if (!this._view.hasSelection)
>       return false;
> 
>     var nodes = this._view.getSelectionNodes();
>     var root = this._view.getResultNode();
> 
>     var bmFolderId = PlacesUtils.bookmarksMenuFolderId;
>     var ubFolderId = PlacesUtils.unfiledBookmarksFolderId;
>-    var btFolderId = PlacesUtils.toolbarFolderId;
>-    
>+    var btFolderId = PlacesUtils.toolbarFolderId;  
>+      

new trailing spaces.

>     for (var i = 0; i < nodes.length; ++i) {
>       // Disallow removing the view's root node 
>       if (nodes[i] == root)
>         return false;
> 
>-      // Disallow removing the toolbar folder
>+      // Disallow removing the toolbar, menu, unfiled folders     

more trailing space; also, please change that to "Disallow removing the toolbar, menu and unfiled-bookmarks folders"

r=mano otherwise.

Comment 15

[Asaf Romano (gone)]

FWIW, you can remove the three variables:
     var bmFolderId = PlacesUtils.bookmarksMenuFolderId;
     var ubFolderId = PlacesUtils.unfiledBookmarksFolderId;
     var btFolderId = PlacesUtils.toolbarFolderId;

and use PlacesUtils directly

Comment 16

[Marco Bonardo [:mak]]

Attachment: fix Mano's comments

fixed comments, this is ready for checkin

Comment 17

[Reed Loden [:reed]]

Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  controller.js
new revision: 1.189; previous revision: 1.188
done

Comment 18

[Dão Gottwald [:dao]]

(In reply to comment #16)
> Created an attachment (id=291648) [details]
> fix Mano's comments

the spaces mentioned by dietrich and mano are still there ...

Comment 19

[Marco Bonardo [:mak]]

i thought they were referring to lines with useless spacing like:

>+    

that were fixed

other lines are following previous code lining and other code i found in the same file. could you explain please? thank you Dao

Comment 20

[Marco Bonardo [:mak]]

Attachment: trailing spaces fix

i get it, thank you Dao, Sorry Reed for double work (will check spaces till now)

Comment 21

[Marco Bonardo [:mak]]

Attachment: more trailing spaces fix

Comment 22

[Reed Loden [:reed]]

Checking in browser/components/places/content/controller.js;
/cvsroot/mozilla/browser/components/places/content/controller.js,v  <--  controller.js
new revision: 1.190; previous revision: 1.189
done

Comment 23

[Stanislav]

I still cannot expand folder "All Bookmarks"

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007120705 Minefield/3.0b2pre

Comment 24

[Marco Bonardo [:mak]]

this bug cannot un-corrupt corrupted profiles, it is to avoid that it can happen

Comment 25

[Tony Chung [:tchung]]

Verified fix on Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9b2) Gecko/2007121014 Firefox/3.0b2.  "Unfiled bookmarks" and "Bookmarks menu" cannot be deleted from places organizer or Bookmarks Sidebar.

Comment 26

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv