484019 - Fix corrupt or wrong roots titles in the database and in the Library

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[James Rome]

I added a new folder called Boids. But as shown in the attached picture, my Bookmarks Menu is not names Boids, and the title field (Name: Bookmarks Menu) is uneditable!

How do I fix this?

Comment 1

[James Rome]

"Oops. typos. The Bookmarks Menu is now named Boids"

And the Boids folder was created where I had wanted it to go.

I am using 3.1 beta3

Comment 2

[Nochum Sossonko [:Natch]]

(In reply to comment #0)
> I added a new folder called Boids. But as shown in the attached picture, my
Did you forget to actually attach it?

Comment 3

[James Rome]

Attachment: picture of the problem

Comment 4

[Nochum Sossonko [:Natch]]

That's very strange... can you try the latest 1.9.1 build? ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla-1.9.1/

Comment 5

[James Rome]

It is also possible that Weave screwed it up. But it is not off, and I should be able to edit the bookmarks.

Comment 6

[James Rome]

that was "it is now off"

Comment 7

[James Rome]

I see the exact same thing with 3.5b4

Comment 8

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

I'm sorry, can someone provide Steps to Reproduce here? Is this a common problem? Do we see it all the time when adding a folder?

I filed bug 484047 which seems similar, though my bookmark toolbar didn't get renamed.

Comment 9

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

Bug 484047 was fixed by updating to a latest nightly - reporter, can you try that?

Comment 10

[James Rome]

As I said, I tried the latest nightly, and got the same result. I was still unable to change the name back.

Comment 11

[Shawn Wilsher :sdwilsh]

(In reply to comment #10)
> As I said, I tried the latest nightly, and got the same result. I was still
> unable to change the name back.
No, you said you tried it with 3.5b4 which is terribly vague.

But, looking at your screenshot, it looks like you probably have a corrupted places.sqlite

Comment 12

[Marco Bonardo [:mak]]

editing a root name is simply not doable. so the real question is, how did that change? the only bug that could potentially cause that is bug 479348 that should instead change title to (no title) if the user tires to edit a root in properties panel (and that is most likely wanted 3.5)

Comment 13

[Marco Bonardo [:mak]]

James can you tell us which add-ons you have?

Comment 14

[James Rome]

Adblock Filterset.G Updater 0.3.1.3 [DISABLED]
Adblock Plus 1.0.1
AutoAuth 1.3
ChatZilla 0.9.84
Download Statusbar 0.9.6.4
Evernote Web Clipper 3.0.0.125 [DISABLED]
Firebug 1.05 [DISABLED]
Firefox PDF Plugin for Mac OS X 1.0.3
FireGPG 0.7.5
Modify Headers 0.6.4 [DISABLED]
Nightly Tester Tools 2.0.2
NoScript 1.9.1
OpenBook 2.0.1.1
PasswordMaker 1.7.2
Sxipper 2.2.1
User Agent Switcher 0.6.11
Weave 0.2.114
Web Developer 1.1.6

Comment 15

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

Clearing blocking nomination until we can get clear steps to reproduce the problem.

Comment 16

[James Rome]

It is impossible to reproduce this of course. But what I did was to add a folder in the usual way to another folder. I am still thinking that Weave munged it somehow.

Is there any way to fix the database manually?

Comment 17

[Marco Bonardo [:mak]]

to clean up the db you can open the error console and evaluate the following:

Components.utils.import("resource://gre/modules/PlacesDBUtils.jsm"); PlacesDBUtils.maintenanceOnIdle();

but this won't fix your wrong root's name, that will need a new preventive maintenance task with a test

This is not a regression because we don't even know how the original rename did happen, sounds like add-on related, so the best we can do is fix bad titles.
Taking and morphing to fix titles.

Comment 18

[David Dahl :ddahl]

I think this also happened to my places.sqlite file, and I do not know why. My "All Bookmarks" root is itemId 27952. I think I renamed it and then fixed it and it's id changed - something like that. My db is a bit puzzling. I noticed this working on bug 416580, (which has little to do with this issue).

Comment 19

[Marco Bonardo [:mak]]

(In reply to comment #18)
> I think this also happened to my places.sqlite file, and I do not know why. My
> "All Bookmarks" root is itemId 27952.

All Bookmarks is not a root, it is a simple folder created by us for the UI, so it's more than usual for it to have an high id.

Comment 20

[Marco Bonardo [:mak]]

morphing title since i'm addressing both database and the Library.

Comment 21

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Refactored part of the left pane folder getter, to make it more readable and better commented. This will fix left pane titles.
Preventive maintenance will instead fix real roots titles (inverted with fix roots task since should come first)
Both have a dedicated test.

Comment 22

[Dietrich Ayala (:dietrich)]

Comment on attachment 373849 [details] [diff] [review]
patch v1.0


>+    // Get all items marked as being the left pane folder.  We should only have
>+    // one of them.
>+    var items = as.getItemsWithAnnotation(ORGANIZER_FOLDER_ANNO, {});
>     if (items.length > 1) {
>       // Something went wrong, we cannot have more than one left pane folder,
>-      // remove all left pane folders and generate a correct new one.
>-      items.forEach(function(aItem) {
>-        PlacesUtils.bookmarks.removeItem(aItem);
>-      });
>+      // remove all left pane folders and continue.  We will create a new one.
>+      items.forEach(bs.removeItem(aItem));

hm, should this be batched, since there's an unknown number of bad left panes? maybe overkill though...

>     }
>     else if (items.length == 1 && items[0] != -1) {
>       leftPaneRoot = items[0];
>-      // check organizer left pane version
>-      var version = PlacesUtils.annotations
>-                               .getItemAnnotation(leftPaneRoot, ORGANIZER_FOLDER_ANNO);
>+      // Check organizer left pane version.
>+      var version = as.getItemAnnotation(leftPaneRoot, ORGANIZER_FOLDER_ANNO);
>       if (version != ORGANIZER_LEFTPANE_VERSION) {
>         // If version is not valid we must rebuild the left pane.
>-        PlacesUtils.bookmarks.removeItem(leftPaneRoot);
>+        bs.removeItem(leftPaneRoot);
>         leftPaneRoot = -1;
>       }
>     }
> 
>+    var queriesTitles = {
>+      "PlacesRoot": "",
>+      "History": this.getString("OrganizerQueryHistory"),
>+      // XXX does this need its own string in places.properties?
>+      "Tags": bs.getItemTitle(PlacesUtils.tagsFolderId),

please file a followup

>+      "AllBookmarks": this.getString("OrganizerQueryAllBookmarks"),
>+      "Downloads": this.getString("OrganizerQueryDownloads"),
>+      "BookmarksToolbar": null,
>+      "BookmarksMenu": null,
>+      "UnfiledBookmarks": null
>+    };
>+
>     if (leftPaneRoot != -1) {
>-      // Build the leftPaneQueries Map
>+      // A valid left pane folder has been found.
>+      // Build the leftPaneQueries Map.  This is used to quickly access them
>+      // associating a mnemonic name to the real item ids.
>       delete this.leftPaneQueries;
>       this.leftPaneQueries = {};
>-      var items = PlacesUtils.annotations
>-                             .getItemsWithAnnotation(ORGANIZER_QUERY_ANNO, {});
>-      for (var i=0; i < items.length; i++) {
>-        var queryName = PlacesUtils.annotations
>-                                   .getItemAnnotation(items[i], ORGANIZER_QUERY_ANNO);
>+      var items = as.getItemsWithAnnotation(ORGANIZER_QUERY_ANNO, {});
>+      // While looping through queries we will also check for titles validity.
>+      for (var i = 0; i < items.length; i++) {
>+        var queryName = as.getItemAnnotation(items[i], ORGANIZER_QUERY_ANNO);
>         this.leftPaneQueries[queryName] = items[i];
>+        // Titles could have been corrupted or the user could have changed his
>+        // locale.  Check title is correctly set and eventually fix it.
>+        if (bs.getItemTitle(items[i]) != queriesTitles[queryName])
>+          bs.setItemTitle(items[i], queriesTitles[queryName]);
>       }

there's no reason for this to be done at startup. either put in delayedStartup(), running a couple of minutes after startup, or run it on a weekly timer.

>       delete this.leftPaneFolderId;
>       return this.leftPaneFolderId = leftPaneRoot;
>     }
> 
>     var self = this;
>-    const EXPIRE_NEVER = PlacesUtils.annotations.EXPIRE_NEVER;
>     var callback = {
>       runBatched: function(aUserData) {
>         delete self.leftPaneQueries;
>         self.leftPaneQueries = { };
> 
>-        // Left Pane Root Folder
>-        leftPaneRoot = PlacesUtils.bookmarks.createFolder(PlacesUtils.placesRootId, "", -1);
>-        // ensure immediate children can't be removed
>-        PlacesUtils.bookmarks.setFolderReadonly(leftPaneRoot, true);
>+        // Helper to create an organizer special query.
>+        function create_query(aQueryName, aParentId, aQueryUrl) {

instead of functions inside this method, please make these methods of the callback object. this will add even more organizational clarity.

>diff --git a/browser/components/places/tests/browser/browser_library_left_pane_fixnames.js b/browser/components/places/tests/browser/browser_library_left_pane_fixnames.js

is there already a test for the removal/recreation when there's multiple left pane roots? if not, should add that while you're here.

>diff --git a/toolkit/components/places/src/nsNavBookmarks.cpp b/toolkit/components/places/src/nsNavBookmarks.cpp
>--- a/toolkit/components/places/src/nsNavBookmarks.cpp
>+++ b/toolkit/components/places/src/nsNavBookmarks.cpp
>@@ -2253,7 +2253,11 @@ nsNavBookmarks::SetItemTitle(PRInt64 aIt
>       "UPDATE moz_bookmarks SET title = ?1, lastModified = ?2 WHERE id = ?3"),
>     getter_AddRefs(statement));
>   NS_ENSURE_SUCCESS(rv, rv);
>-  rv = statement->BindUTF8StringParameter(0, aTitle);
>+  // Support setting a null title, we support this in insertBookmark.
>+  if (aTitle.IsVoid())
>+    rv = mDBInsertBookmark->BindNullParameter(0);
>+  else
>+    rv = statement->BindUTF8StringParameter(0, aTitle);
>   NS_ENSURE_SUCCESS(rv, rv);
>   rv = statement->BindInt64Parameter(1, PR_Now());
>   NS_ENSURE_SUCCESS(rv, rv);

is there a test for this change?

Comment 23

[Dietrich Ayala (:dietrich)]

> there's no reason for this to be done at startup. either put in
> delayedStartup(), running a couple of minutes after startup, or run it on a
> weekly timer.

my mistake, this *doesn't* happen at startup (unless the bookmark sidebar is open), so should be ok.

Comment 24

[Marco Bonardo [:mak]]

(In reply to comment #22)
> (From update of attachment 373849 [details] [diff] [review])
> >+      items.forEach(bs.removeItem(aItem));
> 
> hm, should this be batched, since there's an unknown number of bad left panes?
> maybe overkill though...

not exactly unknown, we had a bug that could cause having 2 items instead of one, that should be the maximum number of items. I think would be overkill.

> >+    var queriesTitles = {
> >+      "PlacesRoot": "",
> >+      "History": this.getString("OrganizerQueryHistory"),
> >+      // XXX does this need its own string in places.properties?
> >+      "Tags": bs.getItemTitle(PlacesUtils.tagsFolderId),
> 
> please file a followup

filed Bug 489681

/browser_library_left_pane_fixnames.js b/browser/components/places/tests/browser/browser_library_left_pane_fixnames.js
> 
> is there already a test for the removal/recreation when there's multiple left
> pane roots? if not, should add that while you're here.

I already added that test with bug 466422

> >diff --git a/toolkit/components/places/src/nsNavBookmarks.cpp b/toolkit/components/places/src/nsNavBookmarks.cpp
> >-  rv = statement->BindUTF8StringParameter(0, aTitle);
> >+  // Support setting a null title, we support this in insertBookmark.
> >+  if (aTitle.IsVoid())
> >+    rv = mDBInsertBookmark->BindNullParameter(0);
> >+  else
> >+    rv = statement->BindUTF8StringParameter(0, aTitle);
> >   NS_ENSURE_SUCCESS(rv, rv);
> >   rv = statement->BindInt64Parameter(1, PR_Now());
> >   NS_ENSURE_SUCCESS(rv, rv);
> 
> is there a test for this change?

Actually the above test will fail if this code acts wrong (i catched that exactly this way), but i'll also add a really small dedicated xpcshell test, just in case...

Comment 25

[Marco Bonardo [:mak]]

Attachment: patch v1.1

addressed comments, added a dedicated test for setting a null title on a bookmark.

Comment 26

[Dietrich Ayala (:dietrich)]

Comment on attachment 374190 [details] [diff] [review]
patch v1.1


>+    // Get all items marked as being the left pane folder.  We should only have
>+    // one of them.
>+    var items = as.getItemsWithAnnotation(ORGANIZER_FOLDER_ANNO, {});
>     if (items.length > 1) {
>       // Something went wrong, we cannot have more than one left pane folder,
>-      // remove all left pane folders and generate a correct new one.
>-      items.forEach(function(aItem) {
>-        PlacesUtils.bookmarks.removeItem(aItem);
>-      });
>+      // remove all left pane folders and continue.  We will create a new one.
>+      items.forEach(bs.removeItem(aItem));

shouldn't this be: items.forEach(bs.removeItem)

does this code actually work as-is?! for example:

[0,1,2].forEach(alert(aItem));

throws "aItem is not defined"

this also would seem to indicate that the test is not checking this codepath properly.

>+    var queriesTitles = {
>+      "PlacesRoot": "",
>+      "History": this.getString("OrganizerQueryHistory"),
>+      // XXX does this need its own string in places.properties?
>+      "Tags": bs.getItemTitle(PlacesUtils.tagsFolderId),
>+      "AllBookmarks": this.getString("OrganizerQueryAllBookmarks"),
>+      "Downloads": this.getString("OrganizerQueryDownloads"),
>+      "BookmarksToolbar": null,
>+      "BookmarksMenu": null,
>+      "UnfiledBookmarks": null
>+    };
>+

note the bug # in the XXX comment, and s/XXX/TODO/

r=me with these fixed

Comment 27

[Marco Bonardo [:mak]]

Attachment: patch v1.2

good catch, no obviously that would have not work and would have cause all browser chrome tests to fail.
Since i'm not lazy today, i've added another test just for checking multiple left panes case, sounds like the old test did not exist (the attached test in the other bug was testing only one part of the fix. My bad.), it's quite similar to the other tests on the same part, so not asking further review.

Comment 28

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/b52c15e23bd2

Comment 29

[Marco Bonardo [:mak]]

backed out due to a failure in browser chrome tests
http://hg.mozilla.org/mozilla-central/rev/8f5f0cf6c611

Comment 30

[James Rome]

So can I use this somehow to fix my corrupted database?

Comment 31

[Marco Bonardo [:mak]]

Attachment: patch v1.3

i've moved the multiple left pane folders test to a chrome test, since it was polluting browser chrome tests values.
I just pushed this to the tryserver, waiting for green.

Comment 32

[Marco Bonardo [:mak]]

Comment on attachment 374924 [details] [diff] [review]
patch v1.3

tryserver went green, just asking final check on the changes in tests.

Comment 33

[Dietrich Ayala (:dietrich)]

Comment on attachment 374924 [details] [diff] [review]
patch v1.3

>diff --git a/toolkit/components/places/tests/unit/test_bookmarks_setNullTitle.js b/toolkit/components/places/tests/unit/test_bookmarks_setNullTitle.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/components/places/tests/unit/test_bookmarks_setNullTitle.js
>@@ -0,0 +1,77 @@
>+/* -*- Mode: Java; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim:set ts=2 sw=2 sts=2 et: */
>+/* ***** BEGIN LICENSE BLOCK *****
>+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
>+ *
>+ * The contents of this file are subject to the Mozilla Public License Version
>+ * 1.1 (the "License"); you may not use this file except in compliance with
>+ * the License. You may obtain a copy of the License at
>+ * http://www.mozilla.org/MPL/
>+ *
>+ * Software distributed under the License is distributed on an "AS IS" basis,
>+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
>+ * for the specific language governing rights and limitations under the
>+ * License.
>+ *
>+ * The Original Code is Places unit test code.
>+ *
>+ * The Initial Developer of the Original Code is
>+ * Mozilla Corporation.
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Drew Willcoxon <[e-mail]> (Original Author)

ahem.

Comment 34

[Marco Bonardo [:mak]]

Attachment: patchv1.4

bad copy headers!

Comment 35

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/313b7c5a27f5

Comment 36

[Dão Gottwald [:dao]]

Comment on attachment 375026 [details] [diff] [review]
patchv1.4

>+      // While looping through queries we will also check for titles validity.
>+      for (var i = 0; i < items.length; i++) {
>+        var queryName = as.getItemAnnotation(items[i], ORGANIZER_QUERY_ANNO);
>+        Components.utils.reportError(items[i] + " " + queryName);

Was this intended to land?

Comment 37

[Marco Bonardo [:mak]]

no, it was not, i'll fix it with a small changeset, thanks.

Comment 38

[Marco Bonardo [:mak]]

removed the wrongly pushed reportError
http://hg.mozilla.org/mozilla-central/rev/9c7d85c8390c

Comment 39

[Marco Bonardo [:mak]]

Attachment: followup, fix wrong binding.

this binding is wrong... i don't see how is possible the test passes, unless storage does not automatically bind an unbinded param to NULL. Shawn?

I'll provide a rollup patch later to ask approval for 1.9.1

Comment 40

[Shawn Wilsher :sdwilsh]

(In reply to comment #39)
> this binding is wrong... i don't see how is possible the test passes, unless
> storage does not automatically bind an unbinded param to NULL. Shawn?
Unless you do not reset, it will be bound as NULL.

Comment 41

[Shawn Wilsher :sdwilsh]

Comment on attachment 377415 [details] [diff] [review]
followup, fix wrong binding.

r=sdwilsh

Comment 42

[Marco Bonardo [:mak]]

pushed followup
http://hg.mozilla.org/mozilla-central/rev/aa601be9ee74

Comment 43

[Marco Bonardo [:mak]]

Attachment: rollup patch for 1.9.1

rollup patch containing patch and the 2 small followup changesets.
This fixes roots names on initialization and with preventive maintenance, has tests.
Notice on 1.9.1 we don't allow anymore users to rename their roots, but if they renamed them before 1.9.1 or they corrupted during 1.9.1 beta, they won't be able to fix titles without this.

Comment 44

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

Comment on attachment 377564 [details] [diff] [review]
rollup patch for 1.9.1

a191=beltzner

Comment 45

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6724b4035e12

Comment 46

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

I modified the root name with the sqlite manager manually and started the maintenance mode afterward. The root title gets corrected successfully.

Verified with builds on trunk and 1.9.1:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090525 Minefield/3.6a1pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1pre) Gecko/20090526 Shiretoko/3.5pre

Comment 47

[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