Bug 60032 - IndexedDB: Transaction rollback prevented by open SQLite statement
: IndexedDB: Transaction rollback prevented by open SQLite statement
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Other Mac OS X 10.5
: P2 Normal
Assigned To: Hans Wennborg
:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-05-03 09:13 PDT by Hans Wennborg
Modified: 2011-05-13 03:21 PDT (History)
3 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Hans Wennborg 2011-05-03 09:13:08 PDT
IndexedDB: Test transaction rollback
Comment 1 Hans Wennborg 2011-05-03 09:14:08 PDT
Created attachment 92083 [details]
Patch
Comment 2 Hans Wennborg 2011-05-03 09:31:38 PDT
Comment on attachment 92083 [details]
Patch

Oops, the expectations are wrong. Hmm, looks like this works in Chrome, but not in DumpRenderTree? Hmm :S
Comment 3 Hans Wennborg 2011-05-03 10:26:21 PDT
(In reply to comment #2)
> (From update of attachment 92083 [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 David Grogan 2011-05-03 18:01:06 PDT
Comment on attachment 92083 [details]
Patch

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 Hans Wennborg 2011-05-04 04:11:28 PDT
(In reply to comment #4)
> (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

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 Andrei Popescu 2011-05-04 05:12:28 PDT
Comment on attachment 92083 [details]
Patch

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 Hans Wennborg 2011-05-04 05:59:24 PDT
(In reply to comment #6)
> (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.

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 Andrei Popescu 2011-05-04 06:01:28 PDT
(In reply to comment #7)
> (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.
> 

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 Hans Wennborg 2011-05-05 06:34:57 PDT
(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 Hans Wennborg 2011-05-05 06:46:32 PDT
Created attachment 92410 [details]
Patch
Comment 11 Andrei Popescu 2011-05-05 07:24:01 PDT
Comment on attachment 92410 [details]
Patch

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 Hans Wennborg 2011-05-05 08:57:14 PDT
(In reply to comment #11)
> (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 } ?
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 Hans Wennborg 2011-05-05 08:57:39 PDT
Created attachment 92423 [details]
Patch
Comment 14 Hans Wennborg 2011-05-09 07:58:06 PDT
(In reply to comment #12)
> (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.

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 Andrei Popescu 2011-05-11 07:13:02 PDT
(In reply to comment #14)
> (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?

Yeah, I guess....ok, thanks for investigating the options. LGTM.
Comment 16 Hans Wennborg 2011-05-11 07:27:07 PDT
Steve, would you like to take a look?
Comment 17 Steve Block 2011-05-13 02:58:27 PDT
Comment on attachment 92423 [details]
Patch

LGTM based on Andrei's review
Comment 18 Hans Wennborg 2011-05-13 03:21:31 PDT
Committed r86422: <http://trac.webkit.org/changeset/86422>