Bug 60032 – IndexedDB: Transaction rollback prevented by open SQLite statement

• Component: New Bugs
• Status: RESOLVED FIXED

[Hans Wennborg]

IndexedDB: Test transaction rollback

[Hans Wennborg]

Created attachment 92083 [details]
Patch

[Hans Wennborg]

Comment on attachment 92083 [details]
Patch

Oops, the expectations are wrong. Hmm, looks like this works in Chrome, but not in DumpRenderTree? Hmm :S

[Hans Wennborg]

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

[David Grogan]

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

[Hans Wennborg]

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

[Andrei Popescu]

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?

[Hans Wennborg]

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

[Andrei Popescu]

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

[Hans Wennborg]

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

[Hans Wennborg]

Created attachment 92410 [details]
Patch

[Andrei Popescu]

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.

[Hans Wennborg]

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

[Hans Wennborg]

Created attachment 92423 [details]
Patch

[Hans Wennborg]

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

[Andrei Popescu]

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

[Hans Wennborg]

Steve, would you like to take a look?

[Steve Block]

Comment on attachment 92423 [details]
Patch

LGTM based on Andrei's review

[Hans Wennborg]

Committed r86422: <http://trac.webkit.org/changeset/86422>