Making sure that all in-progress transactions are rolled back on
authordumi@chromium.org <dumi@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 1 Feb 2010 23:32:44 +0000 (23:32 +0000)
committerdumi@chromium.org <dumi@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 1 Feb 2010 23:32:44 +0000 (23:32 +0000)
the database thread before they're destroyed. Otherwise,
SQLiteTransaction's destructor will try to do a rollback and that
would cause an assertion failure, if the object is not destroyed
on the DB thread.

Reviewed by Eric Seidel.

https://bugs.webkit.org/show_bug.cgi?id=34152

* platform/sql/SQLiteTransaction.cpp:
(WebCore::SQLiteTransaction::stop):
* storage/SQLTransaction.cpp:
(WebCore::SQLTransaction::notifyDatabaseThreadIsShuttingDown):
* storage/SQLTransaction.h:
* storage/SQLTransactionCoordinator.cpp:
(WebCore::SQLTransactionCoordinator::shutdown):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@54162 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebCore/ChangeLog
WebCore/platform/sql/SQLiteTransaction.cpp
WebCore/storage/SQLTransaction.cpp
WebCore/storage/SQLTransaction.h
WebCore/storage/SQLTransactionCoordinator.cpp

index fab3c220a579b578dcedfdd850dcca2287bc078d..9045dba4659cd8ee0044a21b0a55f971ac9bb3a4 100644 (file)
@@ -1,3 +1,23 @@
+2010-02-01  Dumitru Daniliuc  <dumi@chromium.org>
+
+        Reviewed by Eric Seidel.
+
+        Making sure that all in-progress transactions are rolled back on
+        the database thread before they're destroyed. Otherwise,
+        SQLiteTransaction's destructor will try to do a rollback and that
+        would cause an assertion failure, if the object is not destroyed
+        on the DB thread.
+
+        https://bugs.webkit.org/show_bug.cgi?id=34152
+
+        * platform/sql/SQLiteTransaction.cpp:
+        (WebCore::SQLiteTransaction::stop):
+        * storage/SQLTransaction.cpp:
+        (WebCore::SQLTransaction::notifyDatabaseThreadIsShuttingDown):
+        * storage/SQLTransaction.h:
+        * storage/SQLTransactionCoordinator.cpp:
+        (WebCore::SQLTransactionCoordinator::shutdown):
+
 2010-02-01  Sam Weinig  <sam@webkit.org>
 
         Reviewed by Beth Dakin.
index a4b2ac83a1a26c115746f41eeb26615694ed6197..a34613f69e99b980f2dbdaeb9ff00acd93af84d9 100644 (file)
@@ -64,6 +64,11 @@ void SQLiteTransaction::begin()
 
 void SQLiteTransaction::commit()
 {
+    // FIXME: this code is buggy; it assumes that COMMIT always succeeds which is not the case:
+    // the transaction could've been silently rolled back before getting to the COMMIT statement
+    // (https://bugs.webkit.org/show_bug.cgi?id=34280). However, the rest of the code does not
+    // know how to deal with a premature rollback and a failed COMMIT at this moment, so until
+    // we figure out what to do with bug 34280, it's better to leave this code as it is.
     if (m_inProgress) {
         ASSERT(m_db.m_transactionInProgress);
         m_db.executeCommand("COMMIT;");
@@ -84,8 +89,10 @@ void SQLiteTransaction::rollback()
 
 void SQLiteTransaction::stop()
 {
-    m_inProgress = false;
-    m_db.m_transactionInProgress = false;
+    if (m_inProgress) {
+        m_inProgress = false;
+        m_db.m_transactionInProgress = false;
+    }
 }
 
 } // namespace WebCore
index de615ca24eef04cc69bb93b27505cb39e95f5177..db25e1a734b2b14f369ed02f80b3ea965d00e675 100644 (file)
@@ -35,6 +35,7 @@
 #include "Database.h"
 #include "DatabaseAuthorizer.h"
 #include "DatabaseDetails.h"
+#include "DatabaseThread.h"
 #include "ExceptionCode.h"
 #include "Logging.h"
 #include "Page.h"
@@ -83,6 +84,7 @@ SQLTransaction::SQLTransaction(Database* db, PassRefPtr<SQLTransactionCallback>
 
 SQLTransaction::~SQLTransaction()
 {
+    ASSERT(!m_sqliteTransaction);
 }
 
 void SQLTransaction::executeSQL(const String& sqlStatement, const Vector<SQLValue>& arguments, PassRefPtr<SQLStatementCallback> callback, PassRefPtr<SQLStatementErrorCallback> callbackError, ExceptionCode& e)
@@ -203,6 +205,16 @@ void SQLTransaction::performPendingCallback()
         (this->*m_nextStep)();
 }
 
+void SQLTransaction::notifyDatabaseThreadIsShuttingDown()
+{
+    ASSERT(currentThread() == database()->scriptExecutionContext()->databaseThread()->getThreadID());
+
+    // If the transaction is in progress, we should roll it back here, since this is our last
+    // oportunity to do something related to this transaction on the DB thread.
+    // Clearing m_sqliteTransaction invokes SQLiteTransaction's destructor which does just that.
+    m_sqliteTransaction.clear();
+}
+
 void SQLTransaction::acquireLock()
 {
     m_database->transactionCoordinator()->acquireLock(this);
@@ -491,6 +503,7 @@ void SQLTransaction::cleanupAfterSuccessCallback()
     // There is no next step
     LOG(StorageAPI, "Transaction %p is complete\n", this);
     ASSERT(!m_database->m_sqliteDatabase.transactionInProgress());
+    m_sqliteTransaction.clear();
     m_nextStep = 0;
 
     // Release the lock on this database
index 6d6a8d7b16e5063abd4fb81d93ace222da5a88c3..1b02d013be763043758bb1017fda352b28e7ca7a 100644 (file)
@@ -80,6 +80,7 @@ public:
 
     Database* database() { return m_database.get(); }
     bool isReadOnly() { return m_readOnly; }
+    void notifyDatabaseThreadIsShuttingDown();
 
 private:
     SQLTransaction(Database*, PassRefPtr<SQLTransactionCallback>, PassRefPtr<SQLTransactionErrorCallback>,
index efdcd1de90c9cc3079a03554a0993e0041e99871..0fe5bda9c305983ea6271aa997774bbf293097d7 100644 (file)
@@ -109,6 +109,20 @@ void SQLTransactionCoordinator::releaseLock(SQLTransaction* transaction)
 
 void SQLTransactionCoordinator::shutdown()
 {
+    // Notify all transactions in progress that the database thread is shutting down
+    for (CoordinationInfoMap::iterator coordinationInfoIterator = m_coordinationInfoMap.begin();
+         coordinationInfoIterator != m_coordinationInfoMap.end(); ++coordinationInfoIterator) {
+        CoordinationInfo& info = coordinationInfoIterator->second;
+        if (info.activeWriteTransaction)
+            info.activeWriteTransaction->notifyDatabaseThreadIsShuttingDown();
+        for (HashSet<RefPtr<SQLTransaction> >::iterator activeReadTransactionsIterator =
+                     info.activeReadTransactions.begin();
+             activeReadTransactionsIterator != info.activeReadTransactions.end();
+             ++activeReadTransactionsIterator) {
+            (*activeReadTransactionsIterator)->notifyDatabaseThreadIsShuttingDown();
+        }
+    }
+
     // Clean up all pending transactions for all databases
     m_coordinationInfoMap.clear();
 }