DatabaseTracker::closeAllDatabases() iterates through all open databases and calls Database::close(), but that function can only be called from the database thread.
<rdar://problem/22357464>
Created attachment 277581 [details] Patch I am open to suggestions on the approach and names used in this patch. In particular, I am not so happy wwith the name of the enum class DatabaseTracker::CloseDisposition.
Comment on attachment 277581 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277581&action=review I think this is mostly fine, but I did have comments about the name! > Source/WebCore/Modules/webdatabase/DatabaseTracker.h:78 > + enum class CloseDisposition { InterruptCurrentQuery, WaitForCurrentQueryToComplete }; > + WEBCORE_EXPORT void closeAllDatabases(CloseDisposition = CloseDisposition::WaitForCurrentQueryToComplete); Putting enums inside of class {} definition was nice before C++11 because it gave us namespaced enums. I kind of dislike the practice in our `enum class` world, and would rather this be outside of the class definition. As for the name of this one, I agree there's not a lot that's great about it. I don't think "Disposition" is right, as there is no such thing as an inherent personality trait of a "Close". This is more of a requested behavior, and is more about the current running queries than the "close" request. Maybe something like: CurrentQueryBehavior::Interrupt CurrentQueryBehavior::RunToCompletion It's still not obviously a home run, but I think it's much stronger. Play around with that a bit?
Comment on attachment 277581 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277581&action=review > Source/WebCore/Modules/webdatabase/Database.cpp:291 > + if (currentThread() != databaseContext()->databaseThread()->getThreadID()) { Do we need to support calling this on other threads? It doesn’t seem right for the thing to automatically marshal itself over to another thread. Usually that’s the caller’s responsibility and we don’t make a single function for both of these things. > Source/WebCore/Modules/webdatabase/Database.cpp:300 > + synchronizer.waitForTaskCompletion(); Do callers really need this?
(In reply to comment #3) > [...] > I don't think "Disposition" is right, as there is no such thing as an > inherent personality trait of a "Close". > > This is more of a requested behavior, and is more about the current running > queries than the "close" request. Maybe something like: > > CurrentQueryBehavior::Interrupt > CurrentQueryBehavior::RunToCompletion > > It's still not obviously a home run, but I think it's much stronger. Play > around with that a bit? Maybe name this enum CurrentQueryScheduling with enumerators Interrupt and RunToCompletion? Or name this enum SchedulingBehavior with enumerators InterruptCurrentQuery and WaitForCurrentQueryToComplete? Another idea was name the enum ShouldAbortCurrentQuery with enumerator No and Yes. What are your thoughts?
(In reply to comment #4) > Comment on attachment 277581 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=277581&action=review > > > Source/WebCore/Modules/webdatabase/Database.cpp:291 > > + if (currentThread() != databaseContext()->databaseThread()->getThreadID()) { > > Do we need to support calling this on other threads? Currently we do on iOS. On iOS we want to close all databases before Safari/WebProcess is suspended. We currently do this in a process assertion expiration handler. The expiration handler is called from a thread that is not the database thread. > It doesn’t seem right for the thing to automatically marshal itself over to another thread. Usually that’s the caller’s responsibility and we don’t make a single > function for both of these things. I propose that we take a similar approach as we do for Database::openAndVerifyVersion() and Database::performOpenAndVerify(). That is, I propose that we rename Database::close() to Database::performClose() and update DatabaseCloseTask to call the latter. We use the prefix "perform" in the name of Database functions called from database tasks that assume the current thread is the database thread. We can then repurpose the name Database::close() for a function that schedules a DatabaseCloseTask task. > > > Source/WebCore/Modules/webdatabase/Database.cpp:300 > > + synchronizer.waitForTaskCompletion(); > > Do callers really need this? On iOS we depend on Database::close() on blocking until it closes the database as part of closing all databases from our process assertion expiration handler. We need to fix <https://bugs.webkit.org/show_bug.cgi?id=157184>.
Created attachment 277710 [details] Patch
Comment on attachment 277710 [details] Patch Attachment 277710 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/1240504 New failing tests: storage/websql/null-callbacks.html
Created attachment 277715 [details] Archive of layout-test-results from ews103 for mac-yosemite The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-yosemite Platform: Mac OS X 10.10.5
Comment on attachment 277710 [details] Patch Attachment 277710 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/1240508 New failing tests: http/tests/security/storage-blocking-strengthened-local-storage.html
Created attachment 277716 [details] Archive of layout-test-results from ews106 for mac-yosemite-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-yosemite-wk2 Platform: Mac OS X 10.10.5
Comment on attachment 277710 [details] Patch Attachment 277710 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/1240514 New failing tests: http/tests/security/storage-blocking-strengthened-local-storage.html
Created attachment 277718 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.4
Created attachment 277719 [details] Patch Update DatabaseThread::databaseThread() to call Database::performClose() instead of Database::close() as the latter has been repurposed to schedule closing the database. When executing in the database thread we want to actually close the database.
Comment on attachment 277719 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=277719&action=review > Source/WebCore/Modules/webdatabase/Database.cpp:300 > + auto task = std::make_unique<DatabaseCloseTask>(*this, synchronizer); > + databaseContext()->databaseThread()->scheduleImmediateTask(WTFMove(task)); Can be written as: databaseContext()->databaseThread()->scheduleImmediateTask(std::make_unique<DatabaseCloseTask>(*this, synchronizer)); > Source/WebCore/Modules/webdatabase/DatabaseTracker.h:49 > +enum class CurrentQueryBehavior { Interrupt, RunToCompletion }; I think these names are fine.
Committed r200322: <http://trac.webkit.org/changeset/200322>