RESOLVED FIXED 147672
DatabaseTracker::closeAllDatabases calls Database::close from the wrong thread
https://bugs.webkit.org/show_bug.cgi?id=147672
Summary DatabaseTracker::closeAllDatabases calls Database::close from the wrong thread
Anders Carlsson
Reported 2015-08-04 18:10:25 PDT
DatabaseTracker::closeAllDatabases() iterates through all open databases and calls Database::close(), but that function can only be called from the database thread.
Attachments
Patch (12.14 KB, patch)
2016-04-27 21:05 PDT, Daniel Bates
no flags
Patch (13.47 KB, patch)
2016-04-29 09:58 PDT, Daniel Bates
no flags
Archive of layout-test-results from ews103 for mac-yosemite (967.25 KB, application/zip)
2016-04-29 10:44 PDT, Build Bot
no flags
Archive of layout-test-results from ews106 for mac-yosemite-wk2 (824.01 KB, application/zip)
2016-04-29 10:47 PDT, Build Bot
no flags
Archive of layout-test-results from ews122 for ios-simulator-wk2 (637.18 KB, application/zip)
2016-04-29 11:05 PDT, Build Bot
no flags
Patch (14.25 KB, patch)
2016-04-29 11:05 PDT, Daniel Bates
beidson: review+
Daniel Bates
Comment 1 2016-04-27 16:08:28 PDT
Daniel Bates
Comment 2 2016-04-27 21:05:47 PDT
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.
Brady Eidson
Comment 3 2016-04-28 10:05:34 PDT
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?
Darin Adler
Comment 4 2016-04-28 23:52:30 PDT
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?
Daniel Bates
Comment 5 2016-04-29 09:07:36 PDT
(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?
Daniel Bates
Comment 6 2016-04-29 09:54:51 PDT
(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>.
Daniel Bates
Comment 7 2016-04-29 09:58:54 PDT
Build Bot
Comment 8 2016-04-29 10:44:50 PDT
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
Build Bot
Comment 9 2016-04-29 10:44:54 PDT
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
Build Bot
Comment 10 2016-04-29 10:47:55 PDT
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
Build Bot
Comment 11 2016-04-29 10:47:59 PDT
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
Build Bot
Comment 12 2016-04-29 11:05:31 PDT
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
Build Bot
Comment 13 2016-04-29 11:05:35 PDT
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
Daniel Bates
Comment 14 2016-04-29 11:05:49 PDT
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.
Brady Eidson
Comment 15 2016-04-29 11:58:38 PDT
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.
Daniel Bates
Comment 16 2016-05-02 08:56:33 PDT
Note You need to log in before you can comment on or make changes to this bug.