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

@@ -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<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.
   {
@@ -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<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()) {
@@ -1651,6 +1667,7 @@ Connection::initializeClone(Connection* aClone, bool aReadOnly)
     }
   }
 
+  guard.release();
   return NS_OK;
 }
 

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

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"));