Bug 60032 - IndexedDB: Transaction rollback prevented by open SQLite statement
: IndexedDB: Transaction rollback prevented by open SQLite statement
Status: RESOLVED FIXED
: WebKit
New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
:
  Show dependency treegraph
 
Reported: 2011-05-03 09:13 PST by
Modified: 2011-05-13 03:21 PST (History)


Attachments
Patch (5.10 KB, patch)
2011-05-03 09:14 PST, Hans Wennborg
no flags Review Patch | Details | Formatted Diff | Diff
Patch (15.19 KB, patch)
2011-05-05 06:46 PST, Hans Wennborg
no flags Review Patch | Details | Formatted Diff | Diff
Patch (17.97 KB, patch)
2011-05-05 08:57 PST, Hans Wennborg
steveblock: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2011-05-03 09:13:08 PST
IndexedDB: Test transaction rollback
------- Comment #1 From 2011-05-03 09:14:08 PST -------
Created an attachment (id=92083) [details]
Patch
------- Comment #2 From 2011-05-03 09:31:38 PST -------
(From update of attachment 92083 [details])
Oops, the expectations are wrong. Hmm, looks like this works in Chrome, but not in DumpRenderTree? Hmm :S
------- Comment #3 From 2011-05-03 10:26:21 PST -------
(In reply to comment #2)
> (From update of attachment 92083 [details] [details])
> Oops, the expectations are wrong. Hmm, looks like this works in Chrome, but not in DumpRenderTree? Hmm :S

No, the test passes if I run it separately (or with run_webkit_tests.sh --run-singly), but not when it's run as part of the test suite with other tests running in parallel. There's something funky going on here.
------- Comment #4 From 2011-05-03 18:01:06 PST -------
(From update of attachment 92083 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=92083&action=review

> LayoutTests/storage/indexeddb/transaction-rollback.html:48
> +    commitAndContinue(startTransaction);

What's up with commitAndContinue?  To run something after setVersion completes, the tutorial layout test uses

currentTransaction = event.target.result;
currentTransaction.oncomplete = onSetVersionComplete; // startTransaction in your case I suppose
currentTransaction.onabort = unexpectedAbort;

I ask because I was running into the same phenomena (works in --run-singly, doesn't otherwise) when using setTimeout to get async behavior.  I don't have a narrative for your case that explains why you might be running into the same issue, but it seems too coincidental.

dimich explained the problems in https://bugs.webkit.org/show_bug.cgi?id=57789#c35
------- Comment #5 From 2011-05-04 04:11:28 PST -------
(In reply to comment #4)
> (From update of attachment 92083 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92083&action=review
> 
> > LayoutTests/storage/indexeddb/transaction-rollback.html:48
> > +    commitAndContinue(startTransaction);
> 
> What's up with commitAndContinue?  To run something after setVersion completes, the tutorial layout test uses
> 
> currentTransaction = event.target.result;
> currentTransaction.oncomplete = onSetVersionComplete; // startTransaction in your case I suppose
> currentTransaction.onabort = unexpectedAbort;
> 
> I ask because I was running into the same phenomena (works in --run-singly, doesn't otherwise) when using setTimeout to get async behavior.  I don't have a narrative for your case that explains why you might be running into the same issue, but it seems too coincidental.
> 
> dimich explained the problems in https://bugs.webkit.org/show_bug.cgi?id=57789#c35

Ah, thanks for the tip. Unfortunately, this doesn't fix our problem, but at least avoiding setTimeout seems good.

The problem with my layout test is that it fails when running back-to-back with other tests. It has nothing to do with concurrency, it fails even if I run the tests with --child-processes=1.

It seems that a transaction that's created in one test isn't necessarily destructed after that test finishes, and I suspect that can cause trouble for a subsequent test.

I think something is busted with our current transactions implementation.
------- Comment #6 From 2011-05-04 05:12:28 PST -------
(From update of attachment 92083 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=92083&action=review

> LayoutTests/storage/indexeddb/transaction-rollback.html:49
> +}

Yes, we should never use something like commitAndContinue(). Jeremy and I have been trying to eliminate this patter as it's just plain wrong (we're also to blame for it, though :)). If you want to commit a transaction, just set the oncomplete handler.

What exactly is the problem that you are seeing? Is the onabort handler called but the transaction's changes are persisted to disk? Or is abort handler not invoked at all?
------- Comment #7 From 2011-05-04 05:59:24 PST -------
(In reply to comment #6)
> (From update of attachment 92083 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92083&action=review
> 
> > LayoutTests/storage/indexeddb/transaction-rollback.html:49
> > +}
> 
> Yes, we should never use something like commitAndContinue(). Jeremy and I have been trying to eliminate this patter as it's just plain wrong (we're also to blame for it, though :)). If you want to commit a transaction, just set the oncomplete handler.

I guess I chose a bad file to copy-paste from, then :) I see commitAndContinue() in transaction-basics.html, objectstore-basics.html, database-quota.html and database-basics.html.

> 
> What exactly is the problem that you are seeing? Is the onabort handler called but the transaction's changes are persisted to disk? Or is abort handler not invoked at all?

Yes, the onabort handler is called, but the transaction's changes can still be read by a subsequent transaction.
------- Comment #8 From 2011-05-04 06:01:28 PST -------
(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 92083 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=92083&action=review
> > 
> > > LayoutTests/storage/indexeddb/transaction-rollback.html:49
> > > +}
> > 
> > Yes, we should never use something like commitAndContinue(). Jeremy and I have been trying to eliminate this patter as it's just plain wrong (we're also to blame for it, though :)). If you want to commit a transaction, just set the oncomplete handler.
> 
> I guess I chose a bad file to copy-paste from, then :) I see commitAndContinue() in transaction-basics.html, objectstore-basics.html, database-quota.html and database-basics.html.
> 

Damn, we need to clean those up then :)

> > 
> > What exactly is the problem that you are seeing? Is the onabort handler called but the transaction's changes are persisted to disk? Or is abort handler not invoked at all?
> 
> Yes, the onabort handler is called, but the transaction's changes can still be read by a subsequent transaction.

Hmm, if we get that far, we must have called SQLite's abort()...not sure why this happens.
------- Comment #9 From 2011-05-05 06:34:57 PST -------
(In reply to comment #8)
> Hmm, if we get that far, we must have called SQLite's abort()...not sure why this happens.

Turns out the SQLite rollback operation failed (silently!) because we had an outstanding SQL statement that belonged to a cursor which was opened in a previous layout test.

We should make sure we close (as in finalizing the underlying SQLStatement) any open cursors before committing or rolling back a transaction.

Patch coming up.
------- Comment #10 From 2011-05-05 06:46:32 PST -------
Created an attachment (id=92410) [details]
Patch
------- Comment #11 From 2011-05-05 07:24:01 PST -------
(From update of attachment 92410 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=92410&action=review

> Source/WebCore/storage/IDBBackingStore.h:95
>          virtual ~Cursor() {};

Nit: missing space between { and } ?

> Source/WebCore/storage/IDBTransactionBackendInterface.h:58
> +    virtual void unregisterOpenCursor(IDBCursorBackendImpl*) { };

Hmm, we have tried very hard to keep these interfaces as pure as possible....The interface is to allow two implementations: a proxy for Chromium that sends everything over IPC and a real implementation for everyone else. I don't think we should be adding methods that only need to be implemented by the Impl class but not the Proxy. At the very least, if no other solution exists, you should declare these methods as pure virtual and implement them in the proxy with an assert to guarantee that nobody will ever call them on the proxy objects without noticing.
------- Comment #12 From 2011-05-05 08:57:14 PST -------
(In reply to comment #11)
> (From update of attachment 92410 [details] [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=92410&action=review
> 
> > Source/WebCore/storage/IDBBackingStore.h:95
> >          virtual ~Cursor() {};
> 
> Nit: missing space between { and } ?
Oops. Done.

> 
> > Source/WebCore/storage/IDBTransactionBackendInterface.h:58
> > +    virtual void unregisterOpenCursor(IDBCursorBackendImpl*) { };
> 
> Hmm, we have tried very hard to keep these interfaces as pure as possible....The interface is to allow two implementations: a proxy for Chromium that sends everything over IPC and a real implementation for everyone else. I don't think we should be adding methods that only need to be implemented by the Impl class but not the Proxy. At the very least, if no other solution exists, you should declare these methods as pure virtual and implement them in the proxy with an assert to guarantee that nobody will ever call them on the proxy objects without noticing.

We discussed the possibility of using an abort task with the transaction for closing the cursor when a transaction is aborted. However, I keep running into the same problem (an object having a pointer to an IDB*BackendInterface when I really want the *BackendImpl).

I'll look into this some more, but upload the patch with asserts in the proxy for now.
------- Comment #13 From 2011-05-05 08:57:39 PST -------
Created an attachment (id=92423) [details]
Patch
------- Comment #14 From 2011-05-09 07:58:06 PST -------
(In reply to comment #12)
> (In reply to comment #11)
> > (From update of attachment 92410 [details] [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=92410&action=review
> > 
> > > Source/WebCore/storage/IDBBackingStore.h:95
> > >          virtual ~Cursor() {};
> > 
> > Nit: missing space between { and } ?
> Oops. Done.
> 
> > 
> > > Source/WebCore/storage/IDBTransactionBackendInterface.h:58
> > > +    virtual void unregisterOpenCursor(IDBCursorBackendImpl*) { };
> > 
> > Hmm, we have tried very hard to keep these interfaces as pure as possible....The interface is to allow two implementations: a proxy for Chromium that sends everything over IPC and a real implementation for everyone else. I don't think we should be adding methods that only need to be implemented by the Impl class but not the Proxy. At the very least, if no other solution exists, you should declare these methods as pure virtual and implement them in the proxy with an assert to guarantee that nobody will ever call them on the proxy objects without noticing.
> 
> We discussed the possibility of using an abort task with the transaction for closing the cursor when a transaction is aborted. However, I keep running into the same problem (an object having a pointer to an IDB*BackendInterface when I really want the *BackendImpl).
> 
> I'll look into this some more, but upload the patch with asserts in the proxy for now.

These are the different ways to solve this that I've come up with:

1. Let IDBObjectStoreBackendImpl keep track of open cursors. When we open a cursor, we include an abort task which calls IDBObjectStoreBackendImpl::closeOpenCursor(). Problem: a cursor needs to be able to unregister itself when it destructs, but it only has a pointer to the IDBObjectStoreBackend*Interface*. So we get the same problem again.

2. Create a special class to pass into the abort task. When the cursor is created, we can call on that class to register the opened pointer. We also need to pass a pointer to that class into the cursor so it can unregister itself. I don't like this because it feels very complex.

3. Use the current patch. I think this is the cleanest.

I see why we keep the *Interface classes abstract, but I think maybe we're being a bit too strict in that we make it very hard for two *Impl classes to talk to each other -- they're always going to be on the same side of a process boundary anyway, right?
------- Comment #15 From 2011-05-11 07:13:02 PST -------
(In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (From update of attachment 92410 [details] [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=92410&action=review
> > > 
> > > > Source/WebCore/storage/IDBBackingStore.h:95
> > > >          virtual ~Cursor() {};
> > > 
> > > Nit: missing space between { and } ?
> > Oops. Done.
> > 
> > > 
> > > > Source/WebCore/storage/IDBTransactionBackendInterface.h:58
> > > > +    virtual void unregisterOpenCursor(IDBCursorBackendImpl*) { };
> > > 
> > > Hmm, we have tried very hard to keep these interfaces as pure as possible....The interface is to allow two implementations: a proxy for Chromium that sends everything over IPC and a real implementation for everyone else. I don't think we should be adding methods that only need to be implemented by the Impl class but not the Proxy. At the very least, if no other solution exists, you should declare these methods as pure virtual and implement them in the proxy with an assert to guarantee that nobody will ever call them on the proxy objects without noticing.
> > 
> > We discussed the possibility of using an abort task with the transaction for closing the cursor when a transaction is aborted. However, I keep running into the same problem (an object having a pointer to an IDB*BackendInterface when I really want the *BackendImpl).
> > 
> > I'll look into this some more, but upload the patch with asserts in the proxy for now.
> 
> These are the different ways to solve this that I've come up with:
> 
> 1. Let IDBObjectStoreBackendImpl keep track of open cursors. When we open a cursor, we include an abort task which calls IDBObjectStoreBackendImpl::closeOpenCursor(). Problem: a cursor needs to be able to unregister itself when it destructs, but it only has a pointer to the IDBObjectStoreBackend*Interface*. So we get the same problem again.
> 
> 2. Create a special class to pass into the abort task. When the cursor is created, we can call on that class to register the opened pointer. We also need to pass a pointer to that class into the cursor so it can unregister itself. I don't like this because it feels very complex.
> 
> 3. Use the current patch. I think this is the cleanest.
> 
> I see why we keep the *Interface classes abstract, but I think maybe we're being a bit too strict in that we make it very hard for two *Impl classes to talk to each other -- they're always going to be on the same side of a process boundary anyway, right?

Yeah, I guess....ok, thanks for investigating the options. LGTM.
------- Comment #16 From 2011-05-11 07:27:07 PST -------
Steve, would you like to take a look?
------- Comment #17 From 2011-05-13 02:58:27 PST -------
(From update of attachment 92423 [details])
LGTM based on Andrei's review
------- Comment #18 From 2011-05-13 03:21:31 PST -------
Committed r86422: <http://trac.webkit.org/changeset/86422>