Closed
Bug 327350
Opened 18 years ago
Closed 17 years ago
initialization of places services can fail in error conditions
Categories
(Firefox :: Bookmarks & History, defect, P2)
Firefox
Bookmarks & History
Tracking
()
VERIFIED
FIXED
Firefox 3 alpha6
People
(Reporter: brettw, Assigned: dietrich)
References
Details
(Whiteboard: [swag: 1d])
Attachments
(4 files, 2 obsolete files)
2.88 KB,
patch
|
moco
:
review+
|
Details | Diff | Splinter Review |
3.97 KB,
patch
|
Details | Diff | Splinter Review | |
4.08 KB,
patch
|
moco
:
review+
|
Details | Diff | Splinter Review |
10.40 KB,
patch
|
asaf
:
review+
|
Details | Diff | Splinter Review |
The history observer is registered at the beginning, which will be left dangling on error. See bug 327331 for the history service version.
Updated•18 years ago
|
Assignee: nobody → bryner
Updated•18 years ago
|
Priority: -- → P2
Target Milestone: --- → Firefox 2 beta1
Updated•18 years ago
|
Assignee: bryner → nobody
Target Milestone: Firefox 2 beta1 → ---
Assignee | ||
Comment 1•17 years ago
|
||
(In reply to comment #0) > The history observer is registered at the beginning, which will be left > dangling on error. See bug 327331 for the history service version. > this is no longer true, but keeping this bug open until we have bulletproof handling of things like db init failure from horked sqlite file or other.
Flags: blocking-firefox3?
Target Milestone: --- → Firefox 3 alpha5
Comment 2•17 years ago
|
||
we're run into issues with nsNavHistory::Init() and nsAnnotationService::Init() failing, too. for example, we might fail to create some of the statements and then we'll fail to init the service, and nothing will work as you're unable to get the desired service. dietrich, mind if we morph this bug into bulletproofing all the places services?
Updated•17 years ago
|
Flags: blocking-firefox3? → blocking-firefox3+
Assignee | ||
Updated•17 years ago
|
Assignee: nobody → dietrich
Assignee | ||
Comment 3•17 years ago
|
||
(In reply to comment #2) > we're run into issues with nsNavHistory::Init() and nsAnnotationService::Init() > failing, too. > > for example, we might fail to create some of the statements and then we'll fail > to init the service, and nothing will work as you're unable to get the desired > service. > > dietrich, mind if we morph this bug into bulletproofing all the places > services? > > sounds good, modifying this bug to be guaranteed recovery from critical errors at service init time. this includes the following requirements for all services: - if db is corrupt, service should still run - if db is corrupt, we need to back up bookmarks.html and places.sqlite to specially named files, instead of overwriting them. in the case of bookmarks.html, we could be overwriting good w/ bad at shutdown. - if database init and statement creation fails, service should still run (?? hm, this could just cause lots of problems, and misleading bug reports)
Summary: nsNavBookmarks::Init is fragile and leaky in error conditions → initialization of places services can fail in error conditions
Assignee | ||
Comment 4•17 years ago
|
||
(In reply to comment #3) > > sounds good, modifying this bug to be guaranteed recovery from critical errors > at service init time. this includes the following requirements for all > services: > > - if db is corrupt, service should still run this is already in place, but we delete the places.sqlite file. we should probably just back it up to places.sqlite.corrupt or something. > - if db is corrupt, we need to back up bookmarks.html and places.sqlite to > specially named files, instead of overwriting them. in the case of > bookmarks.html, we could be overwriting good w/ bad at shutdown. hm, we don't want the toolkit services to know about bookmarks.html as that's /browser specific. not sure how to force this to backup yet. > - if database init and statement creation fails, service should still run (?? > hm, this could just cause lots of problems, and misleading bug reports) > ok, so this seems like a terribly bad idea. we can't guarantee any level of quality if query compilation fails, or we can't create tables.
Assignee | ||
Comment 5•17 years ago
|
||
per discussion at bug triage meeting: for A5, i'll get in code to backup the bad places.sqlite, before removing and recreating. for A6, confirm that all db initialization code is behind InitDB(), and if that initialization fails, backup places.sqlite, recreate, flip the pref to import bookmarks.html, and try InitDB again.
Assignee | ||
Comment 6•17 years ago
|
||
Attachment #266653 -
Flags: review?(sspitzer)
Comment 7•17 years ago
|
||
Comment on attachment 266653 [details] [diff] [review] backup and re-import if db is corrupt r=sspitzer, but three comments / questions: 1) instead of "places.sqlite.corrupt", can you use CreateUnique() to generate the leaf name before calling CopyTo()? + rv = dbFile->CopyTo(profDir, NS_LITERAL_STRING("places.sqlite.corrupt")); 2) we have: NS_LITERAL_STRING("places.sqlite") can we #define that instead (and then append ".corrupt" when we need it?) 3) for the sake of the person who will attempt to verify this fix, how are you corrupting the db? Perhaps attaching a corrupt places.sqlite file would be useful, if you have one (or can make one.)
Attachment #266653 -
Flags: review?(sspitzer) → review+
Assignee | ||
Comment 8•17 years ago
|
||
(In reply to comment #7) > (From update of attachment 266653 [details] [diff] [review]) > r=sspitzer, but three comments / questions: > > 1) > > instead of "places.sqlite.corrupt", can you use CreateUnique() to generate the > leaf name before calling CopyTo()? > > > + rv = dbFile->CopyTo(profDir, NS_LITERAL_STRING("places.sqlite.corrupt")); so, the attached patch attempts this, but it's pretty ghetto because CreateUnique creates the file, so we get the leafname and then have to remove the file :P also, it doesn't actually work. CreateUnique fails, but i haven't figured out why yet. does anything there look amiss? > 3) > > for the sake of the person who will attempt to verify this fix, how are you > corrupting the db? Perhaps attaching a corrupt places.sqlite file would be > useful, if you have one (or can make one.) > all i do is copy any non-sqlite file and name it places.sqlite. guarantees that sqlite will call it corrupt.
Assignee | ||
Comment 9•17 years ago
|
||
Attachment #266709 -
Flags: review?(sspitzer)
Comment 10•17 years ago
|
||
Comment on attachment 266709 [details] [diff] [review] v3 - working r=sspitzer, thanks dietrich
Attachment #266709 -
Flags: review?(sspitzer) → review+
Assignee | ||
Comment 11•17 years ago
|
||
Checking in src/nsNavHistory.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v <-- nsNavHistory.cpp new revision: 1.129; previous revision: 1.128 done
Assignee | ||
Comment 12•17 years ago
|
||
keeping open, per comment #5.
Target Milestone: Firefox 3 alpha5 → Firefox 3 alpha6
Assignee | ||
Updated•17 years ago
|
Whiteboard: [swag: 1d]
Assignee | ||
Comment 13•17 years ago
|
||
Attachment #269548 -
Flags: review?(mano)
Comment 14•17 years ago
|
||
Comment on attachment 269548 [details] [diff] [review] fix as described in comment #5 reviewed over #places
Attachment #269548 -
Flags: review?(mano) → review-
Assignee | ||
Comment 15•17 years ago
|
||
ManoMac: dietrich: few questions about the backup patch [4:28pm] ManoMac: dietrich: 1) why isn't the file name hardcoded? [4:28pm] ManoMac: dietrich: 2) what's the createunique thing about [4:29pm] ManoMac: 3) s/rv != NS_OK/NS_FAILED(rv) [4:31pm] dietrich: ManoMac: re #1, are you referring to the backup file? [4:31pm] ManoMac: dietrich: right [4:32pm] dietrich: re #2, so we don't overwrite old corrupt files [4:32pm] ManoMac: dietrich: what does createunique do again? [4:32pm] dietrich: re #1 & #2, so that corrupt files will be places.sqlite.corrupt.1/2/3, etc [4:33pm] ManoMac: dietrich: but you can hardcode places.sqlite.corrupt [4:33pm] ManoMac: places.sqlite is hardcoded... [4:33pm] dietrich: ManoMac: ok, will do [4:34pm] ManoMac: dietrich: now, given that places dbs can be _very_ large, you might want to compact the db first [4:34pm] ManoMac: stephend sent me a almost empty places.sqlite... 20mb [4:35pm] ManoMac: we may have to sort out a compact-solution at some point. [4:36pm] dietrich: ugh, 20mb? [4:36pm] dietrich: how almost-empty? [4:36pm] ManoMac: one livemark [4:36pm] ManoMac: empty history [4:36pm] ManoMac: iirc [4:36pm] dietrich: i have a lot of bookmarks, and mine is around 5mb [4:37pm] ManoMac: dietrich: well, that file used to have much more contents once [4:37pm] ManoMac: apparently cleaning up history does not compact the db [4:37pm] dietrich: ah, ok i see
Assignee | ||
Comment 16•17 years ago
|
||
this address your 1st and 3rd issues. we can't vacuum here because the problem is that the db itself cannot be opened. i've filed bug 385834 for adding vacuuming.
Attachment #269548 -
Attachment is obsolete: true
Attachment #269762 -
Flags: review?(mano)
Assignee | ||
Comment 17•17 years ago
|
||
working patch
Attachment #269762 -
Attachment is obsolete: true
Attachment #269764 -
Flags: review?(mano)
Attachment #269762 -
Flags: review?(mano)
Comment 18•17 years ago
|
||
Comment on attachment 269764 [details] [diff] [review] fix v2 redux r=mano.
Attachment #269764 -
Flags: review?(mano) → review+
Assignee | ||
Comment 19•17 years ago
|
||
Checking in toolkit/components/places/src/nsNavHistory.cpp; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v <-- nsNavHistory.cpp new revision: 1.136; previous revision: 1.135 done Checking in toolkit/components/places/src/nsNavHistory.h; /cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.h,v <-- nsNavHistory.h new revision: 1.84; previous revision: 1.83 done
Status: NEW → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 20•17 years ago
|
||
Is an issue that can be verified? If so, what would demonstrate that the issue is resolved?
Assignee | ||
Comment 21•17 years ago
|
||
1. replace your places.sqlite with any kind of file that is not a sqlite db 2. start up the app 3. confirm that the places.sqlite has been renamed to places.sqlite.corrupt 4. confirm that a new places.sqlite has been created 5. confirm that history, bookmarks, livemarks and microsummaries still work
Comment 22•17 years ago
|
||
verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007080805 Minefield/3.0a8pre
Status: RESOLVED → VERIFIED
Comment 23•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
•