Attachment 369894 Details for Bug 197893 – Patch

[patch] Patch

bug-197893-20190514143529.patch (text/plain), 14.75 KB, created by Chris Dumez on 2019-05-14 14:35:29 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-05-14 14:35:29 PDT

Size: 14.75 KB

patch

obsolete

>Subversion Revision: 245297
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 14027746d04fec57476fb4e859baec4aabf393d5..06a3a290a7ba076e13c7077e07e206150ec0f311 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,43 @@
>+2019-05-14  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 NOBODY (OOPS!).
>+
>+        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-14  Chris Dumez  <cdumez@apple.com>
> 
>         The network process tries to take a process assertion when NetworkProcess::processWillSuspendImminently() is called
>diff --git a/Source/WebKit/NetworkProcess/NetworkProcess.cpp b/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>index 9c6484449c6ba27503c8bc05c70709b3e43f8943..b775889381180cf7364c062b9dce942b5dabe6fd 100644
>--- a/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>+++ b/Source/WebKit/NetworkProcess/NetworkProcess.cpp
>@@ -1949,15 +1949,15 @@ void NetworkProcess::actualPrepareToSuspend(ShouldAcknowledgeWhenReadyToSuspend
> #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 d8563d22ab4f9c31ed5cf1943c53d15bd42abbeb..40a1acbbf22d5e1575830b47372af5b3e6c43ad8 100644
>--- a/Source/WebKit/NetworkProcess/NetworkProcess.h
>+++ b/Source/WebKit/NetworkProcess/NetworkProcess.h
>@@ -181,7 +181,7 @@ public:
> 
>     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 35bc82b28f1f7e8e2641a3421f24706de22ac5f9..74c50253538f0a2b66e1bbf9528a021b40ca2be6 100644
>--- a/Source/WebKit/NetworkProcess/NetworkProcess.messages.in
>+++ b/Source/WebKit/NetworkProcess/NetworkProcess.messages.in
>@@ -77,7 +77,7 @@ messages -> NetworkProcess LegacyReceiver {
>     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 af859aec0362e91f930b286b8f42fc20fa883043..b9ac676adaf1a751717dfd4bd43c8be03fa78181 100644
>--- a/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp
>+++ b/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp
>@@ -957,11 +957,8 @@ void NetworkProcessProxy::deleteWebsiteDataInUIProcessForRegistrableDomains(PAL:
> 
> 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 79c2e5a56cae66b32076688204d4d2059301cd24..cd16c81db672b10718d9d86cf11d7eace0ac54e8 100644
>--- a/Source/WebKit/UIProcess/WebProcessProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessProxy.cpp
>@@ -1134,11 +1134,8 @@ RefPtr<API::Object> WebProcessProxy::transformObjectsToHandles(API::Object* obje
> 
> 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 dc35acce8d73c455e0f4c7d7d47f48e6421ffa20..11cfbb51bc123d017b0775b58434ac359dd7618f 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 @@ @implementation WKProcessAssertionBackgroundTaskManager
> {
>     UIBackgroundTaskIdentifier _backgroundTask;
>     HashSet<ProcessAndUIAssertion*> _assertionsNeedingBackgroundTask;
>-    BOOL _assertionHasExpiredInTheBackground;
>+    BOOL _applicationIsBackgrounded;
>+    dispatch_block_t _pendingTaskReleaseTask;
> }
> 
> + (WKProcessAssertionBackgroundTaskManager *)shared
>@@ -68,10 +74,15 @@ - (instancetype)init
>     _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,36 @@ - (void)_notifyAssertionsOfImminentSuspension
>         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);
>+}
>+
>+- (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 +156,7 @@ - (void)_updateBackgroundTask
>                 });
>             }
> 
>-            // 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 2528387ab3b7a226828030be75252d41ca838a0a..b3e233b98336d24bb3ebd1c6ddf6b6c139036ad1 100644
>--- a/Source/WebKit/WebProcess/WebProcess.cpp
>+++ b/Source/WebKit/WebProcess/WebProcess.cpp
>@@ -731,8 +731,6 @@ void WebProcess::didReceiveSyncMessage(IPC::Connection& connection, IPC::Decoder
> {
>     if (messageReceiverMap().dispatchSyncMessage(connection, decoder, replyEncoder))
>         return;
>-
>-    didReceiveSyncWebProcessMessage(connection, decoder, replyEncoder);
> }
> 
> void WebProcess::didReceiveMessage(IPC::Connection& connection, IPC::Decoder& decoder)
>@@ -1489,19 +1487,11 @@ void WebProcess::actualPrepareToSuspend(ShouldAcknowledgeWhenReadyToSuspend shou
>     });
> }
> 
>-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 30bd59a715297470d3e2932959957e6d4e2a60d4..179509661dc8de5de1400940191b19d6201c3f1a 100644
>--- a/Source/WebKit/WebProcess/WebProcess.h
>+++ b/Source/WebKit/WebProcess/WebProcess.h
>@@ -216,7 +216,7 @@ public:
> 
>     void setHiddenPageDOMTimerThrottlingIncreaseLimit(int milliseconds);
> 
>-    void processWillSuspendImminently(CompletionHandler<void(bool)>&&);
>+    void processWillSuspendImminently();
>     void prepareToSuspend();
>     void cancelPrepareToSuspend();
>     void processDidResume();
>@@ -418,7 +418,6 @@ private:
> 
>     // 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 384741211cd94c11b93d797aefcdb0e654f806d0..b39b38c183c66327d42e6edff176bba1c9cabb12 100644
>--- a/Source/WebKit/WebProcess/WebProcess.messages.in
>+++ b/Source/WebKit/WebProcess/WebProcess.messages.in
>@@ -96,7 +96,7 @@ messages -> WebProcess LegacyReceiver {
>     EnsureAutomationSessionProxy(String sessionIdentifier)
>     DestroyAutomationSessionProxy()
> 
>-    ProcessWillSuspendImminently() -> (bool handled) Synchronous
>+    ProcessWillSuspendImminently()
>     PrepareToSuspend()
>     CancelPrepareToSuspend()
>     ProcessDidResume()

Actions: View | Formatted Diff | Diff

Attachments on bug 197893: 369894 | 369964