Changeset 245334 in webkit

Timestamp:

May 15, 2019 11:29:40 AM (5 years ago)

Author:

Chris Dumez

Message:

[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:

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• NetworkProcess/NetworkProcess.cpp (modified) (2 diffs)
• NetworkProcess/NetworkProcess.h (modified) (1 diff)
• NetworkProcess/NetworkProcess.messages.in (modified) (1 diff)
• UIProcess/Network/NetworkProcessProxy.cpp (modified) (1 diff)
• UIProcess/WebProcessProxy.cpp (modified) (1 diff)
• UIProcess/ios/ProcessAssertionIOS.mm (modified) (5 diffs)
• WebProcess/WebProcess.cpp (modified) (2 diffs)
• WebProcess/WebProcess.h (modified) (2 diffs)
• WebProcess/WebProcess.messages.in (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp

@@ -1950 +1950 @@
 }
 
-void NetworkProcess::processWillSuspendImminently(CompletionHandler<void(bool)>&& completionHandler)
-{
-    RELEASE_LOG(ProcessSuspension, "%p - NetworkProcess::processWillSuspendImminently()", this);
+void NetworkProcess::processWillSuspendImminently()
+{
+    RELEASE_LOG(ProcessSuspension, "%p - NetworkProcess::processWillSuspendImminently() BEGIN", this);
 #if PLATFORM(IOS_FAMILY) && ENABLE(INDEXED_DATABASE)
     for (auto& server : m_idbServers.values())
@@ -1958 +1958 @@
 #endif
     actualPrepareToSuspend(ShouldAcknowledgeWhenReadyToSuspend::No);
-    completionHandler(true);
+    RELEASE_LOG(ProcessSuspension, "%p - NetworkProcess::processWillSuspendImminently() END", this);
 }
 

trunk/Source/WebKit/NetworkProcess/NetworkProcess.h

@@ -182 +182 @@
     bool canHandleHTTPSServerTrustEvaluation() const { return m_canHandleHTTPSServerTrustEvaluation; }
 
-    void processWillSuspendImminently(CompletionHandler<void(bool)>&&);
+    void processWillSuspendImminently();
     void prepareToSuspend();
     void cancelPrepareToSuspend();

trunk/Source/WebKit/NetworkProcess/NetworkProcess.messages.in

@@ -78 +78 @@
     ProcessDidTransitionToForeground()
 
-    ProcessWillSuspendImminently() -> (bool handled) Synchronous
+    ProcessWillSuspendImminently()
     PrepareToSuspend()
     CancelPrepareToSuspend()

trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp

@@ -958 +958 @@
 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);
 }
     

trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp

@@ -1147 +1147 @@
 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);
 }
 

trunk/Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm

@@ -38 +38 @@
 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
 
@@ -51 +56 @@
     UIBackgroundTaskIdentifier _backgroundTask;
     HashSet<ProcessAndUIAssertion*> _assertionsNeedingBackgroundTask;
-    BOOL _assertionHasExpiredInTheBackground;
+    BOOL _applicationIsBackgrounded;
+    dispatch_block_t _pendingTaskReleaseTask;
 }
 
@@ -69 +75 @@
 
     [[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;
 }
@@ -102 +113 @@
 }
 
+
+- (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;
         }
@@ -122 +161 @@
             }
 
-            // 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())

trunk/Source/WebKit/WebProcess/WebProcess.cpp

@@ -733 +733 @@
     if (messageReceiverMap().dispatchSyncMessage(connection, decoder, replyEncoder))
         return;
-
-    didReceiveSyncWebProcessMessage(connection, decoder, replyEncoder);
 }
 
@@ -1491 +1489 @@
 }
 
-void WebProcess::processWillSuspendImminently(CompletionHandler<void(bool)>&& completionHandler)
-{
-    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);
+void WebProcess::processWillSuspendImminently()
+{
+    RELEASE_LOG(ProcessSuspension, "%p - WebProcess::processWillSuspendImminently() BEGIN", this);
     actualPrepareToSuspend(ShouldAcknowledgeWhenReadyToSuspend::No);
-    completionHandler(true);
+    RELEASE_LOG(ProcessSuspension, "%p - WebProcess::processWillSuspendImminently() END", this);
 }
 

trunk/Source/WebKit/WebProcess/WebProcess.h

@@ -217 +217 @@
     void setHiddenPageDOMTimerThrottlingIncreaseLimit(int milliseconds);
 
-    void processWillSuspendImminently(CompletionHandler<void(bool)>&&);
+    void processWillSuspendImminently();
     void prepareToSuspend();
     void cancelPrepareToSuspend();
@@ -421 +421 @@
     // 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)

trunk/Source/WebKit/WebProcess/WebProcess.messages.in

@@ -97 +97 @@
     DestroyAutomationSessionProxy()
 
-    ProcessWillSuspendImminently() -> (bool handled) Synchronous
+    ProcessWillSuspendImminently()
     PrepareToSuspend()
     CancelPrepareToSuspend()