Bookmarks outside the three default folders aren't restored

VERIFIED FIXED in Firefox 3

Status

()

P2
critical
VERIFIED FIXED
11 years ago
8 years ago

People

(Reporter: teoli, Assigned: dietrich)

Tracking

({dataloss})

Trunk
Firefox 3
dataloss
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

11 years ago
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.
(Reporter)

Comment 1

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

Comment 2

11 years ago
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 

Updated

11 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows XP → All
Hardware: PC → All

Comment 3

11 years ago
(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
Flags: blocking-firefox3?
Version: unspecified → Trunk
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.
Depends on: 404658
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
Keywords: dataloss
Duplicate of this bug: 411616

Comment 6

11 years ago
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?
(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

11 years ago
(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...
Summary: Bookmarks outside the three default folders aren't backuped. → Bookmarks outside the three default folders aren't backed up
(Assignee)

Updated

11 years ago
Depends on: 384370
(Assignee)

Updated

11 years ago
Assignee: nobody → dietrich
Priority: P3 → P2
Target Milestone: --- → Firefox 3 beta5
(Assignee)

Updated

11 years ago
Depends on: 415389
(Assignee)

Comment 9

11 years ago
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.
Summary: Bookmarks outside the three default folders aren't backed up → Bookmarks outside the three default folders aren't restored
Target Milestone: Firefox 3 beta5 → Firefox 3

Updated

11 years ago
Whiteboard: [needs status update]
(Assignee)

Updated

11 years ago
Whiteboard: [needs status update] → [has wip patch]
(Assignee)

Updated

11 years ago
Blocks: 426081
(Assignee)

Comment 11

11 years ago
Created attachment 314367 [details] [diff] [review]
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.
Attachment #313287 - Attachment is obsolete: true
(Assignee)

Comment 12

11 years ago
Created attachment 315631 [details] [diff] [review]
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
Attachment #314367 - Attachment is obsolete: true
Attachment #315631 - Flags: review?(mano)
(Assignee)

Updated

11 years ago
Whiteboard: [has wip patch] → [has patch][needs review mano]
(Assignee)

Comment 13

11 years ago
Comment on attachment 315631 [details] [diff] [review]
fix

marco, can you do first review here please?
Attachment #315631 - Flags: review?(mano) → review?(mak77)
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
Attachment #315631 - Flags: review?(mak77) → review-
(Assignee)

Updated

11 years ago
Whiteboard: [has patch][needs review mano] → [has wip patch]
(Assignee)

Comment 15

11 years ago
Created attachment 319446 [details] [diff] [review]
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.
Attachment #315631 - Attachment is obsolete: true
Attachment #319446 - Flags: review?(mano)
(Assignee)

Updated

11 years ago
Whiteboard: [has wip patch] → [has patch][needs review mano]
Comment on attachment 319446 [details] [diff] [review]
fixes comments

Discussed over #places.
Attachment #319446 - Flags: review?(mano) → review-
(Assignee)

Updated

11 years ago
Whiteboard: [has patch][needs review mano] → [has patch]
(Assignee)

Comment 17

11 years ago
Created attachment 319504 [details] [diff] [review]
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.
Attachment #319446 - Attachment is obsolete: true
(Assignee)

Comment 18

11 years ago
Created attachment 319520 [details] [diff] [review]
with test
Attachment #319504 - Attachment is obsolete: true
Attachment #319520 - Flags: review?(mano)
(Assignee)

Updated

11 years ago
Whiteboard: [has patch] → [has patch][needs review mano]

Comment 20

11 years ago
Comment on attachment 319520 [details] [diff] [review]
with test

a+ schrep
Attachment #319520 - Flags: approval1.9? → approval1.9+

Updated

11 years ago
Whiteboard: [has patch][needs review mano] → [needs landing
Whiteboard: [needs landing → [has patch][has review][has approval]
(Assignee)

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Whiteboard: [has patch][has review][has approval]

Comment 21

11 years ago
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

Updated

11 years ago
Status: RESOLVED → VERIFIED
Depends on: 450938
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.