initialization of places services can fail in error conditions

VERIFIED FIXED in Firefox 3 alpha6

Status

()

Firefox
Bookmarks & History
P2
normal
VERIFIED FIXED
12 years ago
8 years ago

People

(Reporter: Brett Wilson, Assigned: dietrich)

Tracking

Trunk
Firefox 3 alpha6
Points:
---
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [swag: 1d])

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

12 years ago
The history observer is registered at the beginning, which will be left dangling on error. See bug 327331 for the history service version.
Assignee: nobody → bryner
Priority: -- → P2
Target Milestone: --- → Firefox 2 beta1
Assignee: bryner → nobody
Target Milestone: Firefox 2 beta1 → ---
(Assignee)

Comment 1

11 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
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?
 
Flags: blocking-firefox3? → blocking-firefox3+
(Assignee)

Updated

11 years ago
Assignee: nobody → dietrich
(Assignee)

Comment 3

11 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

11 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

11 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

11 years ago
Created attachment 266653 [details] [diff] [review]
backup and re-import if db is corrupt
Attachment #266653 - Flags: review?(sspitzer)
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

11 years ago
Created attachment 266704 [details] [diff] [review]
v2 - failing

(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

11 years ago
Created attachment 266709 [details] [diff] [review]
v3 - working
Attachment #266709 - Flags: review?(sspitzer)
Comment on attachment 266709 [details] [diff] [review]
v3 - working

r=sspitzer, thanks dietrich
Attachment #266709 - Flags: review?(sspitzer) → review+
(Assignee)

Comment 11

11 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

11 years ago
keeping open, per comment #5.
Target Milestone: Firefox 3 alpha5 → Firefox 3 alpha6
(Assignee)

Updated

11 years ago
Whiteboard: [swag: 1d]
(Assignee)

Updated

11 years ago
Depends on: 380345
(Assignee)

Comment 13

11 years ago
Created attachment 269548 [details] [diff] [review]
fix as described in comment #5
Attachment #269548 - Flags: review?(mano)
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

11 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

11 years ago
Created attachment 269762 [details] [diff] [review]
fix v2

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

11 years ago
Created attachment 269764 [details] [diff] [review]
fix v2 redux

working patch
Attachment #269762 - Attachment is obsolete: true
Attachment #269764 - Flags: review?(mano)
Attachment #269762 - Flags: review?(mano)
(Assignee)

Comment 19

11 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
Last Resolved: 11 years ago
Resolution: --- → FIXED
Is an issue that can be verified? If so, what would demonstrate that the issue is resolved?
(Assignee)

Comment 21

11 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
verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007080805 Minefield/3.0a8pre
Status: RESOLVED → VERIFIED
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.