416208 - Update site specific preferences from changes to mozIStorageService::OpenDatabase

Status: RESOLVED FIXED

(Toolkit :: Preferences, defect, P3)

Description

[Shawn Wilsher :sdwilsh]

See bug 416173.
myk - I'm not sure what the right component this should be in, so please correct me if I'm wrong.

Comment 1

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v1: updates API usage and adds backups/other integrity

Here's a patch that updates the content pref service so that it checks connectionReady after openDatabase instead of catching an exception and uses backupDB() to back up the database before recreating it if it is corrupted.

This patch also adds some additional error checking and recovery.  Specifically, it deletes the database if it runs into a problem while creating it (so as not to leave it in an inconsistent, partly-created state); it backs up and recreates the database if a schema migration fails; and it creates the schema if the database exists and isn't corrupted but the schema was never created.


A note about that last change: it requires changing the meaning of "schemaVersion = 0" from "the database is from a very first version of the prototype extension, which didn't set a schema version" to "the database has not had a schema created in it yet".  Theoretically someone still using that early prototype could lose their site-specific preferences upon upgrading to Firefox 3, but the chance of that is pretty slight (they should have at least autoupdated to later prototypes that did set a schema version), and it was an early prototype, so we never guaranteed data integrity anyway.

The chance of a database without a schema is also pretty small, but it isn't nonexistent.  There's been at least one report of it happening (bug 388106).  It could happen if other code (f.e. an extension) creates the database without creating the schema in it.  It could also happen if a user creates that database from the command line ("sqlite3 content-prefs.sqlite" will do it, as will "touch content-prefs.sqlite").

And in theory it could happen if the disk runs out of space while creating the database (which is likely what caused bug 388106), although other parts of the patch should protect against that now.  So I think it's worth making the change to protect all Firefox users against one remote possibility as opposed to protecting early prototype testers (if any remain) against another possibility.


Given that we're late in the cycle (and for good measure), I've included tests for five anticipated scenarios: creating a new database, opening an existing database, opening an empty database, opening a corrupted database, and opening a database with a corrupted schema.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

This was blocking-firefox3+ when it was in the Firefox product.  Requesting blocking-gecko1.9 now that it's in the Toolkit product.  This is needed to accommodate a change to the way the mozStorage API signals database corruption.  Without this change, database corruption in the content prefs service would no longer be handled, and a broken database would simply stay broken.

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 305381 [details] [diff] [review]
patch v1: updates API usage and adds backups/other integrity

>+  // The database connection to the content preferences database.
>+  // Useful for accessing and manipulating preferences in ways that are caller-
>+  // specific or for which there is not yet a generic method, although generic
>+  // functionality useful to multiple callers should generally be added to this
>+  // unfrozen interface.  Also useful for testing the database creation
>+  // and migration code.
>+  readonly attribute mozIStorageConnection DBConnection;
could this be a javadoc comment please?  I'm not sure automated tools (like the stuff that generates xulplanet) can handle this.

r=sdwilsh

Comment 4

[Myk Melez [:myk] [@mykmelez]]

Attachment: patch v2: comments -> javadoc comments

> could this be a javadoc comment please?  I'm not sure automated tools (like 
> the stuff that generates xulplanet) can handle this.

Yup, here's a patch identical to the previous one except that the cited comment (and the one above it) are now javadoc comments.

This is the version of the patch I'll check in.

Requesting approval for this blocking-firefox3+ (and likely blocking-gecko1.9+) bug that improves the robustness of the content prefs datastore against several problematic scenarios (running out of disk space, database creation by naive third parties, and corruption of unknown origin) while updating its corruption detection to account for recent changes to the mozStorage API and also taking advantage of mozStorage's generic backup functionality to back up corrupt databases before recreating them.

The patch includes tests for five scenarios, two normal ones (creating a new database, and opening an existing database) along with three exceptional ones (opening an empty database, opening a corrupt database, and opening a database with a corrupt schema), which reduces the risk of regression from its changes.

Comment 5

[Damon Sicore (:damons)]

Carrying over blocking flag from firefox product.

Comment 6

[Damon Sicore (:damons)]

Comment on attachment 305415 [details] [diff] [review]
patch v2: comments -> javadoc comments

a1.9+=damons

Comment 7

[Damon Sicore (:damons)]

Comment on attachment 305415 [details] [diff] [review]
patch v2: comments -> javadoc comments

a1.9+=damons

Comment 8

[Myk Melez [:myk] [@mykmelez]]

Checking in toolkit/components/contentprefs/public/nsIContentPrefService.idl;
/cvsroot/mozilla/toolkit/components/contentprefs/public/nsIContentPrefService.idl,v  <--  nsIContentPrefService.idl
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/components/contentprefs/src/nsContentPrefService.js;
/cvsroot/mozilla/toolkit/components/contentprefs/src/nsContentPrefService.js,v  <--  nsContentPrefService.js
new revision: 1.14; previous revision: 1.13
done
Checking in toolkit/components/contentprefs/tests/unit/head_contentPrefs.js;
/cvsroot/mozilla/toolkit/components/contentprefs/tests/unit/head_contentPrefs.js,v  <--  head_contentPrefs.js
new revision: 1.2; previous revision: 1.1
done
Checking in toolkit/components/contentprefs/tests/unit/test_contentPrefs.js;
/cvsroot/mozilla/toolkit/components/contentprefs/tests/unit/test_contentPrefs.js,v  <--  test_contentPrefs.js
new revision: 1.2; previous revision: 1.1
done

Comment 9

[Ed Lee :Mardak]

sdwilsh: Stealing code from download manager schema migration test code ;)

cvs diff toolkit/components/contentprefs/tests/unit/head_contentPrefs.js
Index: toolkit/components/contentprefs/tests/unit/head_contentPrefs.js
===================================================================
RCS file: /cvsroot/mozilla/toolkit/components/contentprefs/tests/unit/head_contentPrefs.js,v
retrieving revision 1.2
diff -r1.2 head_contentPrefs.js
140c140
<       file.remove(false);
---
>       try { file.remove(false); } catch(e) { /* stupid windows box */ }
~/mozilla Ed$ cvs commit -m 'Bustage fix for bug 416208 testcase' !$
cvs commit -m 'Bustage fix for bug 416208 testcase' toolkit/components/contentprefs/tests/unit/head_contentPrefs.js
Checking in toolkit/components/contentprefs/tests/unit/head_contentPrefs.js;
/cvsroot/mozilla/toolkit/components/contentprefs/tests/unit/head_contentPrefs.js,v  <--  head_contentPrefs.js
new revision: 1.3; previous revision: 1.2
done

Comment 10

[Myk Melez [:myk] [@mykmelez]]

FWIW, trying to close the connection before deleting the database throws NS_ERROR_FILE_IS_LOCKED on Mac and Windows (and presumably Linux), presumably because statements in the service haven't been finalized:

Cc["@mozilla.org/content-pref/service;1"].getService(Ci.nsIContentPrefService)
                                         .DBConnection.close();
ContentPrefTest.deleteDatabase();

But simply deleting the database only throws NS_ERROR_FILE_ACCESS_DENIED on Windows.  Nevertheless, I wonder if finalizing the statements, closing the database connection, and then deleting the file might work on Windows.  We'd have to update the public API of the service to do this, however, since the statements needing finalizing are cached in private properties of it.