Closed
Bug 493374
Opened 15 years ago
Closed 15 years ago
Avoid marking database as corrupt if initializing additional db objects fails, and avoid creating more than one corrupt file every 24 hours.
Categories
(Toolkit :: Places, defect)
Toolkit
Places
Tracking
()
RESOLVED
FIXED
mozilla1.9.2a1
People
(Reporter: mak, Assigned: mak)
References
Details
(Keywords: fixed1.9.1)
Attachments
(1 file, 1 obsolete file)
7.68 KB,
patch
|
Details | Diff | Splinter Review |
Our init DB path is wrong in 2 points: - the DB should be considered corrupt only if it is corrupt or if it's schema is corrupt. A failure in initalizing additional items like views, functions, temp tables should not be considered as a corrupt database. - If we mark db as corrupt we backup it and create a new one, if something fails out of the basic database creation or database close fails for any reason this process starts looping, and we continue creating corrupt files till user's disk is full. I think this is something we should really fix for next release, or future changes could become a pain.
Flags: blocking1.9.1?
Assignee | ||
Comment 1•15 years ago
|
||
Dietrich, what do you think of this approach? The main purpose is to avoid in future we could fail in a loop. failing once is more than enough.
Attachment #377846 -
Flags: review?(dietrich)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [has patch][needs review dietrich]
Assignee | ||
Comment 2•15 years ago
|
||
please ignore the printfs, that's debug output i forgot to remove
Comment 3•15 years ago
|
||
yeah only writing a backup once a day sounds ok i guess. far better than the status quo, so r=me on that approach. however, i think that the code itself should be in mozStorage, where the backup code is. otherwise, all mozStorage consumers will need to implement protection against this insane behavior. ccing sdwilsh for his thoughts there.
Updated•15 years ago
|
Attachment #377846 -
Flags: review?(dietrich)
Assignee | ||
Comment 4•15 years ago
|
||
I'm not sure storage wants to limit freedom of its users, we are calling backupdatabase, so we should probably take care of its possible flaws. there's a possibility a storage implementer wants to backup more than once every 24 hours a valid db.
Assignee | ||
Comment 5•15 years ago
|
||
Comment on attachment 377846 [details] [diff] [review] patch v1.0 moving review to Shawn as a reminder
Attachment #377846 -
Flags: review?(sdwilsh)
Assignee | ||
Updated•15 years ago
|
Whiteboard: [has patch][needs review dietrich] → [has patch][needs review sdwilsh]
Comment 6•15 years ago
|
||
I guess one question here is why do we even back this stuff up? I mean, we can't really do anything with these files, right?
Assignee | ||
Comment 7•15 years ago
|
||
(In reply to comment #6) > I guess one question here is why do we even back this stuff up? I mean, we > can't really do anything with these files, right? I think the main purposes are: - allow us to analyze corrupt files users send us (i've not yet seen any though) - allow third party extensions or technical users to recover data from them
Comment 8•15 years ago
|
||
Comment on attachment 377846 [details] [diff] [review] patch v1.0 >+++ b/toolkit/components/places/src/nsNavHistory.cpp >+namespace PlacesFileUtils { per mozilla style guidelines, this should be inside namespace mozilla, and then maybe namespace places? That's the approach I'm going with in bug 455555 at least (and what storage does). >+ bool HasRecentCorruptDB() I'd prefer a lowercase character to start the name since it's not XPCOM, but that's a storage style pref again. >+ profDir->GetDirectoryEntries(getter_AddRefs(entries)); >+ NS_ENSURE_SUCCESS(rv, false); you don't actually assign to rv here >+ // init db file. If we won't be able to connect to the database it is most >+ // likely corrupt, so we will backup it and create a new one. Since you are here, let's make the first line an actual sentence. "Init the database file." please. >+ // Init database schema. If this will fail there's an high possibility the >+ // schema is corrupt or incorrect, so we will force a new database >+ // initialization. Also needs a "the" in the first sentence. >+ // Try again to initialize the schema on the new database. nit: Try to initialize the schema again on the new database. (english is hard) >+ nsCOMPtr<PlacesEvent> completeEvent = >+ new PlacesEvent(PLACES_INIT_COMPLETE_EVENT_TOPIC); This should actually be a nsRefPtr I think. >+ // file at every try to access any Places service. That is bad because would >+ // fast fill user's disk space without any notice. nit: That is bad because it would quickly fill the user's disk space without any notice. >+ // start with a clean database. The process of back-up corrupt database nit: The process of backing up a corrupt database >+ // We can't do much at this point, fire a locked event so that user is nit: ..at this point, so fire a... >+ // notified we can't ensure Places to work. nit: ...notified that we... >+ nsCOMPtr<PlacesEvent> lockedEvent = >+ new PlacesEvent(PLACES_DB_LOCKED_EVENT_TOPIC); Again, I think we want nsRefPtr here. >+ nsCOMPtr<PlacesEvent> lockedEvent = >+ new PlacesEvent(PLACES_DB_LOCKED_EVENT_TOPIC); ditto >+ // DO NOT PUT HERE ANYTHING THAT IS NOT RELATED TO INITIALIZE OR MODIFY >+ // THE DISK DATABASE. nit: ...NOT RELATED TO INITIALIZATION OR MODIFYING THE DISK DATABASE. >+++ b/toolkit/components/places/src/nsNavHistory.h >+ nsresult InitAdditionalDBItems(); java-doc comments pretty please? r=sdwilsh with comments addressed.
Attachment #377846 -
Flags: review?(sdwilsh) → review+
Assignee | ||
Comment 9•15 years ago
|
||
addressed comments
Attachment #377846 -
Attachment is obsolete: true
Assignee | ||
Updated•15 years ago
|
Attachment #378131 -
Attachment is patch: true
Attachment #378131 -
Attachment mime type: application/octet-stream → text/plain
Assignee | ||
Updated•15 years ago
|
Whiteboard: [has patch][needs review sdwilsh] → [has patch][can land]
Assignee | ||
Comment 10•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/58346c44501f
Whiteboard: [has patch][can land] → [has patch]
Target Milestone: --- → mozilla1.9.2a1
Assignee | ||
Updated•15 years ago
|
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [has patch]
Assignee | ||
Comment 11•15 years ago
|
||
Comment on attachment 378131 [details] [diff] [review] patch v1.1 i think this either blocks or should be approved due to the fact in future releases we will for sure hit this again. And if we hit this we end up filling user's disk with trash data. risk is low. building a test could be hard since we need to run a new profile in an old build. Or we could maybe build a broken places.sqlite and try to init it, eventually i could try at this, not sure it will work as expected.
Attachment #378131 -
Flags: approval1.9.1?
Assignee | ||
Updated•15 years ago
|
Attachment #378131 -
Flags: approval1.9.1?
Assignee | ||
Comment 13•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9a49d7ecebe2
Keywords: fixed1.9.1
You need to log in
before you can comment on or make changes to this bug.
Description
•