417228 - Bookmarks outside the three default folders aren't restored

Status: VERIFIED FIXED

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

Description

[Jean-Yves Perrier [:teoli]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3

Bookmarks that are stored outside the "Bookmarks Toolbar", "Bookmarks Menu" and "Unfiled Bookmarks" aren't backuped, leading to dataloss (example: http://forums.mozillazine.org/viewtopic.php?t=627986 )


Reproducible: Always

Steps to Reproduce:
1.Create a bookmark called "XXXXXXXXXXXXXXX"
2.Move it, via the Library, at the top level (i.e. outside the toolbar, the menu, the unfiled bookmarks)
3.Do "Import and Backup" -> Backup
Actual Results:  
No "XXXXXXXXXXXXXXX" bookmark in the generated file

Expected Results:  
Either the bookmarks in the generated file, either the inability to put a bookmark there.

This may lead to dataloss if another problem happen. It already happened to some in beta2 -> beta3.

It is similar, though not identical (and thus not a dup) of bug 405936. A common fix may be possible.

Comment 1

[Jean-Yves Perrier [:teoli]]

Oh I forgot: people using the bookmarks sidebar are very likely to have lots of bookmarks at this position.

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Here you go: Bug 404658 should take a decision on that

why putting items there at all if you already have unfiled bookmarks to put them in?

the backup problem will be solved by new JSON backup (Bug 384370), actually bookmarks saved our of the three main folders are not saved 

Comment 3

[Alvaro Muñoz]

(In reply to comment #2)
> why putting items there at all if you already have unfiled bookmarks to put
> them in?
> 

I think unfiled isn't the best place for a Feeds folder, wich I lost with the update to beta3

Comment 4

[Mike Beltzner [:beltzner, not reading bugmail]]

I think I stated in bug 404658 that if allowing top-level folders causes problems like this, then we shouldn't do it. Setting dependency, but I agree that this blocks, since we're creating an expectation that backup is for all bookmarks. 

Either prevent these bookmarks to be created or be sure to back them up.

Comment 6

[Maaren]

I have problems with the backing up of 'unfiled bookmarks'
When I make a backup, that folder isn't included.

In the profile folder, the .html files of the bookmarks however have that folder

It is maybe not the same, but maybe it has to do with the same problem?

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to comment #6)
> I have problems with the backing up of 'unfiled bookmarks'
> When I make a backup, that folder isn't included.

Related to bug 402302?

Comment 8

[Maaren]

(In reply to comment #7)
> (In reply to comment #6)
> > I have problems with the backing up of 'unfiled bookmarks'
> > When I make a backup, that folder isn't included.
> 
> Related to bug 402302?
> 

Yes, but not on XP, on a mac Leopard and Windows Vista
I have also made a new places.sqlite after some problems...

Comment 9

[Dietrich Ayala (:dietrich)]

Bug 384370 ensures that all bookmarks are backed up to JSON backups, however it doesn't restore anything outside the tags, menu, toolbar, unfiled folders. Morphing summary to reflect this.

Comment 10

[Dietrich Ayala (:dietrich)]

Attachment: wip

Comment 11

[Dietrich Ayala (:dietrich)]

Attachment: more

- fix copy-last-backup-to-backups-dir
- make allBookmarks and leftPane getters not memo-ized
- tmp fixes for testability

need to figure out what's going when a root node of a query is deleted out from under it. right now, this patch works fine, other than leaving db cruft: it is re-creating new left pane hierarchies when a restore occurs while the organizer is open.

Comment 12

[Dietrich Ayala (:dietrich)]

Attachment: fix

- de-memoize leftPane and allBookmarks, to allow for the id to change when restoring. (accessed very rarely, so not worried too much about this)

- add observer to places.js to reset the left pane when it's deleted/re-created

Comment 13

[Dietrich Ayala (:dietrich)]

Comment on attachment 315631 [details] [diff] [review]
fix

marco, can you do first review here please?

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Comment on attachment 315631 [details] [diff] [review]
fix

the fix in bitrotted due to the leftpane versioning patch... so r- for that, however here is a code review, will do a functionality test with the next version.

+  _resetLeftPane: function() {
+    this._initFolderTree();
+    this.selectLeftPaneQuery("AllBookmarks");
+    this._oldLeftPaneId = PlacesUIUtils.leftPaneFolderId;
+  },

is not this._initFolderTree(); already setting this._oldLeftPaneId?

+    if (folder == PlacesUtils.placesRootId
+        && id == this._oldLeftPaneId) {

nit: IIRC we put && at the end on the first line as +

+    if (this._leftPaneFolderId == -1 ||
+        PlacesUtils.bookmarks.getItemIndex(this._leftPaneFolderId) == -1) {
+      var leftPaneRoot = -1;
+      var allBookmarksId;


allBookmarksId can be directly defined in the batch callback since it is only used there

-  get allBookmarksFolderId() {
-    // ensure the left-pane root is initialized;
-    this.leftPaneFolderId;
-    delete this.allBookmarksFolderId;
-    return this.allBookmarksFolderId = this.leftPaneQueries["AllBookmarks"];
+  

nit: trailing space

+          for (var i = 0; i < root.childCount; i++) {
+            var aRootItemId = root.getChild(i).itemId;

why the prepended a? is not that commonly used to decorate passed-in vars? could be confusing

+              }
+            }
+            else if ([this._utils.toolbarFolderId, this._utils.tagsFolderId,
+                      this._utils.unfiledBookmarksFolderId,
+                      this._utils.bookmarksMenuFolderId].indexOf(aRootItemId) != -1)

another check for this._utils.tagsFolderId? is not it already served by the previous if?

+            else if ([this._utils.toolbarFolderId, this._utils.tagsFolderId,
+                      this._utils.unfiledBookmarksFolderId,
+                      this._utils.bookmarksMenuFolderId].indexOf(aRootItemId) != -1)
+              this._utils.bookmarks.removeFolderChildren(aRootItemId);
+            else
+              this._utils.bookmarks.removeItem(aRootItemId);
+          }
+          root.containerOpen = false;
+        }
+      

nit: trailing space

-                var uris = [];
-                tags.forEach(function(aTag) {
-                  var tagURIs = this.tagging.getURIsForTag(aTag);
-                  for (let i in tagURIs)
-                    this.tagging.untagURI(tagURIs[i], [aTag]);
-                }, this);
+        

nit: trailing space

-                tags.forEach(function(aTag) {
-                  var tagURIs = this.tagging.getURIsForTag(aTag);
-                  for (let i in tagURIs)
-                    this.tagging.untagURI(tagURIs[i], [aTag]);
-                }, this);
+        

nit: trailing space

+                break;
+              case "toolbarFolder":
+                container = this.toolbarFolderId;
+                break;
             }
+  

nit: trailing space

+    // create a test root
+    this._folderTitle = "test folder";
+    this._folderId = 

test "root"?

+  // export json to file
+  try {
+    PlacesUtils.backupBookmarksToFile(jsonFile);
+  } catch(ex) { do_throw("couldn't export to file: " + ex); }
+
+  // restore json file
+  try {
+    PlacesUtils.restoreBookmarksFromJSONFile(jsonFile);
+  } catch(ex) { do_throw("couldn't import the exported file: " + ex); }

what about adding some "litter" in the middle, like create a new folder in the root, add some tag... to ensure that they are correctly removed on restore

Comment 15

[Dietrich Ayala (:dietrich)]

Attachment: fixes comments

note: this also changes the endupdatebatch assertion to a warning, since it can occur in valid scenarios, such as when a new observer is added *during* a batch operation.

Comment 16

[Asaf Romano (gone)]

Comment on attachment 319446 [details] [diff] [review]
fixes comments

Discussed over #places.

Comment 17

[Dietrich Ayala (:dietrich)]

Attachment: wip

alternate approach:

- allow exclusions at backup: for example, the left pane folder shouldn't be exported, if it's not going to be replaced during restore. otherwise every restore would add another invisible left pane folder hierarchy.

- allow exclusions at restore: for example, when doing a replace-and-restore, don't delete the left pane folder

this patch is mostly there, just need to write a separate test for exclusions at backup time.

Comment 18

[Dietrich Ayala (:dietrich)]

Attachment: with test

Comment 19

[Asaf Romano (gone)]

Comment on attachment 319520 [details] [diff] [review]
with test

r=mano

Comment 20

[Mike Schroepfer]

Comment on attachment 319520 [details] [diff] [review]
with test

a+ schrep

Comment 21

[Bob Uliss]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9) Gecko/2008052903 Firefox/3.0

tested on MAC - RC2 works OK

Comment 22

[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