WebCore: 1. Fix a bug in SQLiteTransaction: do not assume that COMMIT always
authordumi@chromium.org <dumi@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 5 Feb 2010 02:09:21 +0000 (02:09 +0000)
committerdumi@chromium.org <dumi@chromium.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 5 Feb 2010 02:09:21 +0000 (02:09 +0000)
succeeds.
2. Jump straight to the transaction error callback when a
statement fails in a way that makes sqlite automatically rollback
the transaction.
3. Fix the code that handles the "quota reached" failure, as it is
one of the failures that lead to an automatic transaction
rollback.

Reviewed by Eric Seidel.

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

* platform/sql/SQLiteDatabase.cpp:
(WebCore::SQLiteDatabase::isAutoCommitOn):
* platform/sql/SQLiteDatabase.h:
* platform/sql/SQLiteTransaction.cpp:
(WebCore::SQLiteTransaction::begin):
(WebCore::SQLiteTransaction::commit):
(WebCore::SQLiteTransaction::rollback):
(WebCore::SQLiteTransaction::transactionWasRolledBackBySqlite):
* platform/sql/SQLiteTransaction.h:
* storage/SQLTransaction.cpp:
(WebCore::SQLTransaction::SQLTransaction):
(WebCore::SQLTransaction::runStatements):
(WebCore::SQLTransaction::runCurrentStatement):
(WebCore::SQLTransaction::handleCurrentStatementError):
(WebCore::SQLTransaction::deliverQuotaIncreaseCallback):

LayoutTests: 1. Enhance quota-tracking.html: if sqlite automatically rolls back
a transaction because of a statement failure, make sure the rest
of the statements in the transaction are not executed.
2. Fix the expectations for quota-tracking.html. Sqlite cannot
recover from reaching a DB's max size.

Reviewed by Eric Seidel.

* storage/quota-tracking-expected.txt:
* storage/quota-tracking.html:

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

LayoutTests/ChangeLog
LayoutTests/storage/quota-tracking-expected.txt
LayoutTests/storage/quota-tracking.html
WebCore/ChangeLog
WebCore/platform/sql/SQLiteDatabase.cpp
WebCore/platform/sql/SQLiteDatabase.h
WebCore/platform/sql/SQLiteTransaction.cpp
WebCore/platform/sql/SQLiteTransaction.h
WebCore/storage/SQLTransaction.cpp

index fb6bfe0ac1a54a1fe421ff83cc3de0b7e4560a47..fc6b529a2c5325d0b5241311ccb15fd291e91b96 100644 (file)
@@ -1,3 +1,16 @@
+2010-02-04  Dumitru Daniliuc  <dumi@chromium.org>
+
+        Reviewed by Eric Seidel.
+
+        1. Enhance quota-tracking.html: if sqlite automatically rolls back
+        a transaction because of a statement failure, make sure the rest
+        of the statements in the transaction are not executed.
+        2. Fix the expectations for quota-tracking.html. Sqlite cannot
+        recover from reaching a DB's max size.
+
+        * storage/quota-tracking-expected.txt:
+        * storage/quota-tracking.html:
+
 2010-02-04  Csaba Osztrogon√°c  <ossy@webkit.org>
 
         Unreviewed typo fix for r54379.
index 1243889e9a501dc68295465bb2f709758acd35d7..62a2c6b2ef74687243c1d9f50b34eeb5317f919c 100644 (file)
@@ -1,11 +1,15 @@
 UI DELEGATE DATABASE CALLBACK: exceededDatabaseQuotaForSecurityOrigin:{file, , 0} database:QuotaManagementDatabase2
 This test checks to make sure that quotas are enforced per-origin instead of per-database, as they were prior to http://trac.webkit.org/projects/webkit/changeset/29983.
-The test clears all databases, sets the quota for the origin to 32k, then tries to insert 17k of data into two databases. If things go as planned, the UI Delegate will be informed of the exceeded quota.
+The test clears all databases, sets the quota for the origin to 32k, then tries to insert 17k of data into two databases. If things go as planned, the UI Delegate will be informed of the exceeded quota and should increase the quota for this origin. Inserting 17k of data the third time should succeed again.
 Adding a table
 Inserting some data
 Done adding data
 Adding a table
 Inserting some data
+Expected quota exception - there was not enough remaining storage space, or the storage quota was reached and the user declined to allow more space
+Done adding data
+Adding a table
+Inserting some data
 Done adding data
 Test Complete
 
index 7b47e7b895d4e5149855bae754313c1bda55063b..fb2cc9e022866ba740e85452401b841adf426ee2 100644 (file)
@@ -3,6 +3,7 @@
 <script>
 var database1;
 var database2;
+var database3;
 
 function log(message)
 {
@@ -16,19 +17,33 @@ function finishTest()
         layoutTestController.notifyDone();
 }
 
-function errorFunction(error)
+function statementErrorFunction(tx, error)
 {
-    log("Test failed - " + error.message);
+    log("Unexpected exception - " + error.message);
     finishTest();
 }
 
+function transactionErrorFunction(db, error)
+{
+    // We only expect an error message for database2
+    if (db == database2) {
+        log("Expected quota exception - " + error.message);
+        checkCompletion(db);
+    } else {
+        log("Unexpected exception - " + error.message);
+        finishTest();
+    }
+}
+
 function checkCompletion(db)
 {
     log("Done adding data");
 
     db.complete = true;
-    if (database1.complete && database2.complete)
+    if (database1.complete && database2.complete && database3.complete)
         finishTest();
+    else if (database2.complete)
+        testDatabase(database3);
     else
         testDatabase(database2);
 }
@@ -38,9 +53,20 @@ function addData(db)
     db.transaction(function(tx) {
         log("Inserting some data");
         tx.executeSql("INSERT INTO DataTest (randomData) VALUES (ZEROBLOB(17408))", [],
-                      function(tx, result) { },
-                      function(tx, error) { errorFunction(error); });
-    }, errorFunction, function() {
+                      function(tx, result) { }, statementErrorFunction);
+        if (db == database2) {
+            // Try to run this statement on 'database2' only.
+            // It should not be run, because the previous statement should've
+            // resulted in a failure that made sqlite roll back the entire transaction.
+            tx.executeSql("INSERT INTO DataTest (randomData) VALUES (ZEROBLOB(10))", [],
+                          function(tx, result) {
+                              log("This statement should not have been run.");
+                              finishTest();
+                          }, statementErrorFunction);
+        }
+    }, function(error) {
+        transactionErrorFunction(db, error);
+    }, function() {
         checkCompletion(db);
     });
 }
@@ -50,9 +76,10 @@ function testDatabase(db)
     db.transaction(function(tx) {
         log("Adding a table");
         tx.executeSql("CREATE TABLE DataTest (randomData)", [],
-                      function(tx, result) { },
-                      function(tx, error) { errorFunction(error); });
-    }, errorFunction, function() {
+                      function(tx, result) { }, statementErrorFunction);
+    }, function(error) {
+        transactionErrorFunction(db, error);
+    }, function() {
         addData(db);
     });
 }
@@ -69,8 +96,10 @@ function runTest()
 
     database1 = openDatabase("QuotaManagementDatabase1", "1.0", "Test for quota management <rdar://5628468>", 1);
     database2 = openDatabase("QuotaManagementDatabase2", "1.0", "Test for quota management <rdar://5628468>", 1);
+    database3 = openDatabase("QuotaManagementDatabase3", "1.0", "Test for quota management <rdar://5628468>", 1);
     database1.complete = false;
     database2.complete = false;
+    database3.complete = false;
 
     testDatabase(database1);
 }
@@ -80,7 +109,7 @@ function runTest()
 
 <body onload="runTest()">
 This test checks to make sure that quotas are enforced per-origin instead of per-database, as they were prior to http://trac.webkit.org/projects/webkit/changeset/29983.<br>
-The test clears all databases, sets the quota for the origin to 32k, then tries to insert 17k of data into two databases.  If things go as planned, the UI Delegate will be informed of the exceeded quota.
+The test clears all databases, sets the quota for the origin to 32k, then tries to insert 17k of data into two databases. If things go as planned, the UI Delegate will be informed of the exceeded quota and should increase the quota for this origin. Inserting 17k of data the third time should succeed again.
 <pre id="console">
 </pre>
 </body>
index 85d4a09f7deaa9ddb4e248fbdb1190a092241360..391635f17a453a07622c401f3ef78ec0766fc7e1 100644 (file)
@@ -1,3 +1,34 @@
+2010-02-04  Dumitru Daniliuc  <dumi@chromium.org>
+
+        Reviewed by Eric Seidel.
+
+        1. Fix a bug in SQLiteTransaction: do not assume that COMMIT always
+        succeeds.
+        2. Jump straight to the transaction error callback when a
+        statement fails in a way that makes sqlite automatically rollback
+        the transaction.
+        3. Fix the code that handles the "quota reached" failure, as it is
+        one of the failures that lead to an automatic transaction
+        rollback.
+
+        https://bugs.webkit.org/show_bug.cgi?id=34280
+
+        * platform/sql/SQLiteDatabase.cpp:
+        (WebCore::SQLiteDatabase::isAutoCommitOn):
+        * platform/sql/SQLiteDatabase.h:
+        * platform/sql/SQLiteTransaction.cpp:
+        (WebCore::SQLiteTransaction::begin):
+        (WebCore::SQLiteTransaction::commit):
+        (WebCore::SQLiteTransaction::rollback):
+        (WebCore::SQLiteTransaction::transactionWasRolledBackBySqlite):
+        * platform/sql/SQLiteTransaction.h:
+        * storage/SQLTransaction.cpp:
+        (WebCore::SQLTransaction::SQLTransaction):
+        (WebCore::SQLTransaction::runStatements):
+        (WebCore::SQLTransaction::runCurrentStatement):
+        (WebCore::SQLTransaction::handleCurrentStatementError):
+        (WebCore::SQLTransaction::deliverQuotaIncreaseCallback):
+
 2010-02-04  Peter Kasting  <pkasting@google.com>
 
         Not reviewed, rollback.
index d170db58e7d553e490cc6d74ecbf569909d2ce3d..faaf5def600a027be92ef1ebcfc2a0f54f66538e 100644 (file)
@@ -361,4 +361,9 @@ void SQLiteDatabase::unlock()
     m_lockingMutex.unlock();
 }
 
+bool SQLiteDatabase::isAutoCommitOn() const
+{
+    return sqlite3_get_autocommit(m_db);
+}
+
 } // namespace WebCore
index 99822545e8c4ab6259dc3373e3432b43faa2d1f2..a3e9852a9586dd222c5620677a3307fb2ae6d8f1 100644 (file)
@@ -106,6 +106,7 @@ public:
     // (un)locks the database like a mutex
     void lock();
     void unlock();
+    bool isAutoCommitOn() const;
 
 private:
     static int authorizerFunction(void*, int, const char*, const char*, const char*, const char*);
index a34613f69e99b980f2dbdaeb9ff00acd93af84d9..6f90eac0894e5fc311dba3f04752d1ce2b938b61 100644 (file)
@@ -55,33 +55,31 @@ void SQLiteTransaction::begin()
         // http://www.sqlite.org/lang_transaction.html
         // http://www.sqlite.org/lockingv3.html#locking
         if (m_readOnly)
-            m_inProgress = m_db.executeCommand("BEGIN;");
+            m_inProgress = m_db.executeCommand("BEGIN");
         else
-            m_inProgress = m_db.executeCommand("BEGIN IMMEDIATE;");
+            m_inProgress = m_db.executeCommand("BEGIN IMMEDIATE");
         m_db.m_transactionInProgress = m_inProgress;
     }
 }
 
 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;");
-        m_inProgress = false;
-        m_db.m_transactionInProgress = false;
+        m_inProgress = !m_db.executeCommand("COMMIT");
+        m_db.m_transactionInProgress = m_inProgress;
     }
 }
 
 void SQLiteTransaction::rollback()
 {
+    // We do not use the 'm_inProgress = m_db.executeCommand("ROLLBACK")' construct here,
+    // because m_inProgress should always be set to false after a ROLLBACK, and
+    // m_db.executeCommand("ROLLBACK") can sometimes harmlessly fail, thus returning
+    // a non-zero/true result (http://www.sqlite.org/lang_transaction.html).
     if (m_inProgress) {
         ASSERT(m_db.m_transactionInProgress);
-        m_db.executeCommand("ROLLBACK;");
+        m_db.executeCommand("ROLLBACK");
         m_inProgress = false;
         m_db.m_transactionInProgress = false;
     }
@@ -95,4 +93,11 @@ void SQLiteTransaction::stop()
     }
 }
 
+bool SQLiteTransaction::wasRolledBackBySqlite() const
+{
+    // According to http://www.sqlite.org/c3ref/get_autocommit.html,
+    // the auto-commit flag should be off in the middle of a transaction
+    return m_inProgress && m_db.isAutoCommitOn();
+}
+
 } // namespace WebCore
index 557d81cb4e20b9a0130d4691105e33fe6ae155ec..924241ff5c8a03b1a69726aff6a13619b9cce897 100644 (file)
@@ -44,6 +44,7 @@ public:
     void stop();
     
     bool inProgress() const { return m_inProgress; }
+    bool wasRolledBackBySqlite() const;
 private:
     SQLiteDatabase& m_db;
     bool m_inProgress;
index db25e1a734b2b14f369ed02f80b3ea965d00e675..754cebc3a91c0543519680ca30924a11f2d86140 100644 (file)
@@ -313,7 +313,7 @@ void SQLTransaction::runStatements()
     // If there is a series of statements queued up that are all successful and have no associated
     // SQLStatementCallback objects, then we can burn through the queue
     do {
-        if (m_shouldRetryCurrentStatement) {
+        if (m_shouldRetryCurrentStatement && !m_sqliteTransaction->wasRolledBackBySqlite()) {
             m_shouldRetryCurrentStatement = false;
             // FIXME - Another place that needs fixing up after <rdar://problem/5628468> is addressed.
             // See ::openTransactionAndPreflight() for discussion
@@ -393,8 +393,8 @@ bool SQLTransaction::runCurrentStatement()
 void SQLTransaction::handleCurrentStatementError()
 {
     // Transaction Steps 6.error - Call the statement's error callback, but if there was no error callback,
-    // jump to the transaction error callback
-    if (m_currentStatement->hasStatementErrorCallback()) {
+    // or the transaction was rolled back, jump to the transaction error callback
+    if (m_currentStatement->hasStatementErrorCallback() && !m_sqliteTransaction->wasRolledBackBySqlite()) {
         m_nextStep = &SQLTransaction::deliverStatementCallback;
         LOG(StorageAPI, "Scheduling deliverStatementCallback for transaction %p\n", this);
         m_database->scheduleTransactionCallback(this);