408751 - nsNavHistory::Init is fragile

Status: VERIFIED FIXED

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

Description

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

nsNavHistory::Init is fragile

similar to bug #327350, which was at one point nsNavBookmarks::Init is fragile

see bug #408443 for an example of the fragility causing problems.

We should do things like back force errors (example, back out the fix for bug #408443 and try a2 -> b2 migration) and make things more robust.

Comment 1

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

see also bug #404565 – Bookmarks not loaded, "places.sqlite.corrupt" files generated, which is a dup of #408443.

this bug points out that when we hit this problem, you will end up with about 5 places.sqlite-<x>.corrupt files per link you visit.

Comment 2

[Dietrich Ayala (:dietrich)]

Attachment: fix v1

Comment 3

[Dietrich Ayala (:dietrich)]

Our "if db is not able to initialized, back it up, and start anew" code is broken.

- nsIFile.moveTo() changes the file reference of the existing nsIFile to the new path, which meant that mDBFile is pointing to places.sqlite.corrupt.

- the filename argument we were passing to moveTo() was the non-unique backup filename

I noticed this after reverting the fix to bg 408443, and then seeing that the v6 migration was failing *twice*.

The solution attached is to:

1. create the unique file
2. get it's name
3. remove it
4. copy original file to new unique file name
5. remove original

This is somewhat laborious, but is more explicit than before. Also, BackupDBFile now just backs up as advertised, instead of affecting the original in any way.

Comment 4

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

dietrich,

1)  We had some bug recently where we created places.sqlite.corrupt.1 .2 .3, ..., upon initialization failure, and this explains that.  (See bug #408443, oops, you have already seen it.)

2)  I think it would be better to re-use mozStorageConnection::BackupDB() instead, like they do in the download manager?

Note, we might not have mDBConn at this point, but I don't see why the BackupDB() utility function needs a db connection.  perhaps it was put there out of convenience?   If re-using it makes sense, we should move that code.

http://lxr.mozilla.org/seamonkey/source/toolkit/components/downloads/src/nsDownloadManager.cpp#478

Comment 5

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

actually, BackupDB() uses mDatabaseFile, so re-using this code might not be as easy as I thought.

Hmm, if BackupDB() requires a db connection (and a database file), how is it correct?

In the places code, we backup a db before there is a connection, but in the download manager, our backup could happen at any time, without regards to the state of the sqlite file, right?

Maybe dietrich's original approach is better.

Comment 6

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

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

r=sspitzer, sorry for the flip flop.

after looking into BackupDB(), given that we are backing up without a db connection I like what you've done here better.  And I have concerns about mozStorageConnection::BackupDB()

one request, can you put a call to assert that mDBConn is null in nsNavHistory::BackupDBFile()?

It should always be null because we call it before OpenDatabase(), right?

Comment 7

[Dietrich Ayala (:dietrich)]

Attachment: v2

Comment 8

[Dietrich Ayala (:dietrich)]

(In reply to comment #6)
> (From update of attachment 295410 [details] [diff] [review])
> r=sspitzer, sorry for the flip flop.
> 
> after looking into BackupDB(), given that we are backing up without a db
> connection I like what you've done here better.  And I have concerns about
> mozStorageConnection::BackupDB()
> 
> one request, can you put a call to assert that mDBConn is null in
> nsNavHistory::BackupDBFile()?
> 
> It should always be null because we call it before OpenDatabase(), right?
> 

No, it should not always be null, as in the case of a database upgrade problem (eg: bug 408443) where the database file itself is open and in a valid state from sqlite's pov.

In this patch, I've explicitly closed the connection prior to the backup when aForceInit is true.

Comment 9

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

Comment on attachment 295462 [details] [diff] [review]
v2

r=sspitzer

I'm a little concerned about a scenario where mDBConn null, so we call Close(), but Close() fails (for whatever reason and returns an NS_FAILURE code)

in that case, nsNavHistory::InitDBFile() will return failure, and that would be bad.

Dietrich, please confirm that with a corrupt places.sqlite or non-existent places.sqlite this doesn't happen.

Comment 10

[Dietrich Ayala (:dietrich)]

(In reply to comment #9)
> (From update of attachment 295462 [details] [diff] [review])
> r=sspitzer
> 
> I'm a little concerned about a scenario where mDBConn null, so we call Close(),
> but Close() fails (for whatever reason and returns an NS_FAILURE code)
> 
> in that case, nsNavHistory::InitDBFile() will return failure, and that would be
> bad.
> 
> Dietrich, please confirm that with a corrupt places.sqlite or non-existent
> places.sqlite this doesn't happen.
> 

I tested both scenarios, and Close() did not fail.

Comment 11

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

> I tested both scenarios, and Close() did not fail.

thanks for double checking, dietrich.

Comment 12

[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.224; previous revision: 1.223
done

Comment 13

[Tracy Walker [:tracy]]

verified with:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b5) Gecko/2008032606 Firefox/3.0b5
and
Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-GB; rv:1.9b5) Gecko/2008032604 Firefox/3.0b5

Comment 14

[Tracy Walker [:tracy]]

I just added https://litmus.mozilla.org/show_test.cgi?searchType=by_id&id=5291 to litmus which I believe sort of covers this.  otherwise, I'd mark this in-litmus-

Comment 15

[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