From f4136db758744f9a1e8411d42dc1d58b7903f677 Mon Sep 17 00:00:00 2001 From: Kit Cambridge Date: Wed, 6 Dec 2017 22:34:18 -0800 Subject: [PATCH] Bug 1423820 - Copy temp entities after reattaching databases to a cloned storage connection. r=mak MozReview-Commit-ID: illWRvUv3Y --HG-- extra : rebase_source : eb4538fd4dd0ca2429c341864b8b1b1f761612c6 --- storage/mozStorageConnection.cpp | 95 ++++++++++++-------- storage/mozStorageConnection.h | 1 + storage/test/unit/test_storage_connection.js | 32 +++++-- 3 files changed, 83 insertions(+), 45 deletions(-) diff --git a/storage/mozStorageConnection.cpp b/storage/mozStorageConnection.cpp index dd415c652ebd..08e9727676ba 100644 --- a/storage/mozStorageConnection.cpp +++ b/storage/mozStorageConnection.cpp @@ -767,13 +767,7 @@ Connection::initializeInternal() MOZ_ASSERT(mDBConn); auto guard = MakeScopeExit([&]() { - { - MutexAutoLock lockedScope(sharedAsyncExecutionMutex); - mConnectionClosed = true; - } - MOZ_ALWAYS_TRUE(::sqlite3_close(mDBConn) == SQLITE_OK); - mDBConn = nullptr; - sharedDBMutex.destroy(); + initializeFailed(); }); if (mFileURL) { @@ -888,6 +882,18 @@ Connection::initializeOnAsyncThread(nsIFile* aStorageFile) { return rv; } +void +Connection::initializeFailed() +{ + { + MutexAutoLock lockedScope(sharedAsyncExecutionMutex); + mConnectionClosed = true; + } + MOZ_ALWAYS_TRUE(::sqlite3_close(mDBConn) == SQLITE_OK); + mDBConn = nullptr; + sharedDBMutex.destroy(); +} + nsresult Connection::databaseElementExists(enum DatabaseElementType aElementType, const nsACString &aElementName, @@ -1526,38 +1532,9 @@ Connection::initializeClone(Connection* aClone, bool aReadOnly) return rv; } - // Copy over temporary tables, triggers, and views from the original - // connections. Entities in `sqlite_temp_master` are only visible to the - // connection that created them. - if (!aReadOnly) { - nsCOMPtr stmt; - rv = CreateStatement(NS_LITERAL_CSTRING("SELECT sql FROM sqlite_temp_master " - "WHERE type IN ('table', 'view', " - "'index', 'trigger')"), - getter_AddRefs(stmt)); - // Propagate errors, because failing to copy triggers might cause schema - // coherency issues when writing to the database from the cloned connection. - NS_ENSURE_SUCCESS(rv, rv); - bool hasResult = false; - while (stmt && NS_SUCCEEDED(stmt->ExecuteStep(&hasResult)) && hasResult) { - nsAutoCString query; - rv = stmt->GetUTF8String(0, query); - NS_ENSURE_SUCCESS(rv, rv); - - // The `CREATE` SQL statements in `sqlite_temp_master` omit the `TEMP` - // keyword. We need to add it back, or we'll recreate temporary entities - // as persistent ones. `sqlite_temp_master` also holds `CREATE INDEX` - // statements, but those don't need `TEMP` keywords. - if (StringBeginsWith(query, NS_LITERAL_CSTRING("CREATE TABLE ")) || - StringBeginsWith(query, NS_LITERAL_CSTRING("CREATE TRIGGER ")) || - StringBeginsWith(query, NS_LITERAL_CSTRING("CREATE VIEW "))) { - query.Replace(0, 6, "CREATE TEMP"); - } - - rv = aClone->ExecuteSimpleSQL(query); - NS_ENSURE_SUCCESS(rv, rv); - } - } + auto guard = MakeScopeExit([&]() { + aClone->initializeFailed(); + }); // Re-attach on-disk databases that were attached to the original connection. { @@ -1623,6 +1600,45 @@ Connection::initializeClone(Connection* aClone, bool aReadOnly) } } + // Copy over temporary tables, triggers, and views from the original + // connections. Entities in `sqlite_temp_master` are only visible to the + // connection that created them. + if (!aReadOnly) { + rv = aClone->ExecuteSimpleSQL(NS_LITERAL_CSTRING("BEGIN TRANSACTION")); + NS_ENSURE_SUCCESS(rv, rv); + + nsCOMPtr stmt; + rv = CreateStatement(NS_LITERAL_CSTRING("SELECT sql FROM sqlite_temp_master " + "WHERE type IN ('table', 'view', " + "'index', 'trigger')"), + getter_AddRefs(stmt)); + // Propagate errors, because failing to copy triggers might cause schema + // coherency issues when writing to the database from the cloned connection. + NS_ENSURE_SUCCESS(rv, rv); + bool hasResult = false; + while (stmt && NS_SUCCEEDED(stmt->ExecuteStep(&hasResult)) && hasResult) { + nsAutoCString query; + rv = stmt->GetUTF8String(0, query); + NS_ENSURE_SUCCESS(rv, rv); + + // The `CREATE` SQL statements in `sqlite_temp_master` omit the `TEMP` + // keyword. We need to add it back, or we'll recreate temporary entities + // as persistent ones. `sqlite_temp_master` also holds `CREATE INDEX` + // statements, but those don't need `TEMP` keywords. + if (StringBeginsWith(query, NS_LITERAL_CSTRING("CREATE TABLE ")) || + StringBeginsWith(query, NS_LITERAL_CSTRING("CREATE TRIGGER ")) || + StringBeginsWith(query, NS_LITERAL_CSTRING("CREATE VIEW "))) { + query.Replace(0, 6, "CREATE TEMP"); + } + + rv = aClone->ExecuteSimpleSQL(query); + NS_ENSURE_SUCCESS(rv, rv); + } + + rv = aClone->ExecuteSimpleSQL(NS_LITERAL_CSTRING("COMMIT")); + NS_ENSURE_SUCCESS(rv, rv); + } + // Copy any functions that have been added to this connection. SQLiteMutexAutoLock lockedScope(sharedDBMutex); for (auto iter = mFunctions.Iter(); !iter.Done(); iter.Next()) { @@ -1651,6 +1667,7 @@ Connection::initializeClone(Connection* aClone, bool aReadOnly) } } + guard.release(); return NS_OK; } diff --git a/storage/mozStorageConnection.h b/storage/mozStorageConnection.h index 0adb34e19ec0..ed1b4c7f8876 100644 --- a/storage/mozStorageConnection.h +++ b/storage/mozStorageConnection.h @@ -277,6 +277,7 @@ public: private: ~Connection(); nsresult initializeInternal(); + void initializeFailed(); /** * Sets the database into a closed state so no further actions can be diff --git a/storage/test/unit/test_storage_connection.js b/storage/test/unit/test_storage_connection.js index 81644e7bd2f1..c9371049b08e 100644 --- a/storage/test/unit/test_storage_connection.js +++ b/storage/test/unit/test_storage_connection.js @@ -4,6 +4,16 @@ // This file tests the functions of mozIStorageConnection +function fetchAllNames(conn) { + let names = []; + let stmt = conn.createStatement(`SELECT name FROM test ORDER BY name`); + while (stmt.executeStep()) { + names.push(stmt.getUTF8String(0)); + } + stmt.finalize(); + return names; +} + // Test Functions add_task(async function test_connectionReady_open() { @@ -716,16 +726,26 @@ add_task(async function test_clone_attach_database() { file.append("test_storage_" + (++c) + ".sqlite"); let db = Services.storage.openUnsharedDatabase(file); conn.executeSimpleSQL(`ATTACH DATABASE '${db.databaseFile.path}' AS ${name}`); + db.executeSimpleSQL(`CREATE TABLE test_${name}(name TEXT);`); db.close(); } attachDB(db1, "attached_1"); attachDB(db1, "attached_2"); + db1.executeSimpleSQL(` + CREATE TEMP TRIGGER test_temp_afterinsert_trigger + AFTER DELETE ON test_attached_1 FOR EACH ROW + BEGIN + INSERT INTO test(name) VALUES(OLD.name); + END`); // These should not throw. let stmt = db1.createStatement("SELECT * FROM attached_1.sqlite_master"); stmt.finalize(); stmt = db1.createStatement("SELECT * FROM attached_2.sqlite_master"); stmt.finalize(); + db1.executeSimpleSQL("INSERT INTO test_attached_1(name) VALUES('asuth')"); + db1.executeSimpleSQL("DELETE FROM test_attached_1"); + do_check_true(fetchAllNames(db1).includes("asuth")); // R/W clone. let db2 = db1.clone(); @@ -736,6 +756,11 @@ add_task(async function test_clone_attach_database() { stmt.finalize(); stmt = db2.createStatement("SELECT * FROM attached_2.sqlite_master"); stmt.finalize(); + db2.executeSimpleSQL("INSERT INTO test_attached_1(name) VALUES('past')"); + db2.executeSimpleSQL("DELETE FROM test_attached_1"); + let newNames = fetchAllNames(db2); + do_check_true(newNames.includes("past")); + do_check_matches(fetchAllNames(db1), newNames); // R/O clone. let db3 = db1.clone(true); @@ -789,12 +814,7 @@ add_task(async function test_async_clone_with_temp_trigger_and_table() { deleteStmt.finalize(); do_print("Read from original connection"); - let names = []; - let readStmt = db.createStatement(`SELECT name FROM test`); - while (readStmt.executeStep()) { - names.push(readStmt.getUTF8String(0)); - } - readStmt.finalize(); + let names = fetchAllNames(db); do_check_true(names.includes("mak")); do_check_true(names.includes("standard8")); do_check_true(names.includes("markh"));