Bug 91010 – IndexedDB: Expose mechanism for database to force a connection to close

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: Expose mechanism for database to force a connection to close

[Joshua Bell]

Created attachment 151765 [details]
Patch

[Joshua Bell]

WebKit side of a fix for http://crbug.com/64050 - can't delete browsing data in Chromium port when a connection is open. Chromium side is: https://chromiumcodereview.appspot.com/10695158

This patch exposes a "forceClose" API that the back end can call on each connection to force it to close. In the Chromium port this is done in the browser itself which keeps track of these connections.

I'm not sure where - if anywhere - to put WebKit side tests for this. I'm also not a fan of the name, so suggestions welcome.

[Joshua Bell]

dgrogan@, alecflett@ - any comments? I should probably get this puppy landing.

I'm still not happy with forceClose as a name. Maybe terminateConnection?

[Alec Flett]

Comment on attachment 151765 [details]
Patch

LGTM - I like "forceClose" more..

[David Grogan]

Comment on attachment 151765 [details]
Patch

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

LGTM.

I haven't looked through the chrome patch yet.

> Source/WebCore/Modules/indexeddb/IDBDatabaseCallbacksImpl.cpp:51
> +    if (m_database)

It's legitimate for m_database to be NULL here, this can't be an assert?

> Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp:104
> +    if (!m_databaseCallbacks)

Does this only happen if forceClose is called twice?

[David Grogan]

(In reply to comment #4)
> (From update of attachment 151765 [details])
> LGTM - I like "forceClose" more..

Oh, and agreed.  forceClose ftw.

[David Grogan]

(In reply to comment #2)
> I'm not sure where - if anywhere - to put WebKit side tests for this.

We could add a method to window.internals/layouttestcontroller that simulates a forceClose, or just unit test it.

[Joshua Bell]

Comment on attachment 151765 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBDatabaseCallbacksImpl.cpp:51
>> +    if (m_database)
> 
> It's legitimate for m_database to be NULL here, this can't be an assert?

Not sure what you mean...

m_database can be NULL if the front-end has destructed, which can happen during script GC. Script may be the only thing holding an IDBDatabase alive; when script GC occurs the refcount drops to 0, and the destructor runs which closes the connection (an async call to the backend) and nulls out m_database in the CallbacksImpl. If there's a forceClose in flight from the backend to the frontend these could collide.

So... it's legitimate for m_database to be NULL, so we can't assert.

>> Source/WebKit/chromium/src/WebIDBDatabaseImpl.cpp:104
>> +    if (!m_databaseCallbacks)
> 
> Does this only happen if forceClose is called twice?

Or a close followed by a forceClose, e.g. a close() is in-flight already from script when the UI triggers a forceClose(). On the Chromium side it looks like this class is retained in the dispatcher's map until the database backend is actually destroyed, so nothing earlier prevents that.

[Joshua Bell]

Created attachment 158650 [details]
Patch

[WebKit Review Bot]

Please wait for approval from [e-mail], [e-mail], [e-mail], [e-mail] or [e-mail] before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.

[Joshua Bell]

Created attachment 160039 [details]
Patch

[Joshua Bell]

(In reply to comment #11)
> Created an attachment (id=160039) [details]
> Patch

Rebase on top of the upgradeneeded changes.

dglazkov@, abarth@ - review of the Chromium WebKit public API changes?

[Adam Barth]

Comment on attachment 160039 [details]
Patch

API change LGTM

[Joshua Bell]

tony@ - r?

[Joshua Bell]

Created attachment 160273 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 160273 [details]
Patch for landing

Clearing flags on attachment: 160273

Committed r126516: <http://trac.webkit.org/changeset/126516>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.