Bug 547031 - Improve async error handling in cookies. Part 2: Clean up TryInitDB(). r=sdwilsh, a=betaN+
authorDan Witte <dwitte@mozilla.com>
Fri, 12 Nov 2010 09:32:35 -0800
changeset 57406 adcc4d2f4070d04c182c1c19b171bfe6bb1260f3
parent 57405 12336d312ad1adfc96e6403e16692c96c01f2939
child 57407 09bb5890500d91c69b3474b08660cbe915e3b12d
push id16911
push userdwitte@mozilla.com
push dateFri, 12 Nov 2010 17:33:05 +0000
treeherdermozilla-central@0525032a59a3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssdwilsh, betaN
bugs547031
milestone2.0b8pre
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 547031 - Improve async error handling in cookies. Part 2: Clean up TryInitDB(). r=sdwilsh, a=betaN+
extensions/cookie/test/unit/head_cookies.js
extensions/cookie/test/unit/test_cookies_sync_failure.js
netwerk/cookie/nsCookieService.cpp
netwerk/cookie/nsCookieService.h
--- a/extensions/cookie/test/unit/head_cookies.js
+++ b/extensions/cookie/test/unit/head_cookies.js
@@ -191,16 +191,57 @@ function CookieDatabaseConnection(profil
   if (!exists)
     this.db.schemaVersion = schema;
 
   // Open an exclusive connection, so we error out if the database is open
   // by another reader.
   this.db.executeSimpleSQL("PRAGMA locking_mode = EXCLUSIVE");
 
   switch (schema) {
+  case 1:
+    {
+      if (!exists) {
+        this.db.executeSimpleSQL(
+          "CREATE TABLE moz_cookies (       \
+             id INTEGER PRIMARY KEY,        \
+             name TEXT,                     \
+             value TEXT,                    \
+             host TEXT,                     \
+             path TEXT,                     \
+             expiry INTEGER,                \
+             isSecure INTEGER,              \
+             isHttpOnly INTEGER)");
+      }
+
+      this.stmtInsert = this.db.createStatement(
+        "INSERT INTO moz_cookies (        \
+           id,                            \
+           name,                          \
+           value,                         \
+           host,                          \
+           path,                          \
+           expiry,                        \
+           isSecure,                      \
+           isHttpOnly)                    \
+           VALUES (                       \
+           :id,                           \
+           :name,                         \
+           :value,                        \
+           :host,                         \
+           :path,                         \
+           :expiry,                       \
+           :isSecure,                     \
+           :isHttpOnly)");
+
+      this.stmtDelete = this.db.createStatement(
+        "DELETE FROM moz_cookies WHERE id = :id");
+
+      break;
+    }
+
   case 2:
     {
       if (!exists) {
         this.db.executeSimpleSQL(
           "CREATE TABLE moz_cookies (       \
              id INTEGER PRIMARY KEY,        \
              name TEXT,                     \
              value TEXT,                    \
@@ -365,16 +406,27 @@ CookieDatabaseConnection.prototype =
 {
   insertCookie: function(cookie)
   {
     if (!cookie instanceof Cookie)
       do_throw("not a cookie");
 
     switch (this.schema)
     {
+    case 1:
+      this.stmtInsert.bindByName("id", cookie.creationTime);
+      this.stmtInsert.bindByName("name", cookie.name);
+      this.stmtInsert.bindByName("value", cookie.value);
+      this.stmtInsert.bindByName("host", cookie.host);
+      this.stmtInsert.bindByName("path", cookie.path);
+      this.stmtInsert.bindByName("expiry", cookie.expiry);
+      this.stmtInsert.bindByName("isSecure", cookie.isSecure);
+      this.stmtInsert.bindByName("isHttpOnly", cookie.isHttpOnly);
+      break;
+
     case 2:
       this.stmtInsert.bindByName("id", cookie.creationTime);
       this.stmtInsert.bindByName("name", cookie.name);
       this.stmtInsert.bindByName("value", cookie.value);
       this.stmtInsert.bindByName("host", cookie.host);
       this.stmtInsert.bindByName("path", cookie.path);
       this.stmtInsert.bindByName("expiry", cookie.expiry);
       this.stmtInsert.bindByName("lastAccessed", cookie.lastAccessed);
@@ -390,17 +442,17 @@ CookieDatabaseConnection.prototype =
       this.stmtInsert.bindByName("host", cookie.host);
       this.stmtInsert.bindByName("path", cookie.path);
       this.stmtInsert.bindByName("expiry", cookie.expiry);
       this.stmtInsert.bindByName("lastAccessed", cookie.lastAccessed);
       this.stmtInsert.bindByName("isSecure", cookie.isSecure);
       this.stmtInsert.bindByName("isHttpOnly", cookie.isHttpOnly);
       break;
 
-    case 3:
+    case 4:
       this.stmtInsert.bindByName("baseDomain", cookie.baseDomain);
       this.stmtInsert.bindByName("name", cookie.name);
       this.stmtInsert.bindByName("value", cookie.value);
       this.stmtInsert.bindByName("host", cookie.host);
       this.stmtInsert.bindByName("path", cookie.path);
       this.stmtInsert.bindByName("expiry", cookie.expiry);
       this.stmtInsert.bindByName("lastAccessed", cookie.lastAccessed);
       this.stmtInsert.bindByName("creationTime", cookie.creationTime);
@@ -417,16 +469,17 @@ CookieDatabaseConnection.prototype =
 
   deleteCookie: function(cookie)
   {
     if (!cookie instanceof Cookie)
       do_throw("not a cookie");
 
     switch (this.db.schemaVersion)
     {
+    case 1:
     case 2:
     case 3:
       this.stmtDelete.bindByName("id", cookie.creationTime);
       break;
 
     case 4:
       this.stmtDelete.bindByName("name", cookie.name);
       this.stmtDelete.bindByName("host", cookie.host);
@@ -442,16 +495,19 @@ CookieDatabaseConnection.prototype =
 
   updateCookie: function(cookie)
   {
     if (!cookie instanceof Cookie)
       do_throw("not a cookie");
 
     switch (this.db.schemaVersion)
     {
+    case 1:
+      do_throw("can't update a schema 1 cookie!");
+
     case 2:
     case 3:
       this.stmtUpdate.bindByName("id", cookie.creationTime);
       this.stmtUpdate.bindByName("lastAccessed", cookie.lastAccessed);
       break;
 
     case 4:
       this.stmtDelete.bindByName("name", cookie.name);
@@ -466,17 +522,18 @@ CookieDatabaseConnection.prototype =
 
     do_execute_stmt(this.stmtUpdate);
   },
 
   close: function()
   {
     this.stmtInsert.finalize();
     this.stmtDelete.finalize();
-    this.stmtUpdate.finalize();
+    if (this.stmtUpdate)
+      this.stmtUpdate.finalize();
     this.db.close();
 
     this.stmtInsert = null;
     this.stmtDelete = null;
     this.stmtUpdate = null;
     this.db = null;
   }
 }
new file mode 100644
--- /dev/null
+++ b/extensions/cookie/test/unit/test_cookies_sync_failure.js
@@ -0,0 +1,277 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/
+ */
+
+// Test the various ways opening a cookie database can fail in a synchronous
+// (i.e. immediate) manner, and that the database is renamed and recreated
+// under each circumstance. These circumstances are, in no particular order:
+//
+// 1) A corrupt database, such that opening the connection fails.
+// 2) The 'moz_cookies' table doesn't exist.
+// 3) Not all of the expected columns exist, and statement creation fails when:
+//    a) The schema version is larger than the current version.
+//    b) The schema version is less than or equal to the current version.
+// 4) Migration fails. This will have different modes depending on the initial
+//    version:
+//    a) Schema 1: the 'lastAccessed' column already exists.
+//    b) Schema 2: the 'baseDomain' column already exists; or 'baseDomain'
+//       cannot be computed for a particular host.
+//    c) Schema 3: the 'creationTime' column already exists; or the
+//       'moz_uniqueid' index already exists.
+
+let test_generator = do_run_test();
+
+function run_test() {
+  do_test_pending();
+  do_run_generator(test_generator);
+}
+
+function finish_test() {
+  do_execute_soon(function() {
+    test_generator.close();
+    do_test_finished();
+  });
+}
+
+function do_run_test() {
+  // Set up a profile.
+  this.profile = do_get_profile();
+
+  // Get the cookie file and the backup file.
+  this.cookieFile = profile.clone();
+  cookieFile.append("cookies.sqlite");
+  this.backupFile = profile.clone();
+  backupFile.append("cookies.sqlite.bak");
+  do_check_false(cookieFile.exists());
+  do_check_false(backupFile.exists());
+
+  // Create a cookie object for testing.
+  this.now = Date.now() * 1000;
+  this.futureExpiry = Math.round(this.now / 1e6 + 1000);
+  this.cookie = new Cookie("oh", "hai", "bar.com", "/", this.futureExpiry,
+    this.now, this.now, false, false, false);
+
+  this.sub_generator = run_test_1(test_generator);
+  sub_generator.next();
+  yield;
+
+  this.sub_generator = run_test_2(test_generator);
+  sub_generator.next();
+  yield;
+
+  this.sub_generator = run_test_3(test_generator, 99);
+  sub_generator.next();
+  yield;
+
+  this.sub_generator = run_test_3(test_generator, 4);
+  sub_generator.next();
+  yield;
+
+  this.sub_generator = run_test_3(test_generator, 3);
+  sub_generator.next();
+  yield;
+
+  this.sub_generator = run_test_4_exists(test_generator, 1,
+    "ALTER TABLE moz_cookies ADD lastAccessed INTEGER");
+  sub_generator.next();
+  yield;
+
+  this.sub_generator = run_test_4_exists(test_generator, 2,
+    "ALTER TABLE moz_cookies ADD baseDomain TEXT");
+  sub_generator.next();
+  yield;
+
+  this.sub_generator = run_test_4_baseDomain(test_generator);
+  sub_generator.next();
+  yield;
+
+  this.sub_generator = run_test_4_exists(test_generator, 3,
+    "ALTER TABLE moz_cookies ADD creationTime INTEGER");
+  sub_generator.next();
+  yield;
+
+  this.sub_generator = run_test_4_exists(test_generator, 3,
+    "CREATE UNIQUE INDEX moz_uniqueid ON moz_cookies (name, host, path)");
+  sub_generator.next();
+  yield;
+
+  finish_test();
+  return;
+}
+
+const garbage = "hello thar!";
+
+function create_garbage_file(file)
+{
+  // Create an empty database file.
+  file.create(Ci.nsIFile.NORMAL_FILE_TYPE, -1);
+  do_check_true(file.exists());
+  do_check_eq(file.fileSize, 0);
+
+  // Write some garbage to it.
+  let ostream = Cc["@mozilla.org/network/file-output-stream;1"].
+                createInstance(Ci.nsIFileOutputStream);
+  ostream.init(file, -1, -1, 0);
+  ostream.write(garbage, garbage.length);
+  ostream.close();
+
+  file = file.clone(); // Windows maintains a stat cache. It's lame.
+  do_check_eq(file.fileSize, garbage.length);
+}
+
+function check_garbage_file(file)
+{
+  do_check_true(file.exists());
+  do_check_eq(file.fileSize, garbage.length);
+  file.remove(false);
+  do_check_false(file.exists());
+}
+
+function run_test_1(generator)
+{
+  // Create a garbage database file.
+  create_garbage_file(cookieFile);
+
+  // Load the profile and populate it.
+  let uri = NetUtil.newURI("http://foo.com/");
+  Services.cookies.setCookieString(uri, null, "oh=hai; max-age=1000", null);
+
+  // Fake a profile change.
+  do_close_profile(sub_generator);
+  yield;
+  do_load_profile();
+
+  // Check that the new database contains the cookie, and the old file was
+  // renamed.
+  do_check_eq(do_count_cookies(), 1);
+  check_garbage_file(backupFile);
+
+  // Close the profile.
+  do_close_profile(sub_generator);
+  yield;
+
+  // Clean up.
+  cookieFile.remove(false);
+  do_check_false(cookieFile.exists());
+  do_run_generator(generator);
+}
+
+function run_test_2(generator)
+{
+  // Load the profile and populate it.
+  do_load_profile();
+  let uri = NetUtil.newURI("http://foo.com/");
+  Services.cookies.setCookieString(uri, null, "oh=hai; max-age=1000", null);
+
+  // Fake a profile change.
+  do_close_profile(sub_generator);
+  yield;
+
+  // Drop the table.
+  let db = Services.storage.openDatabase(cookieFile);
+  db.executeSimpleSQL("DROP TABLE moz_cookies");
+  db.close();
+
+  // Load the profile and check that the table is recreated in-place.
+  do_load_profile();
+  do_check_eq(do_count_cookies(), 0);
+  do_check_false(backupFile.exists());
+
+  // Close the profile.
+  do_close_profile(sub_generator);
+  yield;
+
+  // Clean up.
+  cookieFile.remove(false);
+  do_check_false(cookieFile.exists());
+  do_run_generator(generator);
+}
+
+function run_test_3(generator, schema)
+{
+  // Manually create a schema 2 database, populate it, and set the schema
+  // version to the desired number.
+  let schema2db = new CookieDatabaseConnection(profile, 2);
+  schema2db.insertCookie(cookie);
+  schema2db.db.schemaVersion = schema;
+  schema2db.close();
+
+  // Load the profile and check that the column existence test fails.
+  do_load_profile();
+  do_check_eq(do_count_cookies(), 0);
+
+  // Close the profile.
+  do_close_profile(sub_generator);
+  yield;
+
+  // Check that the schema version has been reset.
+  let db = Services.storage.openDatabase(cookieFile);
+  do_check_eq(db.schemaVersion, 4);
+  db.close();
+
+  // Clean up.
+  cookieFile.remove(false);
+  do_check_false(cookieFile.exists());
+  do_run_generator(generator);
+}
+
+function run_test_4_exists(generator, schema, stmt)
+{
+  // Manually create a database, populate it, and add the desired column.
+  let db = new CookieDatabaseConnection(profile, schema);
+  db.insertCookie(cookie);
+  db.db.executeSimpleSQL(stmt);
+  db.close();
+
+  // Load the profile and check that migration fails.
+  do_load_profile();
+  do_check_eq(do_count_cookies(), 0);
+
+  // Close the profile.
+  do_close_profile(sub_generator);
+  yield;
+
+  // Check that the schema version has been reset and the backup file exists.
+  db = Services.storage.openDatabase(cookieFile);
+  do_check_eq(db.schemaVersion, 4);
+  db.close();
+  do_check_true(backupFile.exists());
+
+  // Clean up.
+  cookieFile.remove(false);
+  backupFile.remove(false);
+  do_check_false(cookieFile.exists());
+  do_check_false(backupFile.exists());
+  do_run_generator(generator);
+}
+
+function run_test_4_baseDomain(generator)
+{
+  // Manually create a database and populate it with a bad host.
+  let db = new CookieDatabaseConnection(profile, 2);
+  let badCookie = new Cookie("oh", "hai", ".", "/", this.futureExpiry, this.now,
+    this.now, false, false, false);
+  db.insertCookie(badCookie);
+  db.close();
+
+  // Load the profile and check that migration fails.
+  do_load_profile();
+  do_check_eq(do_count_cookies(), 0);
+
+  // Close the profile.
+  do_close_profile(sub_generator);
+  yield;
+
+  // Check that the schema version has been reset and the backup file exists.
+  db = Services.storage.openDatabase(cookieFile);
+  do_check_eq(db.schemaVersion, 4);
+  db.close();
+  do_check_true(backupFile.exists());
+
+  // Clean up.
+  cookieFile.remove(false);
+  backupFile.remove(false);
+  do_check_false(cookieFile.exists());
+  do_check_false(backupFile.exists());
+  do_run_generator(generator);
+}
--- a/netwerk/cookie/nsCookieService.cpp
+++ b/netwerk/cookie/nsCookieService.cpp
@@ -94,17 +94,17 @@ using namespace mozilla::net;
  ******************************************************************************/
 
 static nsCookieService *gCookieService;
 
 // XXX_hack. See bug 178993.
 // This is a hack to hide HttpOnly cookies from older browsers
 static const char kHttpOnlyPrefix[] = "#HttpOnly_";
 
-static const char kCookieFileName[] = "cookies.sqlite";
+#define COOKIES_FILE "cookies.sqlite"
 #define COOKIES_SCHEMA_VERSION 4
 
 static const PRInt64 kCookieStaleThreshold = 60 * PR_USEC_PER_SEC; // 1 minute in microseconds
 static const PRInt64 kCookiePurgeAge =
   PRInt64(30 * 24 * 60 * 60) * PR_USEC_PER_SEC; // 30 days in microseconds
 
 static const char kOldCookieFileName[] = "cookies.txt";
 
@@ -651,174 +651,206 @@ nsCookieService::Init()
   }
 
   return NS_OK;
 }
 
 void
 nsCookieService::InitDBStates()
 {
-  NS_ASSERTION(!mDBState, "already have a DB state");
-
-  // Create a new default DBState.
+  NS_ASSERTION(!mDBState, "already have a DBState");
+  NS_ASSERTION(!mDefaultDBState, "already have a default DBState");
+  NS_ASSERTION(!mPrivateDBState, "already have a private DBState");
+
+  // Create a new default DBState and set our current one.
   mDefaultDBState = new DBState();
   mDBState = mDefaultDBState;
 
-  // attempt to open and read the database
-  nsresult rv = TryInitDB(PR_FALSE);
-  if (rv == NS_ERROR_FILE_CORRUPTED) {
-    COOKIE_LOGSTRING(PR_LOG_WARNING, ("InitDB(): db corrupt, trying again", rv));
-
-    // Synchronously close the connection, clean up the default DBState, and try
-    // again.
-    CloseDefaultDBConnection();
-    rv = TryInitDB(PR_TRUE);
-  }
-
-  if (NS_FAILED(rv)) {
-    // Reset our DB connection and statements, and run without a db connection.
-    // This is nonfatal -- we can run fine without persistent storage, e.g. if
-    // there's no profile.
-    CloseDefaultDBConnection();
-    COOKIE_LOGSTRING(PR_LOG_WARNING, ("TryInitDB() gave error %x", rv));
-  }
-
-  // If we're in private browsing mode, switch the DBState over.
+  // If we're in private browsing mode, create a private DBState.
   nsCOMPtr<nsIPrivateBrowsingService> pbs =
     do_GetService(NS_PRIVATE_BROWSING_SERVICE_CONTRACTID);
   if (pbs) {
     PRBool inPrivateBrowsing = PR_FALSE;
     pbs->GetPrivateBrowsingEnabled(&inPrivateBrowsing);
     if (inPrivateBrowsing) {
       mPrivateDBState = new DBState();
       mDBState = mPrivateDBState;
     }
   }
+
+  // Get our cookie file.
+  nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR,
+    getter_AddRefs(mDefaultDBState->cookieFile));
+  if (NS_FAILED(rv)) {
+    // We've already set up our DBStates appropriately; nothing more to do.
+    COOKIE_LOGSTRING(PR_LOG_WARNING,
+      ("InitDBStates(): couldn't get cookie file"));
+    return;
+  }
+  mDefaultDBState->cookieFile->AppendNative(NS_LITERAL_CSTRING(COOKIES_FILE));
+
+  // Attempt to open and read the database. If TryInitDB() returns RESULT_RETRY,
+  // do so.
+  OpenDBResult result = TryInitDB(false);
+  if (result == RESULT_RETRY) {
+    // Database may be corrupt. Synchronously close the connection, clean up the
+    // default DBState, and try again.
+    COOKIE_LOGSTRING(PR_LOG_WARNING, ("InitDBStates(): retrying TryInitDB()"));
+
+    CloseDefaultDBConnection();
+    result = TryInitDB(true);
+    if (result == RESULT_RETRY) {
+      // We're done. Change the code to failure so we clean up below.
+      result = RESULT_FAILURE;
+    }
+  }
+
+  if (result == RESULT_FAILURE) {
+    COOKIE_LOGSTRING(PR_LOG_WARNING,
+      ("InitDBStates(): TryInitDB() failed, closing connection"));
+
+    // Connection failure is unrecoverable. Clean up our connection. We can run
+    // fine without persistent storage -- e.g. if there's no profile.
+    CloseDefaultDBConnection();
+  }
 }
 
-nsresult
-nsCookieService::TryInitDB(PRBool aDeleteExistingDB)
+/* Attempt to open and read the database. If 'aRecreateDB' is true, try to
+ * move the existing database file out of the way and create a new one.
+ *
+ * @returns RESULT_OK if opening or creating the database succeeded;
+ *          RESULT_RETRY if the database cannot be opened, is corrupt, or some
+ *          other failure occurred that might be resolved by recreating the
+ *          database; or RESULT_FAILED if there was an unrecoverable error and
+ *          we must run without a database.
+ *
+ * If RESULT_RETRY or RESULT_FAILED is returned, the caller should perform
+ * cleanup of the default DBState.
+ */
+OpenDBResult
+nsCookieService::TryInitDB(bool aRecreateDB)
 {
   NS_ASSERTION(!mDefaultDBState->dbConn, "nonnull dbConn");
   NS_ASSERTION(!mDefaultDBState->stmtInsert, "nonnull stmtInsert");
   NS_ASSERTION(!mDefaultDBState->insertListener, "nonnull insertListener");
   NS_ASSERTION(!mDefaultDBState->syncConn, "nonnull syncConn");
 
-  nsCOMPtr<nsIFile> cookieFile;
-  nsresult rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR,
-    getter_AddRefs(cookieFile));
-  if (NS_FAILED(rv)) return rv;
-
-  cookieFile->AppendNative(NS_LITERAL_CSTRING(kCookieFileName));
-
-  // remove an existing db, if we've been told to (i.e. it's corrupt)
-  if (aDeleteExistingDB) {
-    rv = cookieFile->Remove(PR_FALSE);
-    NS_ENSURE_SUCCESS(rv, rv);
+  // Ditch an existing db, if we've been told to (i.e. it's corrupt). We don't
+  // want to delete it outright, since it may be useful for debugging purposes,
+  // so we move it out of the way.
+  nsresult rv;
+  if (aRecreateDB) {
+    nsCOMPtr<nsIFile> backupFile;
+    mDefaultDBState->cookieFile->Clone(getter_AddRefs(backupFile));
+    rv = backupFile->MoveToNative(NULL,
+      NS_LITERAL_CSTRING(COOKIES_FILE ".bak"));
+    NS_ENSURE_SUCCESS(rv, RESULT_FAILURE);
   }
 
   // open a connection to the cookie database, and only cache our connection
   // and statements upon success. The connection is opened unshared to eliminate
   // cache contention between the main and background threads.
-  rv = mStorageService->OpenUnsharedDatabase(cookieFile,
+  rv = mStorageService->OpenUnsharedDatabase(mDefaultDBState->cookieFile,
     getter_AddRefs(mDefaultDBState->dbConn));
-  NS_ENSURE_SUCCESS(rv, rv);
+  NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
   // Set up our listeners.
   mDefaultDBState->insertListener = new InsertCookieDBListener(mDefaultDBState);
   mDefaultDBState->updateListener = new UpdateCookieDBListener(mDefaultDBState);
   mDefaultDBState->removeListener = new RemoveCookieDBListener(mDefaultDBState);
   mDefaultDBState->closeListener = new CloseCookieDBListener(mDefaultDBState);
 
   // Grow cookie db in 512KB increments
   mDefaultDBState->dbConn->SetGrowthIncrement(512 * 1024, EmptyCString());
 
   PRBool tableExists = PR_FALSE;
   mDefaultDBState->dbConn->TableExists(NS_LITERAL_CSTRING("moz_cookies"),
     &tableExists);
   if (!tableExists) {
     rv = CreateTable();
-    NS_ENSURE_SUCCESS(rv, rv);
+    NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
   } else {
     // table already exists; check the schema version before reading
     PRInt32 dbSchemaVersion;
     rv = mDefaultDBState->dbConn->GetSchemaVersion(&dbSchemaVersion);
-    NS_ENSURE_SUCCESS(rv, rv);
+    NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
     // Start a transaction for the whole migration block.
     mozStorageTransaction transaction(mDefaultDBState->dbConn, PR_TRUE);
 
     switch (dbSchemaVersion) {
-    // upgrading.
-    // every time you increment the database schema, you need to implement
-    // the upgrading code from the previous version to the new one.
+    // Upgrading.
+    // Every time you increment the database schema, you need to implement
+    // the upgrading code from the previous version to the new one. If migration
+    // fails for any reason, it's a bug -- so we return RESULT_RETRY such that
+    // the original database will be saved, in the hopes that we might one day
+    // see it and fix it.
     case 1:
       {
         // Add the lastAccessed column to the table.
         rv = mDefaultDBState->dbConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
           "ALTER TABLE moz_cookies ADD lastAccessed INTEGER"));
-        NS_ENSURE_SUCCESS(rv, rv);
+        NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
       }
       // Fall through to the next upgrade.
 
     case 2:
       {
         // Add the baseDomain column and index to the table.
         rv = mDefaultDBState->dbConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
           "ALTER TABLE moz_cookies ADD baseDomain TEXT"));
-        NS_ENSURE_SUCCESS(rv, rv);
+        NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
         // Compute the baseDomains for the table. This must be done eagerly
         // otherwise we won't be able to synchronously read in individual
         // domains on demand.
         nsCOMPtr<mozIStorageStatement> select;
         rv = mDefaultDBState->dbConn->CreateStatement(NS_LITERAL_CSTRING(
           "SELECT id, host FROM moz_cookies"), getter_AddRefs(select));
-        NS_ENSURE_SUCCESS(rv, rv);
+        NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
         nsCOMPtr<mozIStorageStatement> update;
         rv = mDefaultDBState->dbConn->CreateStatement(NS_LITERAL_CSTRING(
           "UPDATE moz_cookies SET baseDomain = :baseDomain WHERE id = :id"),
           getter_AddRefs(update));
-        NS_ENSURE_SUCCESS(rv, rv);
+        NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
         nsCString baseDomain, host;
         PRBool hasResult;
         while (1) {
           rv = select->ExecuteStep(&hasResult);
-          NS_ENSURE_SUCCESS(rv, rv);
+          NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
           if (!hasResult)
             break;
 
           PRInt64 id = select->AsInt64(0);
           select->GetUTF8String(1, host);
 
           rv = GetBaseDomainFromHost(host, baseDomain);
-          if (NS_FAILED(rv))
-            continue;
+          NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
           mozStorageStatementScoper scoper(update);
 
           rv = update->BindUTF8StringByName(NS_LITERAL_CSTRING("baseDomain"),
                                             baseDomain);
           NS_ASSERT_SUCCESS(rv);
           rv = update->BindInt64ByName(NS_LITERAL_CSTRING("id"),
                                        id);
           NS_ASSERT_SUCCESS(rv);
 
           rv = update->ExecuteStep(&hasResult);
-          NS_ENSURE_SUCCESS(rv, rv);
+          NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
         }
 
         // Create an index on baseDomain.
         rv = mDefaultDBState->dbConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
           "CREATE INDEX moz_basedomain ON moz_cookies (baseDomain)"));
-        NS_ENSURE_SUCCESS(rv, rv);
+        NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
       }
       // Fall through to the next upgrade.
 
     case 3:
       {
         // Add the creationTime column to the table, and create a unique index
         // on (name, host, path). Before we do this, we have to purge the table
         // of expired cookies such that we know that the (name, host, path)
@@ -830,41 +862,41 @@ nsCookieService::TryInitDB(PRBool aDelet
         // Select the whole table, and order by the fields we're interested in.
         // This means we can simply do a linear traversal of the results and
         // check for duplicates as we go.
         nsCOMPtr<mozIStorageStatement> select;
         rv = mDefaultDBState->dbConn->CreateStatement(NS_LITERAL_CSTRING(
           "SELECT id, name, host, path FROM moz_cookies "
             "ORDER BY name ASC, host ASC, path ASC, expiry ASC"),
           getter_AddRefs(select));
-        NS_ENSURE_SUCCESS(rv, rv);
+        NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
         nsCOMPtr<mozIStorageStatement> deleteExpired;
         rv = mDefaultDBState->dbConn->CreateStatement(NS_LITERAL_CSTRING(
           "DELETE FROM moz_cookies WHERE id = :id"),
           getter_AddRefs(deleteExpired));
-        NS_ENSURE_SUCCESS(rv, rv);
+        NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
         // Read the first row.
         PRBool hasResult;
         rv = select->ExecuteStep(&hasResult);
-        NS_ENSURE_SUCCESS(rv, rv);
+        NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
         if (hasResult) {
           nsCString name1, host1, path1;
           PRInt64 id1 = select->AsInt64(0);
           select->GetUTF8String(1, name1);
           select->GetUTF8String(2, host1);
           select->GetUTF8String(3, path1);
 
           nsCString name2, host2, path2;
           while (1) {
             // Read the second row.
             rv = select->ExecuteStep(&hasResult);
-            NS_ENSURE_SUCCESS(rv, rv);
+            NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
             if (!hasResult)
               break;
 
             PRInt64 id2 = select->AsInt64(0);
             select->GetUTF8String(1, name2);
             select->GetUTF8String(2, host2);
             select->GetUTF8String(3, path2);
@@ -874,64 +906,64 @@ nsCookieService::TryInitDB(PRBool aDelet
             if (name1 == name2 && host1 == host2 && path1 == path2) {
               mozStorageStatementScoper scoper(deleteExpired);
 
               rv = deleteExpired->BindInt64ByName(NS_LITERAL_CSTRING("id"),
                 id1);
               NS_ASSERT_SUCCESS(rv);
 
               rv = deleteExpired->ExecuteStep(&hasResult);
-              NS_ENSURE_SUCCESS(rv, rv);
+              NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
             }
 
             // Make the second row the first for the next iteration.
             name1 = name2;
             host1 = host2;
             path1 = path2;
             id1 = id2;
           }
         }
 
         // Add the creationTime column to the table.
         rv = mDefaultDBState->dbConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
           "ALTER TABLE moz_cookies ADD creationTime INTEGER"));
-        NS_ENSURE_SUCCESS(rv, rv);
+        NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
         // Copy the id of each row into the new creationTime column.
         rv = mDefaultDBState->dbConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
           "UPDATE moz_cookies SET creationTime = "
             "(SELECT id WHERE id = moz_cookies.id)"));
-        NS_ENSURE_SUCCESS(rv, rv);
+        NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
         // Create a unique index on (name, host, path) to allow fast lookup.
         rv = mDefaultDBState->dbConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
           "CREATE UNIQUE INDEX moz_uniqueid "
           "ON moz_cookies (name, host, path)"));
-        NS_ENSURE_SUCCESS(rv, rv);
+        NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
       }
       // Fall through to the next upgrade.
 
       // No more upgrades. Update the schema version.
       rv = mDefaultDBState->dbConn->SetSchemaVersion(COOKIES_SCHEMA_VERSION);
-      NS_ENSURE_SUCCESS(rv, rv);
+      NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
     case COOKIES_SCHEMA_VERSION:
       break;
 
     case 0:
       {
         NS_WARNING("couldn't get schema version!");
           
         // the table may be usable; someone might've just clobbered the schema
         // version. we can treat this case like a downgrade using the codepath
         // below, by verifying the columns we care about are all there. for now,
         // re-set the schema version in the db, in case the checks succeed (if
         // they don't, we're dropping the table anyway).
         rv = mDefaultDBState->dbConn->SetSchemaVersion(COOKIES_SCHEMA_VERSION);
-        NS_ENSURE_SUCCESS(rv, rv);
+        NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
       }
       // fall through to downgrade check
 
     // downgrading.
     // if columns have been added to the table, we can still use the ones we
     // understand safely. if columns have been deleted or altered, just
     // blow away the table and start from scratch! if you change the way
     // a column is interpreted, make sure you also change its name so this
@@ -955,20 +987,20 @@ nsCookieService::TryInitDB(PRBool aDelet
             "isHttpOnly "
           "FROM moz_cookies"), getter_AddRefs(stmt));
         if (NS_SUCCEEDED(rv))
           break;
 
         // our columns aren't there - drop the table!
         rv = mDefaultDBState->dbConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
           "DROP TABLE moz_cookies"));
-        NS_ENSURE_SUCCESS(rv, rv);
+        NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
         rv = CreateTable();
-        NS_ENSURE_SUCCESS(rv, rv);
+        NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
       }
       break;
     }
   }
 
   // make operations on the table asynchronous, for performance
   mDefaultDBState->dbConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
     "PRAGMA synchronous = OFF"));
@@ -1001,55 +1033,54 @@ nsCookieService::TryInitDB(PRBool aDelet
       ":path, "
       ":expiry, "
       ":lastAccessed, "
       ":creationTime, "
       ":isSecure, "
       ":isHttpOnly"
     ")"),
     getter_AddRefs(mDefaultDBState->stmtInsert));
-  NS_ENSURE_SUCCESS(rv, rv);
+  NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
   rv = mDefaultDBState->dbConn->CreateAsyncStatement(NS_LITERAL_CSTRING(
     "DELETE FROM moz_cookies "
     "WHERE name = :name AND host = :host AND path = :path"),
     getter_AddRefs(mDefaultDBState->stmtDelete));
-  NS_ENSURE_SUCCESS(rv, rv);
+  NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
   rv = mDefaultDBState->dbConn->CreateAsyncStatement(NS_LITERAL_CSTRING(
     "UPDATE moz_cookies SET lastAccessed = :lastAccessed "
     "WHERE name = :name AND host = :host AND path = :path"),
     getter_AddRefs(mDefaultDBState->stmtUpdate));
-  NS_ENSURE_SUCCESS(rv, rv);
+  NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
   // if we deleted a corrupt db, don't attempt to import - return now
-  if (aDeleteExistingDB)
-    return NS_OK;
+  if (aRecreateDB)
+    return RESULT_OK;
 
   // check whether to import or just read in the db
   if (tableExists)
     return Read();
 
   nsCOMPtr<nsIFile> oldCookieFile;
   rv = NS_GetSpecialDirectory(NS_APP_USER_PROFILE_50_DIR,
     getter_AddRefs(oldCookieFile));
-  if (NS_FAILED(rv)) return rv;
-
+  if (NS_FAILED(rv)) return RESULT_OK;
+
+  // Import cookies, and clean up the old file regardless of success or failure.
+  // Note that we have to switch out our DBState temporarily, in case we're in
+  // private browsing mode; otherwise ImportCookies() won't be happy.
+  DBState* initialState = mDBState;
+  mDBState = mDefaultDBState;
   oldCookieFile->AppendNative(NS_LITERAL_CSTRING(kOldCookieFileName));
-  rv = ImportCookies(oldCookieFile);
-  if (NS_FAILED(rv)) {
-    if (rv == NS_ERROR_FILE_NOT_FOUND)
-      return NS_OK;
-
-    return rv;
-  }
-
-  // we're done importing - delete the old cookie file
+  ImportCookies(oldCookieFile);
   oldCookieFile->Remove(PR_FALSE);
-  return NS_OK;
+  mDBState = initialState;
+
+  return RESULT_OK;
 }
 
 // Sets the schema version and creates the moz_cookies table.
 nsresult
 nsCookieService::CreateTable()
 {
   // Set the schema version, before creating the table.
   nsresult rv = mDefaultDBState->dbConn->SetSchemaVersion(
@@ -1548,17 +1579,18 @@ nsCookieService::Remove(const nsACString
   return NS_OK;
 }
 
 /******************************************************************************
  * nsCookieService impl:
  * private file I/O functions
  ******************************************************************************/
 
-nsresult
+// Begin an asynchronous read from the database.
+OpenDBResult
 nsCookieService::Read()
 {
   // Set up a statement for the read. Note that our query specifies that
   // 'baseDomain' not be NULL -- see below for why.
   nsCOMPtr<mozIStorageStatement> stmtRead;
   nsresult rv = mDefaultDBState->dbConn->CreateStatement(NS_LITERAL_CSTRING(
     "SELECT "
       "name, "
@@ -1568,44 +1600,44 @@ nsCookieService::Read()
       "expiry, "
       "lastAccessed, "
       "creationTime, "
       "isSecure, "
       "isHttpOnly, "
       "baseDomain "
     "FROM moz_cookies "
     "WHERE baseDomain NOTNULL"), getter_AddRefs(stmtRead));
-  NS_ENSURE_SUCCESS(rv, rv);
+  NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
   // Set up a statement to delete any rows with a NULL 'baseDomain'
   // column. This takes care of any cookies set by browsers that don't
   // understand the 'baseDomain' column, where the database schema version
   // is from one that does. (This would occur when downgrading.)
   nsCOMPtr<mozIStorageStatement> stmtDeleteNull;
   rv = mDefaultDBState->dbConn->CreateStatement(NS_LITERAL_CSTRING(
     "DELETE FROM moz_cookies WHERE baseDomain ISNULL"),
     getter_AddRefs(stmtDeleteNull));
-  NS_ENSURE_SUCCESS(rv, rv);
+  NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
 
   // Init our readSet hash and execute the statements. Note that, after this
   // point, we cannot fail without altering the cleanup code in InitDBStates()
   // to handle closing of the now-asynchronous connection.
   mDefaultDBState->readSet.Init();
 
   mDefaultDBState->readListener = new ReadCookieDBListener(mDefaultDBState);
   rv = stmtRead->ExecuteAsync(mDefaultDBState->readListener,
     getter_AddRefs(mDefaultDBState->pendingRead));
   NS_ASSERT_SUCCESS(rv);
 
   nsCOMPtr<mozIStoragePendingStatement> handle;
   rv = stmtDeleteNull->ExecuteAsync(mDefaultDBState->removeListener,
     getter_AddRefs(handle));
   NS_ASSERT_SUCCESS(rv);
 
-  return NS_OK;
+  return RESULT_OK;
 }
 
 // Extract data from a single result row and create an nsCookie.
 // This is templated since 'T' is different for sync vs async results.
 template<class T> nsCookie*
 nsCookieService::GetCookieFromRow(T &aRow)
 {
   // Skip reading 'baseDomain' -- up to the caller.
@@ -1700,24 +1732,21 @@ nsCookieService::CancelAsyncRead(PRBool 
   if (aPurgeReadSet)
     mDefaultDBState->readSet.Clear();
 }
 
 mozIStorageConnection*
 nsCookieService::GetSyncDBConn()
 {
   NS_ASSERTION(!mDefaultDBState->syncConn, "already have sync db connection");
+  NS_ASSERTION(mDefaultDBState->cookieFile, "no cookie file");
 
   // Start a new connection for sync reads to reduce contention with the
   // background thread.
-  nsCOMPtr<nsIFile> cookieFile;
-  mDefaultDBState->dbConn->GetDatabaseFile(getter_AddRefs(cookieFile));
-  NS_ASSERTION(cookieFile, "no cookie file on connection");
-
-  mStorageService->OpenUnsharedDatabase(cookieFile,
+  mStorageService->OpenUnsharedDatabase(mDefaultDBState->cookieFile,
     getter_AddRefs(mDefaultDBState->syncConn));
   if (!mDefaultDBState->syncConn) {
     NS_ERROR("can't open sync db connection");
     COOKIE_LOGSTRING(PR_LOG_DEBUG,
       ("GetSyncDBConn(): can't open sync db connection"));
   }
 
   return mDefaultDBState->syncConn;
@@ -3566,8 +3595,9 @@ nsCookieService::UpdateCookieInList(nsCo
                                       aCookie->Path());
     NS_ASSERT_SUCCESS(rv);
 
     // Add our bound parameters to the array.
     rv = aParamsArray->AddParams(params);
     NS_ASSERT_SUCCESS(rv);
   }
 }
+
--- a/netwerk/cookie/nsCookieService.h
+++ b/netwerk/cookie/nsCookieService.h
@@ -156,16 +156,17 @@ struct DBState
     hostTable.Init();
   }
 
   NS_INLINE_DECL_REFCOUNTING(DBState)
 
   nsTHashtable<nsCookieEntry>     hostTable;
   PRUint32                        cookieCount;
   PRInt64                         cookieOldestTime;
+  nsCOMPtr<nsIFile>               cookieFile;
   nsCOMPtr<mozIStorageConnection> dbConn;
   nsCOMPtr<mozIStorageAsyncStatement> stmtInsert;
   nsCOMPtr<mozIStorageAsyncStatement> stmtDelete;
   nsCOMPtr<mozIStorageAsyncStatement> stmtUpdate;
 
   // Various parts representing asynchronous read state. These are useful
   // while the background read is taking place.
   nsCOMPtr<mozIStorageConnection>       syncConn;
@@ -197,16 +198,24 @@ enum CookieStatus
   STATUS_REJECTED,
   // STATUS_REJECTED_WITH_ERROR indicates the cookie should be rejected because
   // of an error (rather than something the user can control). this is used for
   // notification purposes, since we only want to notify of rejections where
   // the user can do something about it (e.g. whitelist the site).
   STATUS_REJECTED_WITH_ERROR
 };
 
+// Result codes for TryInitDB() and Read().
+enum OpenDBResult
+{
+  RESULT_OK,
+  RESULT_RETRY,
+  RESULT_FAILURE
+};
+
 /******************************************************************************
  * nsCookieService:
  * class declaration
  ******************************************************************************/
 
 class nsCookieService : public nsICookieService
                       , public nsICookieManager2
                       , public nsIObserver
@@ -223,21 +232,21 @@ class nsCookieService : public nsICookie
     nsCookieService();
     virtual ~nsCookieService();
     static nsICookieService*      GetXPCOMSingleton();
     nsresult                      Init();
 
   protected:
     void                          PrefChanged(nsIPrefBranch *aPrefBranch);
     void                          InitDBStates();
-    nsresult                      TryInitDB(PRBool aDeleteExistingDB);
+    OpenDBResult                  TryInitDB(bool aDeleteExistingDB);
     nsresult                      CreateTable();
     void                          CloseDBStates();
     void                          CloseDefaultDBConnection();
-    nsresult                      Read();
+    OpenDBResult                  Read();
     template<class T> nsCookie*   GetCookieFromRow(T &aRow);
     void                          AsyncReadComplete();
     void                          CancelAsyncRead(PRBool aPurgeReadSet);
     mozIStorageConnection*        GetSyncDBConn();
     void                          EnsureReadDomain(const nsCString &aBaseDomain);
     void                          EnsureReadComplete();
     nsresult                      NormalizeHost(nsCString &aHost);
     nsresult                      GetBaseDomain(nsIURI *aHostURI, nsCString &aBaseDomain, PRBool &aRequireHostMatch);