WebKit Bugzilla
Attachment 369964 Details for
Bug 197893
: [WK2][iOS] UIProcess may get killed because it is taking too long to release its background task after expiration
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-197893-20190515101355.patch (text/plain), 14.87 KB, created by
Chris Dumez
on 2019-05-15 10:13:55 PDT
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Chris Dumez
Created:
2019-05-15 10:13:55 PDT
Size:
14.87 KB
patch
obsolete
>Subversion Revision: 245326 >diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog >index b25a8b549bd0b2dd3fc1a5ba0914c60becb9192c..01ef436edfdcdfdd903afde9e62d2f73c8efef7e 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 Alex Christensen <achristensen@webkit.org> > > Allow NSFileCoordinator to be called from WebContent process >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 d642781c35f0a6ee57ee04e223bc5ee1333988c3..62934c626abd202b064f316ffcdcbf9baef5d0d9 100644 >--- a/Source/WebKit/UIProcess/WebProcessProxy.cpp >+++ b/Source/WebKit/UIProcess/WebProcessProxy.cpp >@@ -1137,11 +1137,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..2aca868bb8b5713159c91d2abe5feca9461cc72f 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,40 @@ - (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); >+#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 @@ - (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 97b8305880eebf442ead494cb5c3e5017a2f0e04..d21bbf67fd276563011d65ceb5d876f2768c1621 100644 >--- a/Source/WebKit/WebProcess/WebProcess.cpp >+++ b/Source/WebKit/WebProcess/WebProcess.cpp >@@ -732,8 +732,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) >@@ -1490,19 +1488,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 4b6ddc693476833dfb3da1025b25afb0911d341f..96d3ce953d30d4e43f546755941988c68c2db23c 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(); >@@ -420,7 +420,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()
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 197893
:
369894
| 369964