nsNavHistory::Init is fragile

VERIFIED FIXED

Status

()

Firefox
Bookmarks & History
P1
normal
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: (not reading, please use seth@sspitzer.org instead), Assigned: dietrich)

Tracking

Trunk
Points:
---
Bug Flags:
blocking-firefox3 +
in-litmus +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

v2
2.75 KB, patch
(not reading, please use seth@sspitzer.org instead)
: review+
Details | Diff | Splinter Review
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.
Flags: blocking-firefox3?
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.
Flags: blocking-firefox3? → blocking-firefox3+
Priority: -- → P1
(Assignee)

Updated

11 years ago
Assignee: nobody → dietrich
(Assignee)

Comment 2

11 years ago
Created attachment 295410 [details] [diff] [review]
fix v1
Attachment #295410 - Flags: review?(sspitzer)
(Assignee)

Comment 3

11 years ago
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.
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
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 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?
Attachment #295410 - Flags: review?(sspitzer) → review+
(Assignee)

Comment 7

11 years ago
Created attachment 295462 [details] [diff] [review]
v2
Attachment #295410 - Attachment is obsolete: true
Attachment #295462 - Flags: review?(sspitzer)
(Assignee)

Comment 8

11 years ago
(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 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.
Attachment #295462 - Flags: review?(sspitzer) → review+
(Assignee)

Comment 10

10 years ago
(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.
> I tested both scenarios, and Close() did not fail.

thanks for double checking, dietrich.
Status: NEW → ASSIGNED
(Assignee)

Comment 12

10 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Flags: in-litmus?
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
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-
Flags: in-litmus? → in-litmus+
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.