407296 - bad problems if browser.places.leftPaneFolderId points to an id that does not exist: places organizer is blank

Status: VERIFIED FIXED

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

Description

[(not reading, please use [e-mail] instead)]

problems if browser.places.leftPaneFolderId points to an id that does not exist

I'm not sure how he got into this state, but mossop sent me a places.sqlite and prefs.js that didn't match up.

(mossop ran an old build from august against a profile created with a recently nightly, and then switched back to today's nightly, so perhaps that triggered this.)

in his prefs.js, browser.places.leftPaneFolderId pointed to 465, but that was not the right value.

interesting, in his moz_anno_attributes table, he doesn't have the PlacesOrganizer/OrganizerQuery annotation.

other interesting things about his profile:

1)  his top level roots in moz_bookmarks do not have titles
2)  due to his leftPaneFolderId being bad, he sees this on the console:

Gives: Error: uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsINavHistoryService.executeQueries]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://browser/content/places/tree.xml :: load :: line 97"  data: no]

which leaves his organizer blank.

more info coming...

Comment 1

[(not reading, please use [e-mail] instead)]

	Mossop	I'm having problems with my bookmarks
	sspitzerMsgMe	what's up with your bookmarks?
	Mossop	Well it's suddenly decided to associate a live title with two bookmarks that werent before. And the bookmarks organiser shows nothing at all
	Mossop	Gives: Error: uncaught exception: [Exception... "Component returned failure code: 0x8000ffff (NS_ERROR_UNEXPECTED) [nsINavHistoryService.executeQueries]"  nsresult: "0x8000ffff (NS_ERROR_UNEXPECTED)"  location: "JS frame :: chrome://browser/content/places/tree.xml :: load :: line 97"  data: no]
	sspitzerMsgMe	that error sounds familiar, one sec
	Mossop	This might have been caused by me accidentally running this profile on a build from August :(
	sspitzerMsgMe	thinking, one sec
	sspitzerMsgMe	august
	sspitzerMsgMe	I wonder if we had a schema change since the
	sspitzerMsgMe	when you ran
	sspitzerMsgMe	did you run with a build after:
	sspitzerMsgMe	the fix for bug #406094
	sspitzerMsgMe	went in?
	sspitzerMsgMe	which was on 2007-12-04 11:26
	sspitzerMsgMe	here's something I could imagine:
	sspitzerMsgMe	you have an old profile
	Mossop	So I would have been running a build from after that fix. Then I ran a build from August with the same profile. Now I'm back on todays nightly still with the same profile
	sspitzerMsgMe	oh, I see, you went backwards to an old build
	Mossop	yeah
	sspitzerMsgMe	the last schema change we had was in july, so a aug old build shouldn't attempt to drop your places.sqlite
	sspitzerMsgMe	can you save aside your places.sqlite file?
	sspitzerMsgMe	so I can look at it?
	Mossop	Yeah have done
	sspitzerMsgMe	as for your problem, can you look at your prefs?
	sspitzerMsgMe	look for this:
	sspitzerMsgMe	browser.places
	sspitzerMsgMe	I'm interested in the
	sspitzerMsgMe	browser.places.allBookmarksFolderId and
	sspitzerMsgMe	browser.places.leftPaneFolderId
	sspitzerMsgMe	can you zip up your places.sqlite for me, and let me know the value of thse prefs?
	Mossop	AllBookmarksFolderId=468, leftPaneFolderId=465
	Mossop	I'll send you the file now

Comment 2

[(not reading, please use [e-mail] instead)]

> 1)  his top level roots in moz_bookmarks do not have titles

Note, I saw that after restoring browser.places.leftPaneFolderId to 409

This issue looked is similar to bug #406190

Comment 3

[(not reading, please use [e-mail] instead)]

separate from mossops bug, why do we need to keep the value for browser.places.leftPaneFolderId in prefs?  Couldn't this be kept in places.sqlite?  can't we determine it by annotation or store it directly?

Comment 4

[Marco Bonardo [:mak]]

(no title) for roots should be caused by the lack of a call to nsNavBookmarks::InitDefaults(); that is called by nsNavBookmarks::InitRoots if importDefaults is not set to FALSE

moving to an older build with a current profile could have called initroots with importdefaults false, and have caused new roots to be created without title... but are there in the file old roots with right titles? i had tested some time ago a build from october/early-november and it created no title roots for some reason (that now appear to be fixed)... 

Comment 5

[Dave Townsend [:mossop]]

Maybe or maybe not related but on shutdown in a debug build I get:

WARNING: TokensToQueries(), ignoring unknown key: : file /Users/dave/mozilla/source/trunk/mozilla/toolkit/components/places/src/nsNavHistoryQuery.cpp, line 771
WARNING: resolveNullBookmarkTitles: file /Users/dave/mozilla/source/trunk/mozilla/toolkit/components/places/src/nsNavHistoryQuery.cpp, line 772
WARNING: TokensToQueries(), ignoring unknown key: : file /Users/dave/mozilla/source/trunk/mozilla/toolkit/components/places/src/nsNavHistoryQuery.cpp, line 771
WARNING: resolveNullBookmarkTitles: file /Users/dave/mozilla/source/trunk/mozilla/toolkit/components/places/src/nsNavHistoryQuery.cpp, line 772

Comment 6

[(not reading, please use [e-mail] instead)]

> Maybe or maybe not related but on shutdown in a debug build I get:

I don't think that is related.   

You are getting that warning because you have some old place: uris (created by an earlier version of minefield).

See bug #404689 for more details

Comment 7

[(not reading, please use [e-mail] instead)]

dietrich/mano/marco:

I just hit this bug myself, but I'm not sure how.

My browser.places.leftPaneFolderId pref pointed to a folder id that did not exist.

I think we need to stop using a pref for browser.places.leftPaneFolderId, and then also address the "(no title) problem before we ship firefox.

Comment 8

[Dietrich Ayala (:dietrich)]

Yes, this should block. Using an annotation for this, as you suggested above, would at least encapsulate the solution in the db. It might be less fragile then, though we had problems with this approach for the bookmarks toolbar.

Comment 9

[Marco Bonardo [:mak]]

or on first use check if it exists, if not set it to -1 so it's recreated...

i think however that this is caused by moving through older and newer builds, maybe by a db schema change, if you move down to a previous schema the code calls forcemigrationDB that re-create the bookmarks table but maybe it's not resetting the leftpanefolderid to -1...

Comment 10

[Marco Bonardo [:mak]]

s/forcemigrationDB/ForceMigrateBookmarksDB/

Comment 11

[Marco Bonardo [:mak]]

should we set these in ForceMigrateBookmarksDB?
rv = prefs->SetBoolPref(PREF_BROWSER_IMPORT_DEFAULTS, PR_TRUE);
rv = prefs->SetBoolPref(PREF_BROWSER_CREATEDSMARTBOOKMARKS, PR_FALSE);
rv = prefs->SetIntPref(PREF_BROWSER_LEFTPANEFOLDERID, -1);

Comment 12

[(not reading, please use [e-mail] instead)]

Just hit this again, and while I was (accidentally) switching between builds, I don't think it was builds that would force db migration (ForceMigrateBookmarksDB()).

I think I was switching between my trunk debug build and Firefox 3b2.  (It's been a while since we've done a forced migration, right?)

I'm increasing the priority of this one, as I think we need to figure it out before we ship.

Comment 13

[(not reading, please use [e-mail] instead)]

I have not tried it out, but if we did hit a forced migration, it seems likely to cause us problems (as our browser.places.leftPaneFolderId pref would be out of sync.)

So upon forced migration, this seems correct:

rv = prefs->SetIntPref(PREF_BROWSER_LEFTPANEFOLDERID, -1);

About "rv = prefs->SetBoolPref(PREF_BROWSER_CREATEDSMARTBOOKMARKS, PR_FALSE);"

since bug #406114 is not fixed, our migration is going to be using bookmarks.html (and not bookmarks.postplaces.html), so I guess we'd want that.  (But as soon as we start using .json, or if that bug gets fixed, we would not want that.)

Comment 14

[(not reading, please use [e-mail] instead)]

> Using an annotation for this, as you suggested above,
> would at least encapsulate the solution in the db. It might be less fragile
> then, though we had problems with this approach for the bookmarks toolbar.

keep in mind that if we use annotations, we'd need .json backup, otherwise backup/restore would not work as we don't write/read annotations from the .html format.

Comment 15

[(not reading, please use [e-mail] instead)]

> Just hit this again, and while I was (accidentally) switching between builds, I
> don't think it was builds that would force db migration
> (ForceMigrateBookmarksDB()).

I take that back, I am hitting ForceMigrateBookmarksDB() because in my trunk build, I have bumped the schema from version 6 to version 7.

When I run fx 3 b 2, we hit the downgrade code in nsNavHistory::InitDB(), and the downgrade code in the trunk will call ForceMigrateBookmarksDB()

633       // Downgrade v1,2,4,5
634       // v3,6 have no backwards incompatible changes.
635       if (DBSchemaVersion > 2) {
636         // perform downgrade to v2
637         rv = ForceMigrateBookmarksDB(mDBConn);
638         NS_ENSURE_SUCCESS(rv, rv);
639       }

So, we're re-importing (bookmarks.html, ugh, not bookmarks.postplaces.html) and the pref for browser.places.leftPaneFolderId will no longer be correct.

This explains mossop's original issue in comment #0, when he switched back and forth between a recent build and one for last august.

I think the good news is that as long as users don't toggle between pre-M7 (when we went to schema version 6, see bug #319455), they won't hit this.

The bad news is that if I bump the schema for bug #394038, toggling between betas and future builds (nightlies, b3, RTM, anything after we bump the schema), users will hit this bug.

Note, Marco is doing a schema migration for bug #392399 as well.

MigrateV7Up() may need to call "rv = prefs->SetIntPref(PREF_BROWSER_LEFTPANEFOLDERID, -1);"

Comment 16

[Marco Bonardo [:mak]]

> MigrateV7Up() may need to call "rv =
> prefs->SetIntPref(PREF_BROWSER_LEFTPANEFOLDERID, -1);"

why migrateV7Up?, if you have v6 you already have a valid leftpaneid, the problem is when you downgrade to a version that calls ForceMigrateBookmarksDB, i think that the prefs->SetIntPref(PREF_BROWSER_LEFTPANEFOLDERID, -1) should be called every time we create a new bookmark table...

Comment 17

[Dietrich Ayala (:dietrich)]

(In reply to comment #16)
> > MigrateV7Up() may need to call "rv =
> > prefs->SetIntPref(PREF_BROWSER_LEFTPANEFOLDERID, -1);"
> 
> why migrateV7Up?, if you have v6 you already have a valid leftpaneid, the
> problem is when you downgrade to a version that calls ForceMigrateBookmarksDB,
> i think that the prefs->SetIntPref(PREF_BROWSER_LEFTPANEFOLDERID, -1) should be
> called every time we create a new bookmark table...
> 

see bug 394205, where i'm decoupling back-end upgrades from all our front-end prefs.

Comment 18

[Marco Bonardo [:mak]]

so the problem could be solved setting mImportBookmarks = PR_TRUE; in Bug 394205
yes, that is a better way, but will cause a TS regression due to _initPlaces changes, however i vote for that solution

Comment 20

[Seth Spitzer]

see also bug #412211

Comment 21

[Dietrich Ayala (:dietrich)]

Attachment: fix v1

get the type of the leftPaneRoot, and rebuild if it's non-existent or the incorrect type.

Comment 22

[Asaf Romano (gone)]

diff -w please.

Comment 23

[Dietrich Ayala (:dietrich)]

Attachment: diff -w

Comment 24

[Asaf Romano (gone)]

Dietrich, I think we should rather switch to an annotation identifier here (and just leave the old folder in current profile, there's no dataloss). Otherwise we could probably get into a state in which there are multiple items annotated with the special-queries annotations.

Comment 25

[Asaf Romano (gone)]

Comment on attachment 296904 [details] [diff] [review]
fix v1

See last comment.

Comment 26

[Dietrich Ayala (:dietrich)]

Attachment: fix using anno

Comment 27

[Asaf Romano (gone)]

Comment on attachment 299026 [details] [diff] [review]
fix using anno

>+    if (items.length != 0 && items[0] != -1)
>+      leftPaneRoot = items[0];
>+    // if the pref is set to -1 then we must create a new root because we have a new places.sqlite

This comment is obsolete.

r=mano otherwise.

Comment 28

[Dietrich Ayala (:dietrich)]

Checking in browser/components/places/content/utils.js;
/cvsroot/mozilla/browser/components/places/content/utils.js,v  <--  utils.js
new revision: 1.98; previous revision: 1.97
done

Comment 29

[Tracy Walker [:tracy]]

Has anyone run into this since the fix went in?  I'm not sure how to reproduce it.

Comment 30

[Tracy Walker [:tracy]]

I've been bouncing back and betwen builds and branches and haven't had an troubles with Places.

Comment 31

[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