Bug 1613835 - Use a SQLite API call to check for in-progress transactions. r=mak

Previously, `mozIStorageConnection#transactionInProgress` returned true
only if a transaction was started via `beginTransaction()`. This meant
that manually executing `BEGIN`, as `Sqlite.jsm` and the Rust bindings
do, wouldn't accurately report if a transaction was in progress.
Similarly, the flag wasn't accurate in cases where SQLite automatically
rolled back a transaction.

Fortunately, SQLite provides the `sqlite3_get_autocommit()` function,
which we can use to determine if a transaction is open or not. This
commit refactors the `transactionInProgress` getter, along with all
`Connection` methods that depend on it, to use the SQLite API instead
of managing that state on the connection. `mozStorageTransaction` and
`Sqlite.jsm` still use their own flags to decide whether to commit
their transactions, for reasons explained in the IDL comment.

This commit also moves `transactionInProgress` to
`mozIStorageAsyncConnection`, so that `Sqlite.jsm` can use it, and
exposes it to Rust.

Differential Revision: https://phabricator.services.mozilla.com/D63732

--HG--
extra : moz-landing-system : lando

storage/mozIStorageAsyncConnection.idl

@@ -74,6 +74,30 @@ interface mozIStorageAsyncConnection : nsISupports {
    */
   readonly attribute int32_t variableLimit;
 
+  /**
+   * Returns true if a transaction is active on this connection.
+   *
+   * Note that this is true if a transaction is active on the connection,
+   * regardless of how it was opened. There are several ways to open one:
+   *
+   * 1. Explicitly calling `beginTransaction` on a `mozIStorageConnection`.
+   * 2. Calling `executeSimpleSQL("BEGIN")` or
+   *    `createStatement("BEGIN").execute()` on a `mozIStorageConnection`.
+   * 3. Executing an async statement, like
+   *    `createAsyncStatement("BEGIN").executeAsync(...)`. This is what
+   *    `Sqlite.jsm` does under the hood.
+   *
+   * Because of this, it's important *not* to use this attribute to decide
+   * whether to *commit* the active transaction, because the caller that opened
+   * it may not expect that. This is why both `mozStorageTransaction` and
+   * `Sqlite.jsm` use an internal variable (`mHasTransaction` for the former;
+   * `_hasInProgressTransaction` for the latter) to check if their transaction
+   * is already in progress, instead of just checking this attribute before
+   * committing. Otherwise, mozStorage might accidentally commit (or roll back!)
+   * a transaction started by `Sqlite.jsm`, and vice versa.
+   */
+  readonly attribute boolean transactionInProgress;
+
   /**
    * Close this database connection, allowing all pending statements
    * to complete first.

storage/mozIStorageConnection.idl

@@ -174,11 +174,6 @@ interface mozIStorageConnection : mozIStorageAsyncConnection {
   //////////////////////////////////////////////////////////////////////////////
   //// Transactions
 
-  /**
-   * Returns true if a transaction is active on this connection.
-   */
-  readonly attribute boolean transactionInProgress;
-
   /**
    * Begin a new transaction. If a transaction is active, throws an error.
    */

storage/mozStorageAsyncStatementExecution.cpp

@@ -319,16 +319,20 @@ nsresult AsyncExecuteStatements::notifyComplete() {
 
   // Handle our transaction, if we have one
   if (mHasTransaction) {
+    SQLiteMutexAutoLock lockedScope(mDBMutex);
     if (mState == COMPLETED) {
-      nsresult rv = mConnection->commitTransactionInternal(mNativeConnection);
+      nsresult rv = mConnection->commitTransactionInternal(lockedScope,
+                                                           mNativeConnection);
       if (NS_FAILED(rv)) {
         mState = ERROR;
+        // We cannot hold the DB mutex while calling notifyError.
+        SQLiteMutexAutoUnlock unlockedScope(mDBMutex);
         (void)notifyError(mozIStorageError::ERROR,
                           "Transaction failed to commit");
       }
     } else {
-      DebugOnly<nsresult> rv =
-          mConnection->rollbackTransactionInternal(mNativeConnection);
+      DebugOnly<nsresult> rv = mConnection->rollbackTransactionInternal(
+          lockedScope, mNativeConnection);
       NS_WARNING_ASSERTION(NS_SUCCEEDED(rv), "Transaction failed to rollback");
     }
     mHasTransaction = false;
@@ -486,9 +490,12 @@ AsyncExecuteStatements::Run() {
   }
   if (mState == CANCELED) return notifyComplete();
 
-  if (statementsNeedTransaction() && mConnection->getAutocommit()) {
+  if (statementsNeedTransaction()) {
+    SQLiteMutexAutoLock lockedScope(mDBMutex);
+    if (!mConnection->transactionInProgress(lockedScope)) {
       if (NS_SUCCEEDED(mConnection->beginTransactionInternal(
-            mNativeConnection, mozIStorageConnection::TRANSACTION_IMMEDIATE))) {
+              lockedScope, mNativeConnection,
+              mozIStorageConnection::TRANSACTION_IMMEDIATE))) {
         mHasTransaction = true;
       }
 #ifdef DEBUG
@@ -497,6 +504,7 @@ AsyncExecuteStatements::Run() {
       }
 #endif
     }
+  }
 
   // Execute each statement, giving the callback results if it returns any.
   for (uint32_t i = 0; i < mStatements.Length(); i++) {

storage/mozStorageConnection.cpp

@@ -466,7 +466,6 @@ Connection::Connection(Service* aService, int aFlags,
       mAsyncExecutionThreadShuttingDown(false),
       mConnectionClosed(false),
       mDefaultTransactionType(mozIStorageConnection::TRANSACTION_DEFERRED),
-      mTransactionInProgress(false),
       mDestroying(false),
       mProgressHandler(nullptr),
       mFlags(aFlags),
@@ -1913,13 +1912,13 @@ Connection::GetTransactionInProgress(bool* _inProgress) {
   if (!connectionReady()) {
     return NS_ERROR_NOT_INITIALIZED;
   }
-  nsresult rv = ensureOperationSupported(SYNCHRONOUS);
+  nsresult rv = ensureOperationSupported(ASYNCHRONOUS);
   if (NS_FAILED(rv)) {
     return rv;
   }
 
   SQLiteMutexAutoLock lockedScope(sharedDBMutex);
-  *_inProgress = mTransactionInProgress;
+  *_inProgress = transactionInProgress(lockedScope);
   return NS_OK;
 }
 
@@ -1959,13 +1958,17 @@ Connection::BeginTransaction() {
     return rv;
   }
 
-  return beginTransactionInternal(mDBConn, mDefaultTransactionType);
+  SQLiteMutexAutoLock lockedScope(sharedDBMutex);
+  return beginTransactionInternal(lockedScope, mDBConn,
+                                  mDefaultTransactionType);
 }
 
-nsresult Connection::beginTransactionInternal(sqlite3* aNativeConnection,
+nsresult Connection::beginTransactionInternal(
+    const SQLiteMutexAutoLock& aProofOfLock, sqlite3* aNativeConnection,
     int32_t aTransactionType) {
-  SQLiteMutexAutoLock lockedScope(sharedDBMutex);
-  if (mTransactionInProgress) return NS_ERROR_FAILURE;
+  if (transactionInProgress(aProofOfLock)) {
+    return NS_ERROR_FAILURE;
+  }
   nsresult rv;
   switch (aTransactionType) {
     case TRANSACTION_DEFERRED:
@@ -1980,7 +1983,6 @@ nsresult Connection::beginTransactionInternal(sqlite3* aNativeConnection,
     default:
       return NS_ERROR_ILLEGAL_VALUE;
   }
-  if (NS_SUCCEEDED(rv)) mTransactionInProgress = true;
   return rv;
 }
 
@@ -1994,15 +1996,17 @@ Connection::CommitTransaction() {
     return rv;
   }
 
-  return commitTransactionInternal(mDBConn);
+  SQLiteMutexAutoLock lockedScope(sharedDBMutex);
+  return commitTransactionInternal(lockedScope, mDBConn);
 }
 
-nsresult Connection::commitTransactionInternal(sqlite3* aNativeConnection) {
-  SQLiteMutexAutoLock lockedScope(sharedDBMutex);
-  if (!mTransactionInProgress) return NS_ERROR_UNEXPECTED;
+nsresult Connection::commitTransactionInternal(
+    const SQLiteMutexAutoLock& aProofOfLock, sqlite3* aNativeConnection) {
+  if (!transactionInProgress(aProofOfLock)) {
+    return NS_ERROR_UNEXPECTED;
+  }
   nsresult rv =
       convertResultCode(executeSql(aNativeConnection, "COMMIT TRANSACTION"));
-  if (NS_SUCCEEDED(rv)) mTransactionInProgress = false;
   return rv;
 }
 
@@ -2016,16 +2020,18 @@ Connection::RollbackTransaction() {
     return rv;
   }
 
-  return rollbackTransactionInternal(mDBConn);
+  SQLiteMutexAutoLock lockedScope(sharedDBMutex);
+  return rollbackTransactionInternal(lockedScope, mDBConn);
 }
 
-nsresult Connection::rollbackTransactionInternal(sqlite3* aNativeConnection) {
-  SQLiteMutexAutoLock lockedScope(sharedDBMutex);
-  if (!mTransactionInProgress) return NS_ERROR_UNEXPECTED;
+nsresult Connection::rollbackTransactionInternal(
+    const SQLiteMutexAutoLock& aProofOfLock, sqlite3* aNativeConnection) {
+  if (!transactionInProgress(aProofOfLock)) {
+    return NS_ERROR_UNEXPECTED;
+  }
 
   nsresult rv =
       convertResultCode(executeSql(aNativeConnection, "ROLLBACK TRANSACTION"));
-  if (NS_SUCCEEDED(rv)) mTransactionInProgress = false;
   return rv;
 }
 

storage/mozStorageConnection.h

@@ -229,16 +229,31 @@ class Connection final : public mozIStorageConnection,
    * @see BeginTransactionAs, CommitTransaction, RollbackTransaction.
    */
   nsresult beginTransactionInternal(
-      sqlite3* aNativeConnection,
+      const SQLiteMutexAutoLock& aProofOfLock, sqlite3* aNativeConnection,
       int32_t aTransactionType = TRANSACTION_DEFERRED);
-  nsresult commitTransactionInternal(sqlite3* aNativeConnection);
-  nsresult rollbackTransactionInternal(sqlite3* aNativeConnection);
+  nsresult commitTransactionInternal(const SQLiteMutexAutoLock& aProofOfLock,
+                                     sqlite3* aNativeConnection);
+  nsresult rollbackTransactionInternal(const SQLiteMutexAutoLock& aProofOfLock,
+                                       sqlite3* aNativeConnection);
 
   /**
    * Indicates if this database connection is open.
    */
   inline bool connectionReady() { return mDBConn != nullptr; }
 
+  /**
+   * Indicates if this database connection has an open transaction. Because
+   * multiple threads can execute statements on the same connection, this method
+   * requires proof that the caller is holding `sharedDBMutex`.
+   *
+   * Per the SQLite docs, `sqlite3_get_autocommit` returns 0 if autocommit mode
+   * is disabled. `BEGIN` disables autocommit mode, and `COMMIT`, `ROLLBACK`, or
+   * an automatic rollback re-enables it.
+   */
+  inline bool transactionInProgress(const SQLiteMutexAutoLock& aProofOfLock) {
+    return !getAutocommit();
+  }
+
   /**
    * Indicates if this database connection supports the given operation.
    *
@@ -401,12 +416,6 @@ class Connection final : public mozIStorageConnection,
    */
   mozilla::Atomic<int32_t> mDefaultTransactionType;
 
-  /**
-   * Tracks if we have a transaction in progress or not.  Access protected by
-   * sharedDBMutex.
-   */
-  bool mTransactionInProgress;
-
   /**
    * Used to trigger cleanup logic only the first time our refcount hits 1.  We
    * may trigger a failsafe Close() that invokes SpinningSynchronousClose()

storage/rust/src/lib.rs

@@ -125,6 +125,21 @@ impl Conn {
         Transaction::new(self, behavior)
     }
 
+    /// Indicates if a transaction is currently open on this connection.
+    /// Attempting to open a new transaction when one is already in progress
+    /// will fail with a "cannot start a transaction within a transaction"
+    /// error.
+    ///
+    /// Note that this is `true` even if the transaction was started by another
+    /// caller, like `Sqlite.jsm` or `mozStorageTransaction` from C++. See the
+    /// explanation above `mozIStorageConnection.transactionInProgress` for why
+    /// this matters.
+    pub fn transaction_in_progress(&self) -> Result<bool> {
+        let mut in_progress = false;
+        unsafe { self.handle.GetTransactionInProgress(&mut in_progress) }.to_result()?;
+        Ok(in_progress)
+    }
+
     /// Opens a transaction with the requested behavior.
     pub fn transaction_with_behavior(
         &mut self,

toolkit/modules/Sqlite.jsm

@@ -595,7 +595,7 @@ ConnectionData.prototype = Object.freeze({
   },
 
   get transactionInProgress() {
-    return this._open && this._hasInProgressTransaction;
+    return this._open && this._dbConn.transactionInProgress;
   },
 
   executeTransaction(func, type) {
@@ -1544,6 +1544,11 @@ OpenedConnection.prototype = Object.freeze({
 
   /**
    * Whether a transaction is currently in progress.
+   *
+   * Note that this is true if a transaction is active on the connection,
+   * regardless of whether it was started by `Sqlite.jsm` or another consumer.
+   * See the explanation above `mozIStorageConnection.transactionInProgress` for
+   * why this distinction matters.
    */
   get transactionInProgress() {
     return this._connectionData.transactionInProgress;