Bug 1423820 - Copy temp entities after reattaching databases to a cloned storage connection. r?mak draft
authorKit Cambridge <kit@yakshaving.ninja>
Wed, 06 Dec 2017 22:34:18 -0800
changeset 709158 de639e97737784d52b4f197c7757c22bc6ebf83b
parent 708361 dff2f7200586a6b5967d0d7b15bd64be41abeb19
child 743344 8b645c011069dd31bdb0f50252e69776d19296e8
push id92557
push userbmo:kit@mozilla.com
push dateThu, 07 Dec 2017 17:45:35 +0000
reviewersmak
bugs1423820
milestone59.0a1
Bug 1423820 - Copy temp entities after reattaching databases to a cloned storage connection. r?mak MozReview-Commit-ID: illWRvUv3Y
storage/mozStorageConnection.cpp
storage/mozStorageConnection.h
storage/test/unit/test_storage_connection.js
--- a/storage/mozStorageConnection.cpp
+++ b/storage/mozStorageConnection.cpp
@@ -762,23 +762,17 @@ Connection::initialize(nsIFileURL *aFile
 }
 
 nsresult
 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) {
     const char* dbPath = ::sqlite3_db_filename(mDBConn, "main");
     MOZ_ASSERT(dbPath);
 
     const char* telemetryFilename =
       ::sqlite3_uri_parameter(dbPath, "telemetryFilename");
@@ -883,16 +877,28 @@ Connection::initializeOnAsyncThread(nsIF
     nsCOMPtr<nsIRunnable> event =
       NewRunnableMethod("Connection::shutdownAsyncThread",
                         this, &Connection::shutdownAsyncThread);
     Unused << NS_DispatchToMainThread(event);
   }
   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,
                                   bool *_exists)
 {
   if (!mDBConn) return NS_ERROR_NOT_INITIALIZED;
 
   // When constructing the query, make sure to SELECT the correct db's sqlite_master
@@ -1521,48 +1527,19 @@ nsresult
 Connection::initializeClone(Connection* aClone, bool aReadOnly)
 {
   nsresult rv = mFileURL ? aClone->initialize(mFileURL)
                          : aClone->initialize(mDatabaseFile);
   if (NS_FAILED(rv)) {
     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<mozIStorageStatement> 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.
   {
     nsCOMPtr<mozIStorageStatement> stmt;
     rv = CreateStatement(NS_LITERAL_CSTRING("PRAGMA database_list"),
                          getter_AddRefs(stmt));
     MOZ_ASSERT(NS_SUCCEEDED(rv));
     bool hasResult = false;
@@ -1618,16 +1595,55 @@ Connection::initializeClone(Connection* 
     if (stmt && NS_SUCCEEDED(stmt->ExecuteStep(&hasResult)) && hasResult) {
       pragmaQuery.AppendLiteral(" = ");
       pragmaQuery.AppendInt(stmt->AsInt32(0));
       rv = aClone->ExecuteSimpleSQL(pragmaQuery);
       MOZ_ASSERT(NS_SUCCEEDED(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) {
+    rv = aClone->ExecuteSimpleSQL(NS_LITERAL_CSTRING("BEGIN TRANSACTION"));
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    nsCOMPtr<mozIStorageStatement> 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()) {
     const nsACString &key = iter.Key();
     Connection::FunctionInfo data = iter.UserData();
 
     MOZ_ASSERT(data.type == Connection::FunctionInfo::SIMPLE ||
                data.type == Connection::FunctionInfo::AGGREGATE,
@@ -1646,16 +1662,17 @@ Connection::initializeClone(Connection* 
         static_cast<mozIStorageAggregateFunction *>(data.function.get());
       rv = aClone->CreateAggregateFunction(key, data.numArgs, function);
       if (NS_FAILED(rv)) {
         NS_WARNING("Failed to copy aggregate function to cloned connection");
       }
     }
   }
 
+  guard.release();
   return NS_OK;
 }
 
 NS_IMETHODIMP
 Connection::Clone(bool aReadOnly,
                   mozIStorageConnection **_connection)
 {
   MOZ_ASSERT(threadOpenedOn == NS_GetCurrentThread());
--- a/storage/mozStorageConnection.h
+++ b/storage/mozStorageConnection.h
@@ -272,16 +272,17 @@ public:
    */
   bool isAsyncExecutionThreadAvailable();
 
   nsresult initializeClone(Connection *aClone, bool aReadOnly);
 
 private:
   ~Connection();
   nsresult initializeInternal();
+  void initializeFailed();
 
   /**
    * Sets the database into a closed state so no further actions can be
    * performed.
    *
    * @note mDBConn is set to nullptr in this method.
    */
   nsresult setClosedState();
--- a/storage/test/unit/test_storage_connection.js
+++ b/storage/test/unit/test_storage_connection.js
@@ -1,14 +1,24 @@
 /* This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 // 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() {
   // there doesn't seem to be a way for the connection to not be ready (unless
   // we close it with mozIStorageConnection::Close(), but we don't for this).
   // It can only fail if GetPath fails on the database file, or if we run out
   // of memory trying to use an in-memory database
 
@@ -711,36 +721,51 @@ add_task(async function test_clone_attac
   let db1 = Services.storage.openUnsharedDatabase(getTestDB());
 
   let c = 0;
   function attachDB(conn, name) {
     let file = Services.dirsvc.get("ProfD", Ci.nsIFile);
     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();
   do_check_true(db2.connectionReady);
 
   // These should not throw.
   stmt = db2.createStatement("SELECT * FROM attached_1.sqlite_master");
   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);
   do_check_true(db3.connectionReady);
 
   // These should not throw.
   stmt = db3.createStatement("SELECT * FROM attached_1.sqlite_master");
   stmt.finalize();
@@ -784,22 +809,17 @@ add_task(async function test_async_clone
 
   do_print("Fire temp trigger on read-write clone");
   let deleteStmt = readWriteClone.createAsyncStatement(`
     DELETE FROM test_temp`);
   await executeAsync(deleteStmt);
   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"));
 
   do_print("Create read-only clone");
   let readOnlyClone = await asyncClone(db, true);
   do_check_true(readOnlyClone instanceof Ci.mozIStorageAsyncConnection);