DatabaseTracker::closeAllDatabases calls Database::close from the wrong thread

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Anders Carlsson]

DatabaseTracker::closeAllDatabases() iterates through all open databases and calls Database::close(), but that function can only be called from the database thread.

[Daniel Bates]

<rdar://problem/22357464>

[Daniel Bates]

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 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 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]

(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]

(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]

Created attachment 277710 [details]
Patch

[Build Bot]

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]

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 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]

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 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]

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]

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 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]

Committed r200322: <http://trac.webkit.org/changeset/200322>