WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
REOPENED
144657
[iOS][WK2] Pause/resume database thread when UIProcess enters/leaves the background
https://bugs.webkit.org/show_bug.cgi?id=144657
Summary
[iOS][WK2] Pause/resume database thread when UIProcess enters/leaves the back...
Daniel Bates
Reported
2015-05-05 19:10:50 PDT
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
>.
Attachments
Patch
(13.48 KB, patch)
2015-05-05 19:27 PDT
,
Daniel Bates
no flags
Details
Formatted Diff
Diff
Patch
(13.66 KB, patch)
2015-05-05 20:21 PDT
,
Daniel Bates
no flags
Details
Formatted Diff
Diff
Patch
(13.71 KB, patch)
2015-05-05 20:30 PDT
,
Daniel Bates
no flags
Details
Formatted Diff
Diff
Patch
(14.32 KB, patch)
2015-05-06 14:55 PDT
,
Daniel Bates
aestes
: review+
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Daniel Bates
Comment 1
2015-05-05 19:27:40 PDT
Created
attachment 252439
[details]
Patch
Daniel Bates
Comment 2
2015-05-05 20:21:44 PDT
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
Comment 3
2015-05-05 20:30:00 PDT
Created
attachment 252443
[details]
Patch
Andy Estes
Comment 4
2015-05-06 10:52:11 PDT
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 5
2015-05-06 10:53:57 PDT
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
Comment 6
2015-05-06 14:55:27 PDT
Created
attachment 252522
[details]
Patch Updated patch.
WebKit Commit Bot
Comment 7
2015-05-06 14:59:13 PDT
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 8
2015-05-06 15:27:05 PDT
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
Comment 9
2015-05-06 20:22:12 PDT
(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
Comment 10
2015-05-06 20:25:20 PDT
Committed
r183909
: <
http://trac.webkit.org/changeset/183909
>
Daniel Bates
Comment 11
2015-05-06 20:37:33 PDT
<
rdar://problem/18894598
>
Andy Estes
Comment 12
2015-05-06 22:55:00 PDT
(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
Comment 13
2015-05-21 17:40:38 PDT
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug