Bug 423273 - Fix storage API to not break old behavior. r=shaver, sr=shaver, a=shaver

netwerk/cookie/src/nsCookieService.cpp

@@ -468,22 +468,14 @@ nsCookieService::InitDB()
 
   // cache a connection to the cookie database
   rv = storage->OpenDatabase(cookieFile, getter_AddRefs(mDBConn));
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  PRBool ready;
-  mDBConn->GetConnectionReady(&ready);
-  if (!ready) {
+  if (rv == NS_ERROR_FILE_CORRUPTED) {
     // delete and try again
     rv = cookieFile->Remove(PR_FALSE);
     NS_ENSURE_SUCCESS(rv, rv);
 
     rv = storage->OpenDatabase(cookieFile, getter_AddRefs(mDBConn));
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    mDBConn->GetConnectionReady(&ready);
-    if (!ready)
-      return NS_ERROR_UNEXPECTED;
   }
+  NS_ENSURE_SUCCESS(rv, rv);
 
   PRBool tableExists = PR_FALSE;
   mDBConn->TableExists(NS_LITERAL_CSTRING("moz_cookies"), &tableExists);

storage/public/mozIStorageConnection.idl

@@ -54,7 +54,7 @@ interface nsIFile;
  * creating prepared statements, executing SQL, and examining database
  * errors.
  */
-[scriptable, uuid(ffddc17b-cec3-492c-b13e-d5393c4b1595)]
+[scriptable, uuid(623f9ddb-434b-4d39-bc2d-1c617da241d0)]
 interface mozIStorageConnection : nsISupports {
   /*
    * Initialization and status
@@ -68,9 +68,7 @@ interface mozIStorageConnection : nsISupports {
 
   /**
    * Indicates if the connection is open and ready to use.  This will be false
-   * if the connection failed to open, or it has been closed.  It is strongly
-   * recommended that the database be backed up before removing the file by
-   * calling backupDB.
+   * if the connection failed to open, or it has been closed.
    */
   readonly attribute boolean connectionReady;
 
@@ -259,23 +257,4 @@ interface mozIStorageConnection : nsISupports {
    * @return previous registered handler.
    */
   mozIStorageProgressHandler removeProgressHandler();
-
-  /*
-   * Utilities
-   */
-
-  /**
-   * Copies the current database file to the specified parent directory with the
-   * specified file name.  If the parent directory is not specified, it places
-   * the backup in the same directory as the current file.  This function
-   * ensures that the file being created is unique.
-   *
-   * @param aFileName
-   *        The name of the new file to create.
-   * @param [optional] aParentDirectory
-   *        The directory you'd like the file to be placed.
-   * @return The nsIFile representing the backup file.
-   */
-  nsIFile backupDB(in AString aFileName,
-                   [optional] in nsIFile aParentDirectory);
 };

storage/public/mozIStorageService.idl

@@ -48,7 +48,7 @@ interface nsIFile;
  *
  * This is the only way to open a database connection.
  */
-[scriptable, uuid(336d2741-8438-449d-8746-8c37c62a2ccb)]
+[scriptable, uuid(fe8e95cb-b377-4c8d-bccb-d9198c67542b)]
 interface mozIStorageService : nsISupports {
   /**
    * Get a connection to a named special database storage.
@@ -100,6 +100,8 @@ interface mozIStorageService : nsISupports {
    *
    * @throws NS_ERROR_OUT_OF_MEMORY
    *         If allocating a new storage object fails.
+   * @throws NS_ERROR_FILE_CORRUPTED
+   *         If the database file is corrupted.
    */
   mozIStorageConnection openDatabase(in nsIFile aDatabaseFile);
 
@@ -135,9 +137,31 @@ interface mozIStorageService : nsISupports {
    *
    * @throws NS_ERROR_OUT_OF_MEMORY
    *         If allocating a new storage object fails.
+   * @throws NS_ERROR_FILE_CORRUPTED
+   *         If the database file is corrupted.
    */
   mozIStorageConnection openUnsharedDatabase(in nsIFile aDatabaseFile);
 
+  /*
+   * Utilities
+   */
+
+  /**
+   * Copies the specified database file to the specified parent directory with
+   * the specified file name.  If the parent directory is not specified, it
+   * places the backup in the same directory as the current file.  This function
+   * ensures that the file being created is unique.
+   *
+   * @param aDBFile
+   *        The database file that will be backed up.
+   * @param aBackupFileName
+   *        The name of the new backup file to create.
+   * @param [optional] aBackupParentDirectory
+   *        The directory you'd like the backup file to be placed.
+   * @return The nsIFile representing the backup file.
+   */
+  nsIFile backupDatabaseFile(in nsIFile aDBFile, in AString aBackupFileName,
+                             [optional] in nsIFile aBackupParentDirectory);
 };
 
 %{C++

storage/src/mozStorageConnection.cpp

@@ -823,48 +823,6 @@ mozStorageConnection::ProgressHandler()
     return 0;
 }
 
-/**
- ** Utilities
- **/
-
-NS_IMETHODIMP
-mozStorageConnection::BackupDB(const nsAString &aFileName,
-                               nsIFile *aParentDirectory,
-                               nsIFile **backup)
-{
-    NS_ASSERTION(mDatabaseFile, "No database file to backup!");
-
-    nsresult rv;
-    nsCOMPtr<nsIFile> parentDir = aParentDirectory;
-    if (!parentDir) {
-        // This argument is optional, and defaults to the same parent directory
-        // as the current file.
-        rv = mDatabaseFile->GetParent(getter_AddRefs(parentDir));
-        NS_ENSURE_SUCCESS(rv, rv);
-    }
-
-    nsCOMPtr<nsIFile> backupDB;
-    rv = parentDir->Clone(getter_AddRefs(backupDB));
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    rv = backupDB->Append(aFileName);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    rv = backupDB->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    nsAutoString fileName;
-    rv = backupDB->GetLeafName(fileName);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    rv = backupDB->Remove(PR_FALSE);
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    backupDB.swap(*backup);
-
-    return mDatabaseFile->CopyTo(parentDir, fileName);
-}
-
 /**
  ** Other bits
  **/

storage/src/mozStorageService.cpp

@@ -177,18 +177,17 @@ mozStorageService::OpenSpecialDatabase(const char *aStorageKey, mozIStorageConne
 NS_IMETHODIMP
 mozStorageService::OpenDatabase(nsIFile *aDatabaseFile, mozIStorageConnection **_retval)
 {
-    mozStorageConnection *msc = new mozStorageConnection(this);
+    nsRefPtr<mozStorageConnection> msc = new mozStorageConnection(this);
     if (!msc)
         return NS_ERROR_OUT_OF_MEMORY;
 
-    // We want to return a valid connection regardless if it succeeded or not so
-    // that consumers can backup the database if it failed.
     {
         nsAutoLock lock(mLock);
-        (void)msc->Initialize(aDatabaseFile);
+        nsresult rv = msc->Initialize(aDatabaseFile);
+        NS_ENSURE_SUCCESS(rv, rv);
     }
-    NS_ADDREF(*_retval = msc);
 
+    NS_ADDREF(*_retval = msc);
     return NS_OK;
 }
 
@@ -206,21 +205,63 @@ mozStorageService::OpenUnsharedDatabase(nsIFile *aDatabaseFile, mozIStorageConne
     // lifetimes, unaffected by changes to the shared caches setting, so we can
     // disable shared caches temporarily while we initialize the new connection
     // without affecting the caches currently in use by other connections.
-    // We want to return a valid connection regardless if it succeeded or not so
-    // that consumers can backup the database if it failed.
+    nsresult rv;
     {
         nsAutoLock lock(mLock);
         int rc = sqlite3_enable_shared_cache(0);
         if (rc != SQLITE_OK)
             return ConvertResultCode(rc);
 
-        (void)msc->Initialize(aDatabaseFile);
+        rv = msc->Initialize(aDatabaseFile);
 
         rc = sqlite3_enable_shared_cache(1);
         if (rc != SQLITE_OK)
             return ConvertResultCode(rc);
     }
-    NS_ADDREF(*_retval = msc);
+    NS_ENSURE_SUCCESS(rv, rv);
 
+    NS_ADDREF(*_retval = msc);
     return NS_OK;
 }
+
+/**
+ ** Utilities
+ **/
+
+NS_IMETHODIMP
+mozStorageService::BackupDatabaseFile(nsIFile *aDBFile,
+                                      const nsAString &aBackupFileName,
+                                      nsIFile *aBackupParentDirectory,
+                                      nsIFile **backup)
+{
+    nsresult rv;
+    nsCOMPtr<nsIFile> parentDir = aBackupParentDirectory;
+    if (!parentDir) {
+        // This argument is optional, and defaults to the same parent directory
+        // as the current file.
+        rv = aDBFile->GetParent(getter_AddRefs(parentDir));
+        NS_ENSURE_SUCCESS(rv, rv);
+    }
+
+    nsCOMPtr<nsIFile> backupDB;
+    rv = parentDir->Clone(getter_AddRefs(backupDB));
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    rv = backupDB->Append(aBackupFileName);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    rv = backupDB->CreateUnique(nsIFile::NORMAL_FILE_TYPE, 0600);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    nsAutoString fileName;
+    rv = backupDB->GetLeafName(fileName);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    rv = backupDB->Remove(PR_FALSE);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    backupDB.swap(*backup);
+
+    return aDBFile->CopyTo(parentDir, fileName);
+}
+

storage/test/unit/head_storage.js

@@ -49,6 +49,14 @@ function getTestDB()
   return db;
 }
 
+/**
+ * Obtains a corrupt database to test against.
+ */
+function getCorruptDB()
+{
+  return do_get_file("storage/test/unit/corruptDB.sqlite");
+}
+
 function cleanup()
 {
   // close the connection

storage/test/unit/test_bug-416173.js

@@ -1,68 +0,0 @@
-/* ***** BEGIN LICENSE BLOCK *****
- * Version: MPL 1.1/GPL 2.0/LGPL 2.1
- *
- * The contents of this file are subject to the Mozilla Public License Version
- * 1.1 (the "License"); you may not use this file except in compliance with
- * the License. You may obtain a copy of the License at
- * http://www.mozilla.org/MPL/
- *
- * Software distributed under the License is distributed on an "AS IS" basis,
- * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
- * for the specific language governing rights and limitations under the
- * License.
- *
- * The Original Code is Storage Test Code.
- *
- * The Initial Developer of the Original Code is
- * Mozilla Corporation.
- * Portions created by the Initial Developer are Copyright (C) 2008
- * the Initial Developer. All Rights Reserved.
- *
- * This code is based off of like.test from the sqlite code
- *
- * Contributor(s):
- *   Shawn Wilsher <me@shawnwilsher.com> (Original Author)
- *
- * Alternatively, the contents of this file may be used under the terms of
- * either the GNU General Public License Version 2 or later (the "GPL"), or
- * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
- * in which case the provisions of the GPL or the LGPL are applicable instead
- * of those above. If you wish to allow use of your version of this file only
- * under the terms of either the GPL or the LGPL, and not to allow others to
- * use your version of this file under the terms of the MPL, indicate your
- * decision by deleting the provisions above and replace them with the notice
- * and other provisions required by the GPL or the LGPL. If you do not delete
- * the provisions above, a recipient may use your version of this file under
- * the terms of any one of the MPL, the GPL or the LGPL.
- *
- * ***** END LICENSE BLOCK ***** */
-
-// Tests to make sure that mozIStorageConnection::backupDB works when trying to
-// open a corrupted database.
-
-const BACKUP_FILE_NAME = "test_storage.sqlite.backup";
-
-function test_backup_bad_connection()
-{
-  var msc = getDatabase(do_get_file("storage/test/unit/corruptDB.sqlite"));
-  do_check_false(msc.connectionReady);
-
-  var backup = msc.backupDB(BACKUP_FILE_NAME);
-  do_check_eq(BACKUP_FILE_NAME, backup.leafName);
-  do_check_true(backup.exists());
-  
-  backup.remove(false);
-}
-
-var tests = [test_backup_bad_connection];
-
-function run_test()
-{
-  cleanup();
-
-  for (var i = 0; i < tests.length; i++)
-    tests[i]();
-    
-  cleanup();
-}
-

storage/test/unit/test_storage_connection.js

@@ -37,8 +37,6 @@
 
 // This file tests the functions of mozIStorageConnection
 
-const BACKUP_FILE_NAME = "test_storage.sqlite.backup";
-
 function test_connectionReady_open()
 {
   // there doesn't seem to be a way for the connection to not be ready (unless
@@ -184,44 +182,6 @@ function test_set_schemaVersion_negative()
   do_check_eq(version, msc.schemaVersion);
 }
 
-function test_backup_not_new_filename()
-{
-  var msc = getOpenedDatabase();
-  const fname = getTestDB().leafName;
-
-  var backup = msc.backupDB(fname);
-  do_check_neq(fname, backup.leafName);
-
-  backup.remove(false);
-}
-
-function test_backup_new_filename()
-{
-  var msc = getOpenedDatabase();
-
-  var backup = msc.backupDB(BACKUP_FILE_NAME);
-  do_check_eq(BACKUP_FILE_NAME, backup.leafName);
-  
-  backup.remove(false);
-}
-
-function test_backup_new_folder()
-{
-  var msc = getOpenedDatabase();
-  var parentDir = getTestDB().parent;
-  parentDir.append("test_storage_temp");
-  if (parentDir.exists())
-    parentDir.remove(true);
-  parentDir.create(Ci.nsIFile.DIRECTORY_TYPE, 0755);
-  do_check_true(parentDir.exists());
-
-  var backup = msc.backupDB(BACKUP_FILE_NAME, parentDir);
-  do_check_eq(BACKUP_FILE_NAME, backup.leafName);
-  do_check_true(parentDir.equals(backup.parent));
-
-  parentDir.remove(true);
-}
-
 function test_createTable(){
   var temp = getTestDB().parent;
   temp.append("test_db_table");
@@ -255,9 +215,6 @@ var tests = [
   test_set_schemaVersion,
   test_set_schemaVersion_same,
   test_set_schemaVersion_negative,
-  test_backup_not_new_filename,
-  test_backup_new_filename,
-  test_backup_new_folder,
   test_createTable,
 ];
 

storage/test/unit/test_storage_service.js

@@ -38,6 +38,8 @@
 // This file tests the functions of mozIStorageService except for
 // openUnsharedDatabase, which is tested by test_storage_service_unshared.js.
 
+const BACKUP_FILE_NAME = "test_storage.sqlite.backup";
+
 function test_openSpecialDatabase_invalid_arg()
 {
   try {
@@ -68,8 +70,61 @@ function test_openDatabase_file_exists()
   do_check_true(db.exists());
 }
 
-var tests = [test_openSpecialDatabase_invalid_arg, test_openDatabase_file_DNE, 
-             test_openDatabase_file_exists];
+function test_corrupt_db_throws_with_openDatabase()
+{
+  try {
+    getDatabase(getCorruptDB());
+    do_throw("should not be here");
+  }
+  catch (e) {
+    do_check_eq(Cr.NS_ERROR_FILE_CORRUPTED, e.result);
+  }
+}
+
+function test_backup_not_new_filename()
+{
+  const fname = getTestDB().leafName;
+
+  var backup = getService().backupDatabaseFile(getTestDB(), fname);
+  do_check_neq(fname, backup.leafName);
+
+  backup.remove(false);
+}
+
+function test_backup_new_filename()
+{
+  var backup = getService().backupDatabaseFile(getTestDB(), BACKUP_FILE_NAME);
+  do_check_eq(BACKUP_FILE_NAME, backup.leafName);
+  
+  backup.remove(false);
+}
+
+function test_backup_new_folder()
+{
+  var parentDir = getTestDB().parent;
+  parentDir.append("test_storage_temp");
+  if (parentDir.exists())
+    parentDir.remove(true);
+  parentDir.create(Ci.nsIFile.DIRECTORY_TYPE, 0755);
+  do_check_true(parentDir.exists());
+
+  var backup = getService().backupDatabaseFile(getTestDB(), BACKUP_FILE_NAME,
+                                               parentDir);
+  do_check_eq(BACKUP_FILE_NAME, backup.leafName);
+  do_check_true(parentDir.equals(backup.parent));
+
+  parentDir.remove(true);
+}
+
+var tests = [
+  test_openSpecialDatabase_invalid_arg,
+  test_openDatabase_file_DNE, 
+  test_openDatabase_file_exists,
+  test_corrupt_db_throws_with_openDatabase,
+  test_backup_not_new_filename,
+  test_backup_new_filename,
+  test_backup_new_folder,
+];
 
 function run_test()
 {

toolkit/components/contentprefs/src/nsContentPrefService.js

@@ -695,12 +695,15 @@ ContentPrefService.prototype = {
     if (!dbFile.exists())
       dbConnection = this._dbCreate(dbService, dbFile);
     else {
-      dbConnection = dbService.openDatabase(dbFile);
-
+      try {
+        dbConnection = dbService.openDatabase(dbFile);
+      }
       // If the connection isn't ready after we open the database, that means
       // the database has been corrupted, so we back it up and then recreate it.
-      if (!dbConnection.connectionReady)
-        dbConnection = this._dbBackUpAndRecreate(dbService, dbFile, dbConnection);
+      catch (e if e.result == Cr.NS_ERROR_FILE_CORRUPTED) {
+        dbConnection = this._dbBackUpAndRecreate(dbService, dbFile,
+                                                 dbConnection);
+      }
 
       // Get the version of the schema in the file.
       var version = dbConnection.schemaVersion;
@@ -762,7 +765,7 @@ ContentPrefService.prototype = {
   _dbBackUpAndRecreate: function ContentPrefService__dbBackUpAndRecreate(aDBService,
                                                                          aDBFile,
                                                                          aDBConnection) {
-    aDBConnection.backupDB("content-prefs.sqlite.corrupt");
+    aDBService.backupDatabaseFile(aDBFile, "content-prefs.sqlite.corrupt");
 
     // Close the database, ignoring the "already closed" exception, if any.
     // It'll be open if we're here because of a migration failure but closed

toolkit/components/downloads/src/nsDownloadManager.cpp

@@ -256,22 +256,14 @@ nsDownloadManager::InitDB(PRBool *aDoImport)
   NS_ENSURE_SUCCESS(rv, rv);
 
   rv = storage->OpenDatabase(dbFile, getter_AddRefs(mDBConn));
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  PRBool ready;
-  (void)mDBConn->GetConnectionReady(&ready);
-  if (!ready) {
+  if (rv == NS_ERROR_FILE_CORRUPTED) {
     // delete and try again, since we don't care so much about losing a users
     // download history
     rv = dbFile->Remove(PR_FALSE);
     NS_ENSURE_SUCCESS(rv, rv);
     rv = storage->OpenDatabase(dbFile, getter_AddRefs(mDBConn));
-    NS_ENSURE_SUCCESS(rv, rv);
-
-    (void)mDBConn->GetConnectionReady(&ready);
-    if (!ready)
-      return NS_ERROR_UNEXPECTED;
   }
+  NS_ENSURE_SUCCESS(rv, rv);
 
   PRBool tableExists;
   rv = mDBConn->TableExists(NS_LITERAL_CSTRING("moz_downloads"), &tableExists);
@@ -497,8 +489,8 @@ nsDownloadManager::InitDB(PRBool *aDoImport)
       // if the statement fails, that means all the columns were not there.
       // First we backup the database
       nsCOMPtr<nsIFile> backup;
-      rv = mDBConn->BackupDB(DM_DB_CORRUPT_FILENAME, nsnull,
-                             getter_AddRefs(backup));
+      rv = storage->BackupDatabaseFile(dbFile, DM_DB_CORRUPT_FILENAME, nsnull,
+                                       getter_AddRefs(backup));
       NS_ENSURE_SUCCESS(rv, rv);
 
       // Then we dump it

toolkit/components/places/src/nsNavHistory.cpp

@@ -521,6 +521,8 @@ nsNavHistory::InitDBFile(PRBool aForceInit)
   if (aForceInit) {
     NS_ASSERTION(mDBConn,
                  "When forcing initialization, a database connection must exist!");
+    NS_ASSERTION(mDBService,
+                 "When forcing initialization, the database service must exist!");
   }
 
   // get profile dir, file
@@ -538,7 +540,8 @@ nsNavHistory::InitDBFile(PRBool aForceInit)
   if (aForceInit) {
     // backup the database
     nsCOMPtr<nsIFile> backup;
-    rv = mDBConn->BackupDB(DB_CORRUPT_FILENAME, profDir, getter_AddRefs(backup));
+    rv = mDBService->BackupDatabaseFile(mDBFile, DB_CORRUPT_FILENAME, profDir,
+                                        getter_AddRefs(backup));
     NS_ENSURE_SUCCESS(rv, rv);
 
     // close database connection if open
@@ -560,16 +563,13 @@ nsNavHistory::InitDBFile(PRBool aForceInit)
   mDBService = do_GetService(MOZ_STORAGE_SERVICE_CONTRACTID, &rv);
   NS_ENSURE_SUCCESS(rv, rv);
   rv = mDBService->OpenDatabase(mDBFile, getter_AddRefs(mDBConn));
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  PRBool ready;
-  (void)mDBConn->GetConnectionReady(&ready);
-  if (!ready) {
+  if (rv == NS_ERROR_FILE_CORRUPTED) {
     dbExists = PR_FALSE;
   
     // backup file
     nsCOMPtr<nsIFile> backup;
-    rv = mDBConn->BackupDB(DB_CORRUPT_FILENAME, profDir, getter_AddRefs(backup));
+    rv = mDBService->BackupDatabaseFile(mDBFile, DB_CORRUPT_FILENAME, profDir,
+                                        getter_AddRefs(backup));
     NS_ENSURE_SUCCESS(rv, rv);
  
     // remove existing file 
@@ -582,13 +582,8 @@ nsNavHistory::InitDBFile(PRBool aForceInit)
     rv = mDBFile->Append(DB_FILENAME);
     NS_ENSURE_SUCCESS(rv, rv);
     rv = mDBService->OpenDatabase(mDBFile, getter_AddRefs(mDBConn));
-    NS_ENSURE_SUCCESS(rv, rv);
-    (void)mDBConn->GetConnectionReady(&ready);
-    if (!ready) {
-      mDBConn = nsnull;
-      return NS_ERROR_UNEXPECTED;
-    }
   }
+  NS_ENSURE_SUCCESS(rv, rv);
   
   // if the db didn't previously exist, or was corrupted, re-import bookmarks.
   if (!dbExists) {

toolkit/components/url-classifier/src/nsUrlClassifierDBService.cpp

@@ -2993,11 +2993,7 @@ nsUrlClassifierDBServiceWorker::OpenDb()
 
   nsCOMPtr<mozIStorageConnection> connection;
   rv = storageService->OpenDatabase(mDBFile, getter_AddRefs(connection));
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  PRBool ready;
-  connection->GetConnectionReady(&ready);
-  if (!ready) {
+  if (rv == NS_ERROR_FILE_CORRUPTED) {
     // delete the db and try opening again
     rv = mDBFile->Remove(PR_FALSE);
     NS_ENSURE_SUCCESS(rv, rv);
@@ -3005,8 +3001,8 @@ nsUrlClassifierDBServiceWorker::OpenDb()
     newDB = PR_TRUE;
 
     rv = storageService->OpenDatabase(mDBFile, getter_AddRefs(connection));
-    NS_ENSURE_SUCCESS(rv, rv);
   }
+  NS_ENSURE_SUCCESS(rv, rv);
 
   if (!newDB) {
     PRInt32 databaseVersion;