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)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: mak, Assigned: mak)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 1 obsolete file)

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?
Blocks: 493291
Attached patch patch v1.0 (obsolete) — Splinter Review
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)
Whiteboard: [has patch][needs review dietrich]
please ignore the printfs, that's debug output i forgot to remove
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.
Attachment #377846 - Flags: review?(dietrich)
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.
Comment on attachment 377846 [details] [diff] [review]
patch v1.0

moving review to Shawn as a reminder
Attachment #377846 - Flags: review?(sdwilsh)
Whiteboard: [has patch][needs review dietrich] → [has patch][needs review sdwilsh]
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?
(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 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+
Attached patch patch v1.1Splinter Review
addressed comments
Attachment #377846 - Attachment is obsolete: true
Attachment #378131 - Attachment is patch: true
Attachment #378131 - Attachment mime type: application/octet-stream → text/plain
Whiteboard: [has patch][needs review sdwilsh] → [has patch][can land]
http://hg.mozilla.org/mozilla-central/rev/58346c44501f
Whiteboard: [has patch][can land] → [has patch]
Target Milestone: --- → mozilla1.9.2a1
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [has patch]
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?
Uh, yes.
Flags: blocking1.9.1? → blocking1.9.1+
Attachment #378131 - Flags: approval1.9.1?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: