Closed
Bug 407296
Opened 17 years ago
Closed 16 years ago
bad problems if browser.places.leftPaneFolderId points to an id that does not exist: places organizer is blank
Categories
(Firefox :: Bookmarks & History, defect, P1)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
People
(Reporter: moco, Assigned: dietrich)
References
Details
Attachments
(1 file, 2 obsolete files)
4.14 KB,
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
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...
Reporter | ||
Comment 1•17 years ago
|
||
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
Reporter | ||
Comment 2•17 years ago
|
||
> 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
Reporter | ||
Comment 3•17 years ago
|
||
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•17 years ago
|
||
(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•17 years ago
|
||
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
Reporter | ||
Comment 6•17 years ago
|
||
> 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
Reporter | ||
Comment 7•17 years ago
|
||
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.
Flags: blocking-firefox3?
Assignee | ||
Comment 8•17 years ago
|
||
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•17 years ago
|
||
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•17 years ago
|
||
s/forcemigrationDB/ForceMigrateBookmarksDB/
Comment 11•17 years ago
|
||
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);
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P3
Reporter | ||
Comment 12•17 years ago
|
||
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.
Priority: P3 → P2
Reporter | ||
Comment 13•17 years ago
|
||
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.)
Reporter | ||
Comment 14•17 years ago
|
||
> 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.
Reporter | ||
Updated•17 years ago
|
Summary: problems if browser.places.leftPaneFolderId points to an id that does not exist → bad problems if browser.places.leftPaneFolderId points to an id that does not exist: places organizer is blank
Reporter | ||
Comment 15•17 years ago
|
||
> 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);"
Priority: P2 → P1
Comment 16•17 years ago
|
||
> 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...
Assignee | ||
Comment 17•17 years ago
|
||
(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•17 years ago
|
||
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
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → dietrich
Comment 20•17 years ago
|
||
see also bug #412211
Assignee | ||
Comment 21•17 years ago
|
||
get the type of the leftPaneRoot, and rebuild if it's non-existent or the incorrect type.
Attachment #296904 -
Flags: review?(mano)
Assignee | ||
Updated•17 years ago
|
Whiteboard: [has patch][needs review mano]
Comment 22•17 years ago
|
||
diff -w please.
Assignee | ||
Comment 23•17 years ago
|
||
Comment 24•17 years ago
|
||
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•17 years ago
|
||
Comment on attachment 296904 [details] [diff] [review] fix v1 See last comment.
Attachment #296904 -
Flags: review?(mano) → review-
Assignee | ||
Updated•17 years ago
|
Whiteboard: [has patch][needs review mano]
Assignee | ||
Comment 26•17 years ago
|
||
Attachment #296904 -
Attachment is obsolete: true
Attachment #296982 -
Attachment is obsolete: true
Attachment #299026 -
Flags: review?(mano)
Assignee | ||
Updated•17 years ago
|
Whiteboard: [has patch][needs review mano]
Comment 27•17 years ago
|
||
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.
Attachment #299026 -
Flags: review?(mano) → review+
Assignee | ||
Comment 28•16 years ago
|
||
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
Whiteboard: [has patch][needs review mano]
Assignee | ||
Updated•16 years ago
|
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 29•16 years ago
|
||
Has anyone run into this since the fix went in? I'm not sure how to reproduce it.
Comment 30•15 years ago
|
||
I've been bouncing back and betwen builds and branches and haven't had an troubles with Places.
Status: RESOLVED → VERIFIED
Comment 31•15 years ago
|
||
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.
Description
•