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>.
Created attachment 252439 [details] Patch
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.
Created attachment 252443 [details] Patch
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.
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.
Created attachment 252522 [details] Patch Updated patch.
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.
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.
(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().
Committed r183909: <http://trac.webkit.org/changeset/183909>
<rdar://problem/18894598>
(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.
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.