Bug 89512 – [Chromium] IndexedDB: Don't close database if pending connections are in flight

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joshua Bell]

If the IDB front IDBDatabaseBackendImpl, IDBRequest and IDBDatabase objects are communicating asynchronously (e.g. Chromium port), then there is a race condition:

A normal connection opens - note the two phase openConnection() / registerFrontendCallbacks()

* IDBRequest req1 = IDBFactory::open()
* which async invokes IDBDatabaseBackendImpl::openConnection()
* which async invokes IDBRequest::OnSuccess() on req1, which creates IDBDatabase connection1
* which async invokes IDBDatabaseBackendImpl::registerFrontendCallbacks(IDBDatabase), which "registers" connection1

Now a second connection opens, where

* IDBRequest req2 = IDBFactory::open()
* which async invokes IDBDatabaseBackendImpl::openConnection()
* which async invokes IDBRequest::OnSuccess() on req2, which creates IDBDatabase connection2
* which async invokes IDBDatabaseBackendImpl::registerFrontendCallbacks(IDBDatabase)...
* INTERRUPT: 
* connection1 async invokes IDBDatabase::close() which calls IDBDatabaseBackendImpl::close()
* at this point there are no registered callbacks, so it releases its backing store and drops itself from the factory
* RESUME: 
* IDBDatabaseBackendImpl::registerFrontendCallbacks(IDBDatabase) "registers" connection2

* connection2.setVersion() is called, but the backing store ref has been released. *boom*

So the bug is that IDBDatabaseBackendImpl::close should not close itself if there are pending connections in the limbo state between openConnection() and registerFrontendCallbacks(). The correct long term fix is probably to eliminate this limbo state entirely.

[Joshua Bell]

Created attachment 148432 [details]
Patch

[Joshua Bell]

+vsevik@ for the inspector changes. This patch requires that the second phase of database connections is processed.

[Joshua Bell]

tony@ can you take a look?

[Tony Chang]

Comment on attachment 148432 [details]
Patch

LGTM

[Joshua Bell]

dgrogan@, can you take a look too since we were discussing this and how it would relate to your upcoming patch?

[David Grogan]

Comment on attachment 148432 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148432&action=review

LGTM

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:347
> +    RefPtr<IDBDatabaseBackendImpl> self = this;

What scenario does this prevent?

[David Grogan]

I didn't look at the inspector part at all.

[Joshua Bell]

Comment on attachment 148432 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=148432&action=review

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:347
>> +    RefPtr<IDBDatabaseBackendImpl> self = this;
> 
> What scenario does this prevent?

Whoops, good catch. That's just residue from early debugging.

[Joshua Bell]

Created attachment 148474 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 148474 [details]
Patch for landing

Clearing flags on attachment: 148474

Committed r120828: <http://trac.webkit.org/changeset/120828>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.