From f893e12aead4e7be7187077ff0efc5ffb659503c Mon Sep 17 00:00:00 2001 From: Mike Conley Date: Mon, 22 Jan 2024 14:26:52 +0000 Subject: [PATCH] Bug 1869060 - Move old backupDatabaseFile utility out from mozIStorageService to a Places helper. r=mak,places-reviewers This utility copies an SQLite database, but does so just by performing a file copy on the database itself. It assumes that there are no open connections on the underlying database file. Given that this is only used by Places, and that a later patch in this stack adds a database backup utility that works even if there are open connections, means we can move this old utility out form the mozIStorageService and into a dedicated Places helper instead. Differential Revision: https://phabricator.services.mozilla.com/D198309 --- storage/mozIStorageService.idl | 21 ----------- storage/mozStorageService.cpp | 34 ------------------ storage/test/unit/test_storage_service.js | 44 ----------------------- toolkit/components/places/Database.cpp | 4 +-- toolkit/components/places/Helpers.cpp | 29 +++++++++++++++ toolkit/components/places/Helpers.h | 22 ++++++++++++ 6 files changed, 53 insertions(+), 101 deletions(-) diff --git a/storage/mozIStorageService.idl b/storage/mozIStorageService.idl index b5655eba173c..22c1dc056540 100644 --- a/storage/mozIStorageService.idl +++ b/storage/mozIStorageService.idl @@ -281,27 +281,6 @@ interface mozIStorageService : nsISupports { mozIStorageConnection openDatabaseWithFileURL( in nsIFileURL aFileURL, [optional] in ACString aTelemetryFilename, [optional] in unsigned long aConnectionFlags); - - /* - * 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++ diff --git a/storage/mozStorageService.cpp b/storage/mozStorageService.cpp index ac5a7b17888c..78aebb5d8dca 100644 --- a/storage/mozStorageService.cpp +++ b/storage/mozStorageService.cpp @@ -685,40 +685,6 @@ Service::OpenDatabaseWithFileURL(nsIFileURL* aFileURL, return NS_OK; } -NS_IMETHODIMP -Service::BackupDatabaseFile(nsIFile* aDBFile, const nsAString& aBackupFileName, - nsIFile* aBackupParentDirectory, nsIFile** backup) { - nsresult rv; - nsCOMPtr 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 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(false); - NS_ENSURE_SUCCESS(rv, rv); - - backupDB.forget(backup); - - return aDBFile->CopyTo(parentDir, fileName); -} - //////////////////////////////////////////////////////////////////////////////// //// nsIObserver diff --git a/storage/test/unit/test_storage_service.js b/storage/test/unit/test_storage_service.js index 68fcb15423e6..62a933cfbfe8 100644 --- a/storage/test/unit/test_storage_service.js +++ b/storage/test/unit/test_storage_service.js @@ -6,8 +6,6 @@ // openSpecialDatabase, which is tested by test_storage_service_special.js and // openUnsharedDatabase, which is tested by test_storage_service_unshared.js. -const BACKUP_FILE_NAME = "test_storage.sqlite.backup"; - function test_openDatabase_null_file() { try { Services.storage.openDatabase(null); @@ -95,45 +93,6 @@ function test_fake_db_throws_with_openDatabase() { ); } -function test_backup_not_new_filename() { - const fname = getTestDB().leafName; - - var backup = Services.storage.backupDatabaseFile(getTestDB(), fname); - Assert.notEqual(fname, backup.leafName); - - backup.remove(false); -} - -function test_backup_new_filename() { - var backup = Services.storage.backupDatabaseFile( - getTestDB(), - BACKUP_FILE_NAME - ); - Assert.equal(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, 0o755); - Assert.ok(parentDir.exists()); - - var backup = Services.storage.backupDatabaseFile( - getTestDB(), - BACKUP_FILE_NAME, - parentDir - ); - Assert.equal(BACKUP_FILE_NAME, backup.leafName); - Assert.ok(parentDir.equals(backup.parent)); - - parentDir.remove(true); -} - function test_openDatabase_directory() { let dir = getTestDB().parent; dir.append("test_storage_temp"); @@ -286,9 +245,6 @@ var tests = [ test_openDatabase_file_exists, test_corrupt_db_throws_with_openDatabase, test_fake_db_throws_with_openDatabase, - test_backup_not_new_filename, - test_backup_new_filename, - test_backup_new_folder, test_openDatabase_directory, test_read_gooddb, test_read_baddb, diff --git a/toolkit/components/places/Database.cpp b/toolkit/components/places/Database.cpp index bd7e1cc2d908..891c53156dc1 100644 --- a/toolkit/components/places/Database.cpp +++ b/toolkit/components/places/Database.cpp @@ -797,8 +797,8 @@ nsresult Database::BackupAndReplaceDatabaseFile( } nsCOMPtr backup; - Unused << aStorage->BackupDatabaseFile(databaseFile, corruptFilename, - profDir, getter_AddRefs(backup)); + Unused << BackupDatabaseFile(databaseFile, corruptFilename, profDir, + getter_AddRefs(backup)); } // If anything fails from this point on, we have a stale connection or diff --git a/toolkit/components/places/Helpers.cpp b/toolkit/components/places/Helpers.cpp index 090743df06fb..90ab263ec161 100644 --- a/toolkit/components/places/Helpers.cpp +++ b/toolkit/components/places/Helpers.cpp @@ -6,6 +6,7 @@ #include "Helpers.h" #include "mozIStorageError.h" #include "prio.h" +#include "nsIFile.h" #include "nsReadableUtils.h" #include "nsString.h" #include "nsNavHistory.h" @@ -349,5 +350,33 @@ void TokensToQueryString(const nsTArray& aTokens, }); } +nsresult BackupDatabaseFile(nsIFile* aDBFile, const nsAString& aBackupFileName, + nsIFile* aBackupParentDirectory, nsIFile** backup) { + nsresult rv; + nsCOMPtr 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 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(false); + NS_ENSURE_SUCCESS(rv, rv); + + backupDB.forget(backup); + return aDBFile->CopyTo(parentDir, fileName); +} + } // namespace places } // namespace mozilla diff --git a/toolkit/components/places/Helpers.h b/toolkit/components/places/Helpers.h index fe7ed82b91aa..6719fdded29b 100644 --- a/toolkit/components/places/Helpers.h +++ b/toolkit/components/places/Helpers.h @@ -18,6 +18,8 @@ #include "mozilla/Telemetry.h" #include "mozIStorageStatementCallback.h" +class nsIFile; + namespace mozilla { namespace places { @@ -209,6 +211,26 @@ nsresult TokenizeQueryString(const nsACString& aQuery, void TokensToQueryString(const nsTArray& aTokens, nsACString& aQuery); +/** + * 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. This utility is meant + * to be used on database files with no open connections. Using this on database + * files with open connections may result in a corrupt backup file. + * + * @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. + * @param backup + * An outparam for the nsIFile pointing to the backup copy. + */ +nsresult BackupDatabaseFile(nsIFile* aDBFile, const nsAString& aBackupFileName, + nsIFile* aBackupParentDirectory, nsIFile** backup); + /** * Used to finalize a statementCache on a specified thread. */