Bug 89512

Summary: [Chromium] IndexedDB: Don't close database if pending connections are in flight
Product: WebKit Reporter: Joshua Bell <jsbell>
Component: WebCore Misc.Assignee: Joshua Bell <jsbell>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, dgrogan, tony, vsevik, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing none

Description Joshua Bell 2012-06-19 13:59:49 PDT
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.
Comment 1 Joshua Bell 2012-06-19 14:53:12 PDT
Created attachment 148432 [details]
Patch
Comment 2 Joshua Bell 2012-06-19 14:54:45 PDT
+vsevik@ for the inspector changes. This patch requires that the second phase of database connections is processed.
Comment 3 Joshua Bell 2012-06-19 14:55:16 PDT
tony@ can you take a look?
Comment 4 Tony Chang 2012-06-19 15:23:13 PDT
Comment on attachment 148432 [details]
Patch

LGTM
Comment 5 Joshua Bell 2012-06-19 15:32:17 PDT
dgrogan@, can you take a look too since we were discussing this and how it would relate to your upcoming patch?
Comment 6 David Grogan 2012-06-19 17:03:24 PDT
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?
Comment 7 David Grogan 2012-06-19 17:05:16 PDT
I didn't look at the inspector part at all.
Comment 8 Joshua Bell 2012-06-19 17:53:32 PDT
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.
Comment 9 Joshua Bell 2012-06-19 17:54:09 PDT
Created attachment 148474 [details]
Patch for landing
Comment 10 WebKit Review Bot 2012-06-20 09:18:09 PDT
Comment on attachment 148474 [details]
Patch for landing

Clearing flags on attachment: 148474

Committed r120828: <http://trac.webkit.org/changeset/120828>
Comment 11 WebKit Review Bot 2012-06-20 09:18:13 PDT
All reviewed patches have been landed.  Closing bug.