Bug 89512 - [Chromium] IndexedDB: Don't close database if pending connections are in flight
Summary: [Chromium] IndexedDB: Don't close database if pending connections are in flight
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joshua Bell
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-06-19 13:59 PDT by Joshua Bell
Modified: 2012-06-20 10:20 PDT (History)
5 users (show)

See Also:


Attachments
Patch (19.09 KB, patch)
2012-06-19 14:53 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff
Patch for landing (18.72 KB, patch)
2012-06-19 17:54 PDT, Joshua Bell
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.