[WK2][iOS] UIProcess may get killed because it is taking too long to release its background task after expiration
https://bugs.webkit.org/show_bug.cgi?id=197893
<rdar://problem/50234105>
Reviewed by Alex Christensen.
The UIProcess may get killed because it is taking too long to release its background task after its expiration handler
was called. The reason is that the background task's expiration handler was sequentially sending a ProcessWillSuspendImminently
synchronous IPC to each of its child processes and only then ends the background task. By the time we receive the response from
all child processes, it may be too late and we get killed.
To address the issue, we now:
1. Send the ProcessWillSuspendImminently asynchronously so that all processes can do their processing in parallel
2. After 2 seconds, the UIProcess releases the background task (We get killed after ~5 seconds)
Also, to make sure that the UIProcess supends promptly, we now make sure we never start a new background task *after*
the app has been backgrounded. The intention of our background task is too finish critical work (like releasing locked
files) after the app gets backgrounded, not to start new work and delay process suspension.
* NetworkProcess/NetworkProcess.cpp:
(WebKit::NetworkProcess::processWillSuspendImminently):
* NetworkProcess/NetworkProcess.h:
* NetworkProcess/NetworkProcess.messages.in:
* UIProcess/Network/NetworkProcessProxy.cpp:
(WebKit::NetworkProcessProxy::sendProcessWillSuspendImminently):
* UIProcess/WebProcessProxy.cpp:
(WebKit::WebProcessProxy::sendProcessWillSuspendImminently):
* UIProcess/ios/ProcessAssertionIOS.mm:
(-[WKProcessAssertionBackgroundTaskManager init]):
(-[WKProcessAssertionBackgroundTaskManager _scheduleReleaseTask]):
(-[WKProcessAssertionBackgroundTaskManager _cancelPendingReleaseTask]):
(-[WKProcessAssertionBackgroundTaskManager _updateBackgroundTask]):
* WebProcess/WebProcess.cpp:
(WebKit::WebProcess::didReceiveSyncMessage):
(WebKit::WebProcess::processWillSuspendImminently):
* WebProcess/WebProcess.h:
* WebProcess/WebProcess.messages.in:
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@245334 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
index 91dbaf5..648254c 100644
--- a/Source/WebKit/ChangeLog
+++ b/Source/WebKit/ChangeLog
@@ -1,3 +1,43 @@
+2019-05-15 Chris Dumez <cdumez@apple.com>
+
+ [WK2][iOS] UIProcess may get killed because it is taking too long to release its background task after expiration
+ https://bugs.webkit.org/show_bug.cgi?id=197893
+ <rdar://problem/50234105>
+
+ Reviewed by Alex Christensen.
+
+ The UIProcess may get killed because it is taking too long to release its background task after its expiration handler
+ was called. The reason is that the background task's expiration handler was sequentially sending a ProcessWillSuspendImminently
+ synchronous IPC to each of its child processes and only then ends the background task. By the time we receive the response from
+ all child processes, it may be too late and we get killed.
+
+ To address the issue, we now:
+ 1. Send the ProcessWillSuspendImminently asynchronously so that all processes can do their processing in parallel
+ 2. After 2 seconds, the UIProcess releases the background task (We get killed after ~5 seconds)
+
+ Also, to make sure that the UIProcess supends promptly, we now make sure we never start a new background task *after*
+ the app has been backgrounded. The intention of our background task is too finish critical work (like releasing locked
+ files) after the app gets backgrounded, not to start new work and delay process suspension.
+
+ * NetworkProcess/NetworkProcess.cpp:
+ (WebKit::NetworkProcess::processWillSuspendImminently):
+ * NetworkProcess/NetworkProcess.h:
+ * NetworkProcess/NetworkProcess.messages.in:
+ * UIProcess/Network/NetworkProcessProxy.cpp:
+ (WebKit::NetworkProcessProxy::sendProcessWillSuspendImminently):
+ * UIProcess/WebProcessProxy.cpp:
+ (WebKit::WebProcessProxy::sendProcessWillSuspendImminently):
+ * UIProcess/ios/ProcessAssertionIOS.mm:
+ (-[WKProcessAssertionBackgroundTaskManager init]):
+ (-[WKProcessAssertionBackgroundTaskManager _scheduleReleaseTask]):
+ (-[WKProcessAssertionBackgroundTaskManager _cancelPendingReleaseTask]):
+ (-[WKProcessAssertionBackgroundTaskManager _updateBackgroundTask]):
+ * WebProcess/WebProcess.cpp:
+ (WebKit::WebProcess::didReceiveSyncMessage):
+ (WebKit::WebProcess::processWillSuspendImminently):
+ * WebProcess/WebProcess.h:
+ * WebProcess/WebProcess.messages.in:
+
2019-05-15 Jiewen Tan <jiewen_tan@apple.com>
[WebAuthN] Make WebAuthN default on
diff --git a/Source/WebKit/NetworkProcess/NetworkProcess.cpp b/Source/WebKit/NetworkProcess/NetworkProcess.cpp
index 9c64844..b775889 100644
--- a/Source/WebKit/NetworkProcess/NetworkProcess.cpp
+++ b/Source/WebKit/NetworkProcess/NetworkProcess.cpp
@@ -1949,15 +1949,15 @@
#endif
}
-void NetworkProcess::processWillSuspendImminently(CompletionHandler<void(bool)>&& completionHandler)
+void NetworkProcess::processWillSuspendImminently()
{
- RELEASE_LOG(ProcessSuspension, "%p - NetworkProcess::processWillSuspendImminently()", this);
+ RELEASE_LOG(ProcessSuspension, "%p - NetworkProcess::processWillSuspendImminently() BEGIN", this);
#if PLATFORM(IOS_FAMILY) && ENABLE(INDEXED_DATABASE)
for (auto& server : m_idbServers.values())
server->tryStop(IDBServer::ShouldForceStop::Yes);
#endif
actualPrepareToSuspend(ShouldAcknowledgeWhenReadyToSuspend::No);
- completionHandler(true);
+ RELEASE_LOG(ProcessSuspension, "%p - NetworkProcess::processWillSuspendImminently() END", this);
}
void NetworkProcess::prepareToSuspend()
diff --git a/Source/WebKit/NetworkProcess/NetworkProcess.h b/Source/WebKit/NetworkProcess/NetworkProcess.h
index d8563d2..40a1acb 100644
--- a/Source/WebKit/NetworkProcess/NetworkProcess.h
+++ b/Source/WebKit/NetworkProcess/NetworkProcess.h
@@ -181,7 +181,7 @@
bool canHandleHTTPSServerTrustEvaluation() const { return m_canHandleHTTPSServerTrustEvaluation; }
- void processWillSuspendImminently(CompletionHandler<void(bool)>&&);
+ void processWillSuspendImminently();
void prepareToSuspend();
void cancelPrepareToSuspend();
void processDidResume();
diff --git a/Source/WebKit/NetworkProcess/NetworkProcess.messages.in b/Source/WebKit/NetworkProcess/NetworkProcess.messages.in
index 35bc82b..74c5025 100644
--- a/Source/WebKit/NetworkProcess/NetworkProcess.messages.in
+++ b/Source/WebKit/NetworkProcess/NetworkProcess.messages.in
@@ -77,7 +77,7 @@
ProcessDidTransitionToBackground()
ProcessDidTransitionToForeground()
- ProcessWillSuspendImminently() -> (bool handled) Synchronous
+ ProcessWillSuspendImminently()
PrepareToSuspend()
CancelPrepareToSuspend()
ProcessDidResume()
diff --git a/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp b/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp
index 0ee81be..81c4f0e 100644
--- a/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp
+++ b/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp
@@ -957,11 +957,8 @@
void NetworkProcessProxy::sendProcessWillSuspendImminently()
{
- if (!canSendMessage())
- return;
-
- bool handled = false;
- sendSync(Messages::NetworkProcess::ProcessWillSuspendImminently(), Messages::NetworkProcess::ProcessWillSuspendImminently::Reply(handled), 0, 1_s);
+ if (canSendMessage())
+ send(Messages::NetworkProcess::ProcessWillSuspendImminently(), 0);
}
void NetworkProcessProxy::sendPrepareToSuspend()
diff --git a/Source/WebKit/UIProcess/WebProcessProxy.cpp b/Source/WebKit/UIProcess/WebProcessProxy.cpp
index 4248ec9..8203465 100644
--- a/Source/WebKit/UIProcess/WebProcessProxy.cpp
+++ b/Source/WebKit/UIProcess/WebProcessProxy.cpp
@@ -1146,11 +1146,8 @@
void WebProcessProxy::sendProcessWillSuspendImminently()
{
- if (!canSendMessage())
- return;
-
- bool handled = false;
- sendSync(Messages::WebProcess::ProcessWillSuspendImminently(), Messages::WebProcess::ProcessWillSuspendImminently::Reply(handled), 0, 1_s);
+ if (canSendMessage())
+ send(Messages::WebProcess::ProcessWillSuspendImminently(), 0);
}
void WebProcessProxy::sendPrepareToSuspend()
diff --git a/Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm b/Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm
index dc35acc..2aca868 100644
--- a/Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm
+++ b/Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm
@@ -37,6 +37,11 @@
using WebKit::ProcessAndUIAssertion;
+// This gives some time to our child processes to process the ProcessWillSuspendImminently IPC but makes sure we release
+// the background task before the UIKit timeout (We get killed if we do not release the background task within 5 seconds
+// on the expiration handler getting called).
+static const Seconds releaseBackgroundTaskAfterExpirationDelay { 2_s };
+
@interface WKProcessAssertionBackgroundTaskManager : NSObject
+ (WKProcessAssertionBackgroundTaskManager *)shared;
@@ -50,7 +55,8 @@
{
UIBackgroundTaskIdentifier _backgroundTask;
HashSet<ProcessAndUIAssertion*> _assertionsNeedingBackgroundTask;
- BOOL _assertionHasExpiredInTheBackground;
+ BOOL _applicationIsBackgrounded;
+ dispatch_block_t _pendingTaskReleaseTask;
}
+ (WKProcessAssertionBackgroundTaskManager *)shared
@@ -68,10 +74,15 @@
_backgroundTask = UIBackgroundTaskInvalid;
[[NSNotificationCenter defaultCenter] addObserverForName:UIApplicationWillEnterForegroundNotification object:[UIApplication sharedApplication] queue:nil usingBlock:^(NSNotification *) {
- _assertionHasExpiredInTheBackground = NO;
+ _applicationIsBackgrounded = NO;
+ [self _cancelPendingReleaseTask];
[self _updateBackgroundTask];
}];
+ [[NSNotificationCenter defaultCenter] addObserverForName:UIApplicationDidEnterBackgroundNotification object:[UIApplication sharedApplication] queue:nil usingBlock:^(NSNotification *) {
+ _applicationIsBackgrounded = YES;
+ }];
+
return self;
}
@@ -101,12 +112,40 @@
assertion->uiAssertionWillExpireImminently();
}
+
+- (void)_scheduleReleaseTask
+{
+ ASSERT(!_pendingTaskReleaseTask);
+ if (_pendingTaskReleaseTask)
+ return;
+
+ RELEASE_LOG(ProcessSuspension, "%p - WKProcessAssertionBackgroundTaskManager - _scheduleReleaseTask because the expiration handler has been called", self);
+ _pendingTaskReleaseTask = dispatch_block_create((dispatch_block_flags_t)0, ^{
+ _pendingTaskReleaseTask = nil;
+ [self _releaseBackgroundTask];
+ });
+ dispatch_after(dispatch_time(DISPATCH_TIME_NOW, releaseBackgroundTaskAfterExpirationDelay.value() * NSEC_PER_SEC), dispatch_get_main_queue(), _pendingTaskReleaseTask);
+#if !__has_feature(objc_arc)
+ // dispatch_async() does a Block_copy() / Block_release() on behalf of the caller.
+ Block_release(_pendingTaskReleaseTask);
+#endif
+}
+
+- (void)_cancelPendingReleaseTask
+{
+ if (!_pendingTaskReleaseTask)
+ return;
+
+ RELEASE_LOG(ProcessSuspension, "%p - WKProcessAssertionBackgroundTaskManager - _cancelPendingReleaseTask because the application is foreground again", self);
+ dispatch_block_cancel(_pendingTaskReleaseTask);
+ _pendingTaskReleaseTask = nil;
+}
+
- (void)_updateBackgroundTask
{
if (!_assertionsNeedingBackgroundTask.isEmpty() && _backgroundTask == UIBackgroundTaskInvalid) {
- if (_assertionHasExpiredInTheBackground) {
- RELEASE_LOG_ERROR(ProcessSuspension, "%p - WKProcessAssertionBackgroundTaskManager: Ignored request to start a background task because we're still in the background and the previous task expired", self);
- // Our invalidation handler would not get called if we tried to re-take a new background assertion at this point, and the UIProcess would get killed (rdar://problem/50001505).
+ if (_applicationIsBackgrounded) {
+ RELEASE_LOG_ERROR(ProcessSuspension, "%p - WKProcessAssertionBackgroundTaskManager: Ignored request to start a new background task because the application is already in the background", self);
return;
}
RELEASE_LOG(ProcessSuspension, "%p - WKProcessAssertionBackgroundTaskManager - beginBackgroundTaskWithName", self);
@@ -121,9 +160,7 @@
});
}
- // Remember that the assertion has expired in the background so we do not try to re-take it until the application becomes foreground again.
- _assertionHasExpiredInTheBackground = YES;
- [self _releaseBackgroundTask];
+ [self _scheduleReleaseTask];
}];
} else if (_assertionsNeedingBackgroundTask.isEmpty())
[self _releaseBackgroundTask];
diff --git a/Source/WebKit/WebProcess/WebProcess.cpp b/Source/WebKit/WebProcess/WebProcess.cpp
index 97b8305..d21bbf6 100644
--- a/Source/WebKit/WebProcess/WebProcess.cpp
+++ b/Source/WebKit/WebProcess/WebProcess.cpp
@@ -732,8 +732,6 @@
{
if (messageReceiverMap().dispatchSyncMessage(connection, decoder, replyEncoder))
return;
-
- didReceiveSyncWebProcessMessage(connection, decoder, replyEncoder);
}
void WebProcess::didReceiveMessage(IPC::Connection& connection, IPC::Decoder& decoder)
@@ -1490,19 +1488,11 @@
});
}
-void WebProcess::processWillSuspendImminently(CompletionHandler<void(bool)>&& completionHandler)
+void WebProcess::processWillSuspendImminently()
{
- if (parentProcessConnection()->inSendSync()) {
- // Avoid reentrency bugs such as rdar://problem/21605505 by just bailing
- // if we get an incoming ProcessWillSuspendImminently message when waiting for a
- // reply to a sync message.
- // FIXME: ProcessWillSuspendImminently should not be a sync message.
- return completionHandler(false);
- }
-
- RELEASE_LOG(ProcessSuspension, "%p - WebProcess::processWillSuspendImminently()", this);
+ RELEASE_LOG(ProcessSuspension, "%p - WebProcess::processWillSuspendImminently() BEGIN", this);
actualPrepareToSuspend(ShouldAcknowledgeWhenReadyToSuspend::No);
- completionHandler(true);
+ RELEASE_LOG(ProcessSuspension, "%p - WebProcess::processWillSuspendImminently() END", this);
}
void WebProcess::prepareToSuspend()
diff --git a/Source/WebKit/WebProcess/WebProcess.h b/Source/WebKit/WebProcess/WebProcess.h
index 4b6ddc6..96d3ce9 100644
--- a/Source/WebKit/WebProcess/WebProcess.h
+++ b/Source/WebKit/WebProcess/WebProcess.h
@@ -216,7 +216,7 @@
void setHiddenPageDOMTimerThrottlingIncreaseLimit(int milliseconds);
- void processWillSuspendImminently(CompletionHandler<void(bool)>&&);
+ void processWillSuspendImminently();
void prepareToSuspend();
void cancelPrepareToSuspend();
void processDidResume();
@@ -420,7 +420,6 @@
// Implemented in generated WebProcessMessageReceiver.cpp
void didReceiveWebProcessMessage(IPC::Connection&, IPC::Decoder&);
- void didReceiveSyncWebProcessMessage(IPC::Connection&, IPC::Decoder&, std::unique_ptr<IPC::Encoder>&);
#if PLATFORM(MAC)
void setScreenProperties(const WebCore::ScreenProperties&);
diff --git a/Source/WebKit/WebProcess/WebProcess.messages.in b/Source/WebKit/WebProcess/WebProcess.messages.in
index 3847412..b39b38c 100644
--- a/Source/WebKit/WebProcess/WebProcess.messages.in
+++ b/Source/WebKit/WebProcess/WebProcess.messages.in
@@ -96,7 +96,7 @@
EnsureAutomationSessionProxy(String sessionIdentifier)
DestroyAutomationSessionProxy()
- ProcessWillSuspendImminently() -> (bool handled) Synchronous
+ ProcessWillSuspendImminently()
PrepareToSuspend()
CancelPrepareToSuspend()
ProcessDidResume()
