Bug 34280 – sqlite can silently roll back a transaction if executing a statement results in an error

• Component: New Bugs
• Status: RESOLVED FIXED

[Dumitru Daniliuc]

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.

[Michael Nordman]

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.

[Dumitru Daniliuc]

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.

[WebKit Review Bot]

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.

[Dumitru Daniliuc]

(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.

[Michael Nordman]

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?

[Dumitru Daniliuc]

Created attachment 47987 [details]
patch

Please take another look.

[WebKit Review Bot]

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.

[Michael Nordman]

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?

[Dumitru Daniliuc]

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.

[Michael Nordman]

lgtm (fwiw)

[Michael Nordman]

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?

[Dumitru Daniliuc]

(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.

[Eric Seidel (no email)]

Comment on attachment 47990 [details]
patch

LGTM.

[Daniel Bates]

This appears to have been committed in changeset 54393 <http://trac.webkit.org/changeset/54393>.

[Daniel Bates]

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>

[Dumitru Daniliuc]

Re-closing this bug. This test will be fixed in bug 34911.