Bug 34152

Summary: SQLiteTransactions's destructor should not try to rollback a transaction in progress
Product: WebKit Reporter: Dumitru Daniliuc <dumi>
Component: New BugsAssignee: Dumitru Daniliuc <dumi>
Status: RESOLVED FIXED    
Severity: Normal CC: michaeln
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
patch
eric: review-, dumi: commit-queue-
patch
dumi: commit-queue-
patch
dumi: commit-queue-
patch eric: review+, dumi: commit-queue-

Description Dumitru Daniliuc 2010-01-25 20:00:06 PST
Currently, SQLiteTransaction's destructor tries to rollback a transaction that's in progress. This is not the right thing to do and leads to an assertion failure if the SQLiteTransaction object happens to be destroyed on the main thread (this could happen at shutdown). I believe the right thing to do is to do nothing in ~SQLiteTransaction. SQLTransaction already has logic to rollback a transaction if a transaction's error callback is invoked; and if a shutdown happens in the middle of a transaction, the DB thread makes sure to close all DB connections, which makes sqlite automatically rollback any transaction in progress, according to http://www.sqlite.org/c3ref/close.html.
Comment 1 Dumitru Daniliuc 2010-01-25 20:32:08 PST
Created attachment 47383 [details]
patch
Comment 2 Michael Nordman 2010-01-26 12:04:04 PST
Hmmm... I don't think this is a good change.

sql/SQLiteTransaction dtor with the call to rollback() if in progress looks very correct. 

It sounds like there are some cases where that behavior doesn't work well for storage/SQLTransaction, but I think the place to 'fix' whatever issues those are would be closer to storage/SQLTransaction.
Comment 3 Eric Seidel (no email) 2010-01-26 14:02:28 PST
Comment on attachment 47383 [details]
patch

Should we be replacing this with an ASSERT?  r- pending response to michael's comment.
Comment 4 Dumitru Daniliuc 2010-01-26 20:36:11 PST
Created attachment 47488 [details]
patch

An alternative approach: destroy the SQLiteTransaction objects associated with in-progress transactions (and thus, roll back those transactions) on the DB thread, before the DB thread goes away, but after it stops processing tasks from its task queue.
Comment 5 Michael Nordman 2010-01-26 21:12:35 PST
(In reply to comment #4)
> Created an attachment (id=47488) [details]
> patch

This approach looks much better to me.

It may nice to ASSERT(!m_sqliteTransaction) in the ~SQLTransation.

442	    m_database->m_databaseAuthorizer->disable();
443	    m_sqliteTransaction->commit();
444	    m_database->m_databaseAuthorizer->enable();
445	
446	    // If the commit failed, the transaction will still be marked as "in progress"
447	    if (m_sqliteTransaction->inProgress()) {
448	        m_transactionError = SQLError::create(0, "failed to commit the transaction");
449	        handleTransactionError(false);
450	        return;
451	    }

           m_sqliteTransaction.clear();

I think we could make that assertion if the ptr was cleared after successfully committing in postflightAndCommit(). The error cases and early thread shutdown cases result in a cleared ptr, only the success case leaves it allocated.
Comment 6 Dumitru Daniliuc 2010-01-27 13:20:48 PST
Created attachment 47560 [details]
patch

Please take another look.
Comment 7 Michael Nordman 2010-01-27 14:17:33 PST
this lgtm
Comment 8 Michael Nordman 2010-01-27 14:23:33 PST
Somewhat unrelated to this change...

void SQLiteTransaction::commit()
{
    if (m_inProgress) {
        ASSERT(m_db.m_transactionInProgress);
        m_db.executeCommand("COMMIT;");
        m_inProgress = false;
        m_db.m_transactionInProgress = false;
    }
}

... what if the COMMIT fails?
Comment 9 Dumitru Daniliuc 2010-01-27 14:32:22 PST
(In reply to comment #8)
> Somewhat unrelated to this change...
> 
> void SQLiteTransaction::commit()
> {
>     if (m_inProgress) {
>         ASSERT(m_db.m_transactionInProgress);
>         m_db.executeCommand("COMMIT;");
>         m_inProgress = false;
>         m_db.m_transactionInProgress = false;
>     }
> }
> 
> ... what if the COMMIT fails?

This is a bug that should be fixed in this patch, because SQLTransaction is expecting m_inProgress to be false if commit() fails. SQLTransaction treats a failed commit() as any other transaction error, and tries to rollback the transaction.

New patch coming in a minute.
Comment 10 Dumitru Daniliuc 2010-01-27 18:34:26 PST
Actually, while trying to make the last change Michael suggested ("what if COMMIT fails?"), we ran into a much bigger problem, that is way beyond the scope of this bug. So Michael and I decided that the last patch that I uploaded (and Michael LGTM'd) is the most appropriate patch for this bug.

Eric (or whoever will be reviewing this patch), can you please take a look at the last patch, and ignore Michael's comments about COMMIT failing for now? I will open a separate bug for that issue.
Comment 11 Dumitru Daniliuc 2010-01-29 16:25:13 PST
Created attachment 47743 [details]
patch
Comment 12 Eric Seidel (no email) 2010-02-01 15:10:38 PST
Comment on attachment 47743 [details]
patch

OK.
Comment 13 Dumitru Daniliuc 2010-02-01 15:33:31 PST
Landed as r54162.