429377 - should mozStorageConnection::CommitTransaction() set mTransactionInProgress to false if the commit failed because a SQL statement was in progress?

Status: RESOLVED DUPLICATE of bug 462173

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[Seth Spitzer]

should ozStorageConnection::CommitTransaction() set mTransactionInProgress to false if the commit failed because a SQL statement was in progress?

in mozStorageConnection::CommitTransaction(), we have:

427     nsresult rv = ExecuteSimpleSQL (NS_LITERAL_CSTRING("COMMIT TRANSACTION"));
428     // even if the commit fails, the transaction is aborted
429     mTransactionInProgress = PR_FALSE;
430     return rv;

if you happen to do a begin / end transaction on the connection while in the middle of executeStep() of some other query, the commit will fail.

since mTransactionInProgress is false, even after you are done with executeStep() and you reset the statement, the storage layer doesn't think a transaction is in progress (mTransactionInProgress == PR_FALSE), but one is.

now, all calls to beginTransaction() will fail.  When doing beginTransaction in this state, the sqlite error was "Sqlite error: 1 'cannot start a transaction within a transaction'"

on quit, unless you go around the storage layer and call "COMMIT TRANSACTION", we'll roll back all changes since the failed endTransaction().

if we didn't set mTransactionInProgress to PR_FALSE in this scenario, it would future calls to beginTransaction() / endTransaction() (after the statement was not in progress) to work properly.

thought or comments?

I don't think this is biting us in firefox 3, so not a blocker.

but we might want to double check that we don't need to add a reset() to gStmt after this code:

/toolkit/mozapps/downloads/content/downloads.js, line 1159 -- if (!gStmt.executeStep()) {

Comment 1

[Shawn Wilsher :sdwilsh]

Yes, it should in fact.  And we've fixed it in bug 462173.