Bug 34280 - sqlite can silently roll back a transaction if executing a statement results in an error
Summary: sqlite can silently roll back a transaction if executing a statement results ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P1 Normal
Assignee: Dumitru Daniliuc
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-01-28 12:14 PST by Dumitru Daniliuc
Modified: 2010-02-12 15:54 PST (History)
6 users (show)

See Also:


Attachments
patch (13.04 KB, patch)
2010-02-02 16:07 PST, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch (9.88 KB, patch)
2010-02-02 18:42 PST, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch (13.32 KB, patch)
2010-02-02 20:24 PST, Dumitru Daniliuc
eric: review+
dumi: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dumitru Daniliuc 2010-01-28 12:14:22 PST
According to http://www.sqlite.org/c3ref/get_autocommit.html, sqlite can roll back the entire transaction if executing a statement in that transaction results in a certain error. For example, this happens when the max size of the database is reached while running a statement (LayoutTests/storage/quota-tracking.html). We haven't discovered this problem before, because a bug in WebCore/platform/sql/SQLiteTransaction.cpp was masking it (commit() assumes that m_db.executeCommand("COMMIT") always succeeds).

I don't have any ideas for how to fix this problem at the moment, but if we don't do anything about it, then we can't implement some parts of the WebSQLDatabases spec as they're currently specified. Namely, the spec says that if a statement failed, but has an error callback associated with it that returns false, then we should just move on to the next statement. We cannot do that if sqlite decided to rollback the transaction because of the failed statement.
Comment 1 Michael Nordman 2010-01-29 17:46:51 PST
Ouch... surprising behavior all right, "the transaction might be rolled back automatically".

Not sure i agree that the spec is unimplementable with this behavior. We can detect when this auto-rollback of a transaction has happened with sqlite3_get_autocommit(). So...

Prior to executing any statement in a transaction, test sqlite3_get_autocommit() and if it returns true don't execute the statement, treat this condition as a statement failure. Do the same when it comes time to finally COMMIT.
Comment 2 Dumitru Daniliuc 2010-02-02 16:07:09 PST
Created attachment 47981 [details]
patch

Hixie stated on public-webapps that it's fine to jump straight to the transaction's error callback (and thus, ignore any other pending statements in a transaction), if a statement failed in a way that made sqlite rollback the entire transaction.
Comment 3 WebKit Review Bot 2010-02-02 16:11:52 PST
Attachment 47981 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/storage/SQLTransaction.cpp:387:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Dumitru Daniliuc 2010-02-02 17:17:40 PST
(In reply to comment #3)
> Attachment 47981 [details] did not pass style-queue:
> 
> Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
> WebCore/storage/SQLTransaction.cpp:387:  Boolean expressions that span multiple
> lines should have their operators on the left side of the line instead of the
> right side.  [whitespace/operators] [4]
> Total errors found: 1
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

I am going to ignore this suggestion, because most of the existing code does not follow it.
Comment 5 Michael Nordman 2010-02-02 18:18:26 PST
368     // If the last statement failed because of quota restrictions, then it means sqlite
369     // has already rolled back this transaction; still, ask the user to increase the
370     // quota for this origin for the next run of this application

Missing a '.' at the end here. But more significantly...

From the sqlite docs, SQLITE_FULL doesn't always mean that the xaction was auto rolled back. It looks like there are some cases where the strategy of re-running the current statement with a larger pageSize would work as intended. Is that right?
Comment 6 Dumitru Daniliuc 2010-02-02 18:42:53 PST
Created attachment 47987 [details]
patch

Please take another look.
Comment 7 WebKit Review Bot 2010-02-02 18:45:32 PST
Attachment 47987 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
WebCore/storage/SQLTransaction.cpp:397:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 1


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 8 Michael Nordman 2010-02-02 19:13:53 PST
397     if (m_currentStatement->hasStatementErrorCallback() &&
398         !m_sqliteTransaction->transactionWasRolledBackBySqlite()) {

Can this be on one line in the "no line is too long" webcore style?

403         // http://www.sqlite.org/lang_transaction.html recommends issuing a manual ROLLBACK command,
404         // even if the transaction was already rolled back by sqlite. That's why we don't call
405         // m_sqliteTransaction->stop() here and allow cleanupAfterTransactionErrorCallback()
406         // to eventually call m_sqliteTransaction->rollback().

Not sure you really need this comment here.

47     bool transactionWasRolledBackBySqlite() const;

I wonder if this method name could be improved? Certainly 'transaction' in the name seems redundant, wasRolledBackBySqlite() is a littler shorter.

98     return m_inProgress && m_db.isAutoCommitOn();

A comment a ptr to http://www.sqlite.org/lang_transaction.html may be better in here.

9      Test failed - there was not enough remaining storage space, or the storage quota was reached and the user declined to allow more space.

Can the test be arranged to differently to verify that the quota was bumped up?
Comment 9 Dumitru Daniliuc 2010-02-02 20:24:57 PST
Created attachment 47990 [details]
patch

(In reply to comment #8)
> 397     if (m_currentStatement->hasStatementErrorCallback() &&
> 398         !m_sqliteTransaction->transactionWasRolledBackBySqlite()) {
> 
> Can this be on one line in the "no line is too long" webcore style?

done.

> 403         // http://www.sqlite.org/lang_transaction.html recommends issuing a
> manual ROLLBACK command,
> 404         // even if the transaction was already rolled back by sqlite.
> That's why we don't call
> 405         // m_sqliteTransaction->stop() here and allow
> cleanupAfterTransactionErrorCallback()
> 406         // to eventually call m_sqliteTransaction->rollback().
> 
> Not sure you really need this comment here.

removed.

> 47     bool transactionWasRolledBackBySqlite() const;
> 
> I wonder if this method name could be improved? Certainly 'transaction' in the
> name seems redundant, wasRolledBackBySqlite() is a littler shorter.

done.

> 98     return m_inProgress && m_db.isAutoCommitOn();
> 
> A comment a ptr to http://www.sqlite.org/lang_transaction.html may be better in
> here.

added a reference to http://www.sqlite.org/c3ref/get_autocommit.html. i think it states more clearly that this flag is off during a transaction, so if it's on, then it means the transaction was rolled back.

> 9      Test failed - there was not enough remaining storage space, or the
> storage quota was reached and the user declined to allow more space.
> 
> Can the test be arranged to differently to verify that the quota was bumped up?

done.
Comment 10 Michael Nordman 2010-02-03 13:03:23 PST
lgtm (fwiw)
Comment 11 Michael Nordman 2010-02-03 15:18:13 PST
Maybe we should open a new bug to track the issue with sometimes not being able to bump the quota up and continuing with the same transaction due to it having been automatically rolled back?
Comment 12 Dumitru Daniliuc 2010-02-04 14:54:32 PST
(In reply to comment #11)
> Maybe we should open a new bug to track the issue with sometimes not being able
> to bump the quota up and continuing with the same transaction due to it having
> been automatically rolled back?

this patch fixes that behavior. that is, we still allow the user to bump up the quota, and if the transaction was not rolled back, we continue; otherwise, we do not run the remaining statements. the changes i've made to quota-tracking.html test this.
Comment 13 Eric Seidel (no email) 2010-02-04 15:22:45 PST
Comment on attachment 47990 [details]
patch

LGTM.
Comment 14 Daniel Bates 2010-02-04 21:50:18 PST
This appears to have been committed in changeset 54393 <http://trac.webkit.org/changeset/54393>.
Comment 15 Daniel Bates 2010-02-04 21:53:37 PST
The test storage/quota-tracking.html (included in the patch <https://bugs.webkit.org/attachment.cgi?id=47990>) is failing on the Snow Leopard Intel Release bot.

The results are at: <http://build.webkit.org/results/SnowLeopard%20Intel%20Release%20(Tests)/r54393%20(5258)/storage/>

Stdio output is at: <http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28Tests%29/builds/5258/steps/layout-test/logs/stdio>
Comment 16 Dumitru Daniliuc 2010-02-12 15:54:30 PST
Re-closing this bug. This test will be fixed in bug 34911.