Bug 144657 – [iOS][WK2] Pause/resume database thread when UIProcess enters/leaves the background

• Component: WebKit2
• Status: REOPENED

[Daniel Bates]

We should pause and resume the database thread when the UIProcess enters and leaves the background, respectively, to avoid WebProcess termination due to holding a locked SQLite database file when the WebProcess is suspended.

Analogous behavior is implemented in Legacy WebKit. See <http://trac.webkit.org/browser/trunk/Source/WebKit/ios/Misc/WebUIKitSupport.mm?rev=176347#L113>, <http://trac.webkit.org/browser/trunk/Source/WebKit/ios/Misc/WebUIKitSupport.mm?rev=176347#L118> and <http://trac.webkit.org/browser/trunk/Source/WebKit/mac/Storage/WebDatabaseManager.mm?rev=180704#L291>.

[Daniel Bates]

Created attachment 252439 [details]
Patch

[Daniel Bates]

Created attachment 252442 [details]
Patch

Add PLATFORM(IOS)-guard around call to DatabaseTracker::tracker().setDatabasesPaused() in implementation of DatabaseServer::setPauseAllDatabases(). Filed bug #144660 to remove this added PLATFORM(IOS)-guard as well as other such guards around the database thread pause/resume logic.

[Daniel Bates]

Created attachment 252443 [details]
Patch

[Andy Estes]

Comment on attachment 252443 [details]
Patch

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

> Source/WebCore/Modules/webdatabase/DatabaseServer.h:63
> +    virtual void setPauseAllDatabases(bool) override;

No need for 'virtual' here.

[Daniel Bates]

Comment on attachment 252443 [details]
Patch

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

> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:603
> +    m_process->send(Messages::WebPage::ApplicationDidEnterBackground(), m_pageID);

We need to take a process assertion out on the WebProcess before sending this message to ensure we actually deliver it.

[Daniel Bates]

Created attachment 252522 [details]
Patch

Updated patch.

[WebKit Commit Bot]

Attachment 252522 [details] did not pass style-queue:


ERROR: Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:603:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Andy Estes]

Comment on attachment 252522 [details]
Patch

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

r=me. I made a few nit-picky suggestions, but feel free to land as-is.

> Source/WebCore/Modules/webdatabase/AbstractDatabaseServer.h:74
> +    virtual void setPauseAllDatabases(bool) = 0;

I think this name reads strangely, having two consecutive verbs. setDatabasesPaused sounds slightly better to me.

> Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:603
> +    uint64_t callbackID = m_callbacks.put(std::function<void (CallbackBase::Error)>(), m_process->throttler().backgroundActivityToken());

To me this would read more easily with a lambda: [](CallbackBase::Error) { }

> Source/WebKit2/WebProcess/WebCoreSupport/WebDatabaseManager.h:49
> +    void setPauseAllDatabases(bool = true);

I'm not sure the default argument is worth it. Seems clearer to explicitly pass true at the one call site.

[Daniel Bates]

(In reply to comment #8)
> [...]
> > Source/WebCore/Modules/webdatabase/AbstractDatabaseServer.h:74
> > +    virtual void setPauseAllDatabases(bool) = 0;
> 
> I think this name reads strangely, having two consecutive verbs.
> setDatabasesPaused sounds slightly better to me.
> 

I chose the name setPauseAllDatabases to conform to the Cocoa naming conventions. We tend to follow Cocoa naming conventions.

Notice that the name setDatabasesPaused disagrees with the rule "don’t twist a verb into an adjective by using a participle" in section Naming Methods of the Coding Guidelines for Cocoa: <https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/CodingGuidelines/Articles/NamingMethods.html>.

> > Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:603
> > +    uint64_t callbackID = m_callbacks.put(std::function<void (CallbackBase::Error)>(), m_process->throttler().backgroundActivityToken());
> 
> To me this would read more easily with a lambda: [](CallbackBase::Error) { }
> 

Will write this line as:

uint64_t callbackID = m_callbacks.put(VoidCallback::create([](CallbackBase::Error) { }, m_process->throttler().backgroundActivityToken()));

Additional remarks:

I tried using a lambda expression directly in the call to m_callbacks.put(), but WebPageProxyIOS.mm fails to compile because it cannot find a viable function for CallbackMap::put():
 
[[
...
CompileC WebPageProxyIOS.o
/Volumes/.../OpenSource/Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:603:39: error: no matching member function for call to 'put'
In file included from /Volumes/.../OpenSource/Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:27:
In file included from /Volumes/.../OpenSource/Source/WebKit2/UIProcess/WebPageProxy.h:32:
In file included from /Volumes/.../OpenSource/Source/WebKit2/UIProcess/AutoCorrectionCallback.h:30:
/Volumes/.../OpenSource/Source/WebKit2/UIProcess/GenericCallback.h:194:14: note: candidate template ignored: could not match 'function<void (type-parameter-0-0...)>' against '(lambda at /Volumes/.../OpenSource/Source/WebKit2/UIProcess/ios/WebPageProxyIOS.mm:603:43)'
/Volumes/.../OpenSource/Source/WebKit2/UIProcess/GenericCallback.h:174:14: note: candidate function not viable: requires single argument 'callback', but 2 arguments were provided
1 error generated.
]]

Notice that a lambda expression has an implementation defined type and a std::function can be defined to store a lambda expression. The issue is that the compiler does not know which std::function template to instantiate to match the type of the lambda expression; => it cannot instantiate the variadic template variant of CallbackMap::put(). Obviously, the compiler cannot use CallbackMap::put(PassRefPtr<CallbackBase>). Therefore it cannot find a viable function for CallbackMap::put() when a lambda expression is specified. One way to work around this is explicitly tell the compiler the type of std::function to instantiate for the lambda. The downside of this approach is that you will repeat the data types for the parameters used in the lambda expression.

> > Source/WebKit2/WebProcess/WebCoreSupport/WebDatabaseManager.h:49
> > +    void setPauseAllDatabases(bool = true);
> 
> I'm not sure the default argument is worth it. Seems clearer to explicitly
> pass true at the one call site.

Will remove default argument and explicitly pass true when calling WebDatabaseManager::setPauseAllDatabases() in WebPage::applicationDidEnterBackground().

[Daniel Bates]

Committed r183909: <http://trac.webkit.org/changeset/183909>

[Daniel Bates]

<rdar://problem/18894598>

[Andy Estes]

(In reply to comment #9)
> (In reply to comment #8)
> > [...]
> > > Source/WebCore/Modules/webdatabase/AbstractDatabaseServer.h:74
> > > +    virtual void setPauseAllDatabases(bool) = 0;
> > 
> > I think this name reads strangely, having two consecutive verbs.
> > setDatabasesPaused sounds slightly better to me.
> > 
> 
> I chose the name setPauseAllDatabases to conform to the Cocoa naming
> conventions. We tend to follow Cocoa naming conventions.
>
> Notice that the name setDatabasesPaused disagrees with the rule "don’t twist
> a verb into an adjective by using a participle" in section Naming Methods of
> the Coding Guidelines for Cocoa:
> <https://developer.apple.com/library/mac/documentation/Cocoa/Conceptual/
> CodingGuidelines/Articles/NamingMethods.html>.

The guideline you cite is specific to naming the getter and setter methods of an Objective-C property (it's in the "Accessor Methods" subsection). I don't think that's relevant to how you name a C++ member function, especially one that is not a simple setter.

[Daniel Bates]

Reverted <http://trac.webkit.org/changeset/183909> in <http://trac.webkit.org/changeset/184740> since pausing the database thread prevents in-progress transactions from completing.