Simplify "Unexpectedly Resumed" assertion handling
https://bugs.webkit.org/show_bug.cgi?id=203254
Reviewed by Geoffrey Garen.
When the WebContent process gets resumed from suspension, it now unconditionally takes a
process assertion on behalf on the UIProcess and sends a ProcessDidResume IPC to the
UIProcess. The UIProcess then sends a DidHandleProcessWasResumed IPC back after handing
the ProcessDidResume IPC allowing the WebContent process to release its assertion on
behalf on the UIProcess.
The previous code was racy because it relied on the m_processIsSuspended flag, which was
queried and set from different threads. Also, the 'unexpectedly resumed' naming was
confusing since we'd often take this assertion whenever the WebProcess got resumed,
wether unexpected or not, simply because the processTaskStateDidChange IPC won the race
with the ProcessDidResume IPC from the UIProcess.
* UIProcess/Cocoa/WebProcessProxyCocoa.mm:
(WebKit::WebProcessProxy::processWasResumed):
(WebKit::WebProcessProxy::processWasUnexpectedlyUnsuspended): Deleted.
* UIProcess/WebProcessProxy.h:
* UIProcess/WebProcessProxy.messages.in:
* WebProcess/WebProcess.cpp:
(WebKit::WebProcess::processDidResume):
* WebProcess/WebProcess.h:
* WebProcess/WebProcess.messages.in:
* WebProcess/cocoa/WebProcessCocoa.mm:
(WebKit::WebProcess::processTaskStateDidChange):
(WebKit::WebProcess::didHandleProcessWasResumed):
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@251443 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
index 3d6fb9b..c22b5bf 100644
--- a/Source/WebKit/ChangeLog
+++ b/Source/WebKit/ChangeLog
@@ -1,3 +1,35 @@
+2019-10-22 Chris Dumez <cdumez@apple.com>
+
+ Simplify "Unexpectedly Resumed" assertion handling
+ https://bugs.webkit.org/show_bug.cgi?id=203254
+
+ Reviewed by Geoffrey Garen.
+
+ When the WebContent process gets resumed from suspension, it now unconditionally takes a
+ process assertion on behalf on the UIProcess and sends a ProcessDidResume IPC to the
+ UIProcess. The UIProcess then sends a DidHandleProcessWasResumed IPC back after handing
+ the ProcessDidResume IPC allowing the WebContent process to release its assertion on
+ behalf on the UIProcess.
+
+ The previous code was racy because it relied on the m_processIsSuspended flag, which was
+ queried and set from different threads. Also, the 'unexpectedly resumed' naming was
+ confusing since we'd often take this assertion whenever the WebProcess got resumed,
+ wether unexpected or not, simply because the processTaskStateDidChange IPC won the race
+ with the ProcessDidResume IPC from the UIProcess.
+
+ * UIProcess/Cocoa/WebProcessProxyCocoa.mm:
+ (WebKit::WebProcessProxy::processWasResumed):
+ (WebKit::WebProcessProxy::processWasUnexpectedlyUnsuspended): Deleted.
+ * UIProcess/WebProcessProxy.h:
+ * UIProcess/WebProcessProxy.messages.in:
+ * WebProcess/WebProcess.cpp:
+ (WebKit::WebProcess::processDidResume):
+ * WebProcess/WebProcess.h:
+ * WebProcess/WebProcess.messages.in:
+ * WebProcess/cocoa/WebProcessCocoa.mm:
+ (WebKit::WebProcess::processTaskStateDidChange):
+ (WebKit::WebProcess::didHandleProcessWasResumed):
+
2019-10-22 youenn fablet <youenn@apple.com>
Remove mayHaveServiceWorkerRegisteredForOrigin
diff --git a/Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm b/Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm
index 88aa1ce..83435f9 100644
--- a/Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm
+++ b/Source/WebKit/UIProcess/Cocoa/WebProcessProxyCocoa.mm
@@ -37,6 +37,7 @@
#import "WebProcessPool.h"
#import <sys/sysctl.h>
#import <wtf/NeverDestroyed.h>
+#import <wtf/Scope.h>
#import <wtf/spi/darwin/SandboxSPI.h>
namespace WebKit {
@@ -190,19 +191,22 @@
#endif
#if PLATFORM(IOS_FAMILY)
-void WebProcessProxy::processWasUnexpectedlyUnsuspended()
+void WebProcessProxy::processWasResumed()
{
+ auto exitScope = makeScopeExit([this] {
+ send(Messages::WebProcess::ParentProcessDidHandleProcessWasResumed(), 0);
+ });
+
if (m_throttler.shouldBeRunnable()) {
- // The process becoming unsuspended was not unexpected; it likely was notified of its running state
- // before receiving a processDidResume() message from the UIProcess.
+ // The process becoming unsuspended was not unexpected.
return;
}
// The WebProcess was awakened by something other than the UIProcess. Take out an assertion for a
// limited duration to allow whatever task needs to be accomplished time to complete.
- RELEASE_LOG(ProcessSuspension, "%p - WebProcessProxy::processWasUnexpectedlyUnsuspended()", this);
+ RELEASE_LOG(ProcessSuspension, "%p - WebProcessProxy::processWasResumed() Process was unexpectedly resumed, starting background activity", this);
auto backgroundActivityTimeoutHandler = [activityToken = m_throttler.backgroundActivityToken(), weakThis = makeWeakPtr(this)] {
- RELEASE_LOG(ProcessSuspension, "%p - WebProcessProxy::processWasUnexpectedlyUnsuspended() - lambda, background activity timed out", weakThis.get());
+ RELEASE_LOG(ProcessSuspension, "%p - WebProcessProxy::processWasResumed() - lambda, background activity timed out", weakThis.get());
};
m_unexpectedActivityTimer = makeUnique<WebCore::DeferrableOneShotTimer>(WTFMove(backgroundActivityTimeoutHandler), unexpectedActivityDuration);
}
diff --git a/Source/WebKit/UIProcess/WebProcessProxy.h b/Source/WebKit/UIProcess/WebProcessProxy.h
index 8a47e813ff..967cdb7 100644
--- a/Source/WebKit/UIProcess/WebProcessProxy.h
+++ b/Source/WebKit/UIProcess/WebProcessProxy.h
@@ -326,7 +326,7 @@
#endif
#if PLATFORM(IOS_FAMILY)
- void processWasUnexpectedlyUnsuspended();
+ void processWasResumed();
#endif
void webPageMediaStateDidChange(WebPageProxy&);
diff --git a/Source/WebKit/UIProcess/WebProcessProxy.messages.in b/Source/WebKit/UIProcess/WebProcessProxy.messages.in
index e119e6c..3a183ad 100644
--- a/Source/WebKit/UIProcess/WebProcessProxy.messages.in
+++ b/Source/WebKit/UIProcess/WebProcessProxy.messages.in
@@ -71,7 +71,7 @@
#endif
#if PLATFORM(IOS_FAMILY)
- ProcessWasUnexpectedlyUnsuspended()
+ ProcessWasResumed()
#endif
# Plug-in messages.
diff --git a/Source/WebKit/WebProcess/WebProcess.cpp b/Source/WebKit/WebProcess/WebProcess.cpp
index b87e750..96ebc08 100644
--- a/Source/WebKit/WebProcess/WebProcess.cpp
+++ b/Source/WebKit/WebProcess/WebProcess.cpp
@@ -1539,13 +1539,6 @@
m_webSQLiteDatabaseTracker.setIsSuspended(false);
SQLiteDatabase::setIsDatabaseOpeningForbidden(false);
accessibilityProcessSuspendedNotification(false);
- {
- LockHolder holder(m_unexpectedlyResumedUIAssertionLock);
- if (m_unexpectedlyResumedUIAssertion) {
- RELEASE_LOG(ProcessSuspension, "%p - WebProcess::processDidResume() Releasing 'Unexpectedly resumed' assertion", this);
- m_unexpectedlyResumedUIAssertion = nullptr;
- }
- }
#endif
#if ENABLE(VIDEO)
diff --git a/Source/WebKit/WebProcess/WebProcess.h b/Source/WebKit/WebProcess/WebProcess.h
index a68ab63..8be7260 100644
--- a/Source/WebKit/WebProcess/WebProcess.h
+++ b/Source/WebKit/WebProcess/WebProcess.h
@@ -455,6 +455,7 @@
#if PLATFORM(IOS_FAMILY)
void processTaskStateDidChange(ProcessTaskStateObserver::TaskState) final;
+ void parentProcessDidHandleProcessWasResumed();
bool shouldFreezeOnSuspension() const;
void updateFreezerStatus();
#endif
@@ -535,8 +536,8 @@
#if PLATFORM(IOS_FAMILY)
WebSQLiteDatabaseTracker m_webSQLiteDatabaseTracker;
Ref<ProcessTaskStateObserver> m_taskStateObserver;
- Lock m_unexpectedlyResumedUIAssertionLock;
- RetainPtr<BKSProcessAssertion> m_unexpectedlyResumedUIAssertion;
+ Lock m_processWasResumedUIAssertionLock;
+ RetainPtr<BKSProcessAssertion> m_processWasResumedUIAssertion;
#endif
enum PageMarkingLayersAsVolatileCounterType { };
diff --git a/Source/WebKit/WebProcess/WebProcess.messages.in b/Source/WebKit/WebProcess/WebProcess.messages.in
index c7f3e0a..d88d6075 100644
--- a/Source/WebKit/WebProcess/WebProcess.messages.in
+++ b/Source/WebKit/WebProcess/WebProcess.messages.in
@@ -156,6 +156,7 @@
#if PLATFORM(IOS_FAMILY)
UnblockAccessibilityServer(WebKit::SandboxExtension::Handle handle)
+ ParentProcessDidHandleProcessWasResumed();
#endif
#if PLATFORM(GTK) || PLATFORM(WPE)
diff --git a/Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm b/Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm
index 51bc8a5..ca6f4e2 100644
--- a/Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm
+++ b/Source/WebKit/WebProcess/cocoa/WebProcessCocoa.mm
@@ -297,43 +297,41 @@
{
// NOTE: This will be called from a background thread.
RELEASE_LOG(ProcessSuspension, "%p - WebProcess::processTaskStateDidChange() - taskState(%d)", this, taskState);
- if (taskState == ProcessTaskStateObserver::None)
+ if (taskState != ProcessTaskStateObserver::Running)
return;
- if (taskState == ProcessTaskStateObserver::Suspended) {
- if (m_processIsSuspended)
- return;
-
- RELEASE_LOG(ProcessSuspension, "%p - WebProcess::processTaskStateChanged() - unexpectedly entered Suspended state", this);
- return;
- }
-
- if (!m_processIsSuspended)
- return;
-
- LockHolder holder(m_unexpectedlyResumedUIAssertionLock);
- if (m_unexpectedlyResumedUIAssertion)
+ LockHolder holder(m_processWasResumedUIAssertionLock);
+ if (m_processWasResumedUIAssertion)
return;
// We were awakened from suspension unexpectedly. Notify the WebProcessProxy, but take a process assertion on our parent PID
// to ensure that it too is awakened.
- RELEASE_LOG(ProcessSuspension, "%p - WebProcess::processTaskStateChanged() Taking 'Unexpectedly resumed' assertion", this);
- m_unexpectedlyResumedUIAssertion = adoptNS([[BKSProcessAssertion alloc] initWithPID:parentProcessConnection()->remoteProcessID() flags:BKSProcessAssertionPreventTaskSuspend reason:BKSProcessAssertionReasonFinishTask name:@"Unexpectedly resumed" withHandler:nil]);
+ RELEASE_LOG(ProcessSuspension, "%p - WebProcess::processTaskStateChanged() Taking 'WebProcess was resumed' assertion on behalf on UIProcess", this);
+ m_processWasResumedUIAssertion = adoptNS([[BKSProcessAssertion alloc] initWithPID:parentProcessConnection()->remoteProcessID() flags:BKSProcessAssertionPreventTaskSuspend reason:BKSProcessAssertionReasonFinishTask name:@"WebProcess was resumed" withHandler:nil]);
- auto releaseAssertion = [this] {
- LockHolder holder(m_unexpectedlyResumedUIAssertionLock);
- if (!m_unexpectedlyResumedUIAssertion)
- return;
-
- RELEASE_LOG(ProcessSuspension, "%p - WebProcess::processTaskStateChanged() Releasing 'Unexpectedly resumed' assertion due to time out", this);
- [m_unexpectedlyResumedUIAssertion invalidate];
- m_unexpectedlyResumedUIAssertion = nullptr;
+ m_processWasResumedUIAssertion.get().invalidationHandler = [this] {
+ LockHolder holder(m_processWasResumedUIAssertionLock);
+ RELEASE_LOG(ProcessSuspension, "%p - WebProcess::processTaskStateChanged() Releasing 'WebProcess was resumed' assertion on behalf on UIProcess due invalidation", this);
+ [m_processWasResumedUIAssertion invalidate];
+ m_processWasResumedUIAssertion = nullptr;
};
- m_unexpectedlyResumedUIAssertion.get().invalidationHandler = releaseAssertion;
- parentProcessConnection()->send(Messages::WebProcessProxy::ProcessWasUnexpectedlyUnsuspended(), 0);
- dispatch_after(dispatch_time(DISPATCH_TIME_NOW, (int64_t)(5 * NSEC_PER_SEC)), dispatch_get_global_queue(DISPATCH_QUEUE_PRIORITY_DEFAULT, 0), releaseAssertion);
+ // This will cause the parent process to send a ParentProcessDidHandleProcessWasResumed IPC back, so that we can release our assertion on its behalf.
+ // We are not using sendWithAsyncReply() because it would not be thread-safe.
+ parentProcessConnection()->send(Messages::WebProcessProxy::ProcessWasResumed(), 0);
}
+
+void WebProcess::parentProcessDidHandleProcessWasResumed()
+{
+ LockHolder holder(m_processWasResumedUIAssertionLock);
+ ASSERT(m_processWasResumedUIAssertion);
+
+ RELEASE_LOG(ProcessSuspension, "%p - WebProcess::parentProcessDidHandleProcessWasResumed() Releasing 'WebProcess was resumed' assertion on behalf on UIProcess", this);
+
+ [m_processWasResumedUIAssertion invalidate];
+ m_processWasResumedUIAssertion = nullptr;
+}
+
#endif
#if PLATFORM(IOS_FAMILY)
