IndexedDB: Expose mechanism for database to force a connection to close
Created attachment 151765 [details] Patch
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.
dgrogan@, alecflett@ - any comments? I should probably get this puppy landing. I'm still not happy with forceClose as a name. Maybe terminateConnection?
Comment on attachment 151765 [details] Patch LGTM - I like "forceClose" more..
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?
(In reply to comment #4) > (From update of attachment 151765 [details]) > LGTM - I like "forceClose" more.. Oh, and agreed. forceClose ftw.
(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.
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.
Created attachment 158650 [details] Patch
Please wait for approval from abarth@webkit.org, dglazkov@chromium.org, fishd@chromium.org, jamesr@chromium.org or tkent@chromium.org before submitting, as this patch contains changes to the Chromium public API. See also https://trac.webkit.org/wiki/ChromiumWebKitAPI.
Created attachment 160039 [details] Patch
(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?
Comment on attachment 160039 [details] Patch API change LGTM
tony@ - r?
Created attachment 160273 [details] Patch for landing
Comment on attachment 160273 [details] Patch for landing Clearing flags on attachment: 160273 Committed r126516: <http://trac.webkit.org/changeset/126516>
All reviewed patches have been landed. Closing bug.