Avoid marking database as corrupt if initializing additional db objects fails, and avoid creating more than one corrupt file every 24 hours.

RESOLVED FIXED in mozilla1.9.2a1

Status

()

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: mak, Assigned: mak)

Tracking

({fixed1.9.1})

Trunk
mozilla1.9.2a1
Points:
---
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

10 years ago
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)

Updated

10 years ago
Blocks: 493291
(Assignee)

Comment 1

10 years ago
Posted 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)
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs review dietrich]
(Assignee)

Comment 2

10 years ago
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)
(Assignee)

Comment 4

10 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

10 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

10 years ago
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?
(Assignee)

Comment 7

10 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 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

10 years ago
Posted patch patch v1.1Splinter Review
addressed comments
Attachment #377846 - Attachment is obsolete: true
(Assignee)

Updated

10 years ago
Attachment #378131 - Attachment is patch: true
Attachment #378131 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

10 years ago
Whiteboard: [has patch][needs review sdwilsh] → [has patch][can land]
(Assignee)

Comment 10

10 years ago
http://hg.mozilla.org/mozilla-central/rev/58346c44501f
Whiteboard: [has patch][can land] → [has patch]
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Updated

10 years ago
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Whiteboard: [has patch]
(Assignee)

Comment 11

10 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?
Uh, yes.
Flags: blocking1.9.1? → blocking1.9.1+
(Assignee)

Updated

10 years ago
Attachment #378131 - Flags: approval1.9.1?
(Assignee)

Updated

10 years ago
Duplicate of this bug: 499090
(Assignee)

Updated

10 years ago
Duplicate of this bug: 499090
You need to log in before you can comment on or make changes to this bug.