327350 - initialization of places services can fail in error conditions

Status: VERIFIED FIXED

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

Description

[Brett Wilson]

The history observer is registered at the beginning, which will be left dangling on error. See bug 327331 for the history service version.

Comment 1

[Dietrich Ayala (:dietrich)]

(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.

Comment 2

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

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?
 

Comment 3

[Dietrich Ayala (:dietrich)]

(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)

Comment 4

[Dietrich Ayala (:dietrich)]

(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.

Comment 5

[Dietrich Ayala (:dietrich)]

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.

Comment 6

[Dietrich Ayala (:dietrich)]

Attachment: backup and re-import if db is corrupt

Comment 7

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

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.)

Comment 8

[Dietrich Ayala (:dietrich)]

Attachment: 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.

Comment 9

[Dietrich Ayala (:dietrich)]

Attachment: v3 - working

Comment 10

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

Comment on attachment 266709 [details] [diff] [review]
v3 - working

r=sspitzer, thanks dietrich

Comment 11

[Dietrich Ayala (:dietrich)]

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

Comment 12

[Dietrich Ayala (:dietrich)]

keeping open, per comment #5.

Comment 13

[Dietrich Ayala (:dietrich)]

Attachment: fix as described in comment #5

Comment 14

[Asaf Romano (gone)]

Comment on attachment 269548 [details] [diff] [review]
fix as described in comment #5

reviewed over #places

Comment 15

[Dietrich Ayala (:dietrich)]

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

Comment 16

[Dietrich Ayala (:dietrich)]

Attachment: 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.

Comment 17

[Dietrich Ayala (:dietrich)]

Attachment: fix v2 redux

working patch

Comment 18

[Asaf Romano (gone)]

Comment on attachment 269764 [details] [diff] [review]
fix v2 redux

r=mano.

Comment 19

[Dietrich Ayala (:dietrich)]

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

Comment 20

[Al Billings [:abillings - ex-MoCo]]

Is an issue that can be verified? If so, what would demonstrate that the issue is resolved?

Comment 21

[Dietrich Ayala (:dietrich)]

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

[Tracy Walker [:tracy]]

verified with: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007080805 Minefield/3.0a8pre

Comment 23

[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