- Revision
- 280110
- Author
- [email protected]
- Date
- 2021-07-20 16:08:27 -0700 (Tue, 20 Jul 2021)
Log Message
Completion handler of NetworkProcess::prepareToSuspend may not be invoked promptly
https://bugs.webkit.org/show_bug.cgi?id=228119
Reviewed by Chris Dumez.
WebResourceLoadStatisticsStore and StorageManagerSet dispatch task to suspend background thread on suspension.
When the task is finished on background thread, it dispatches a reply task to main thread. When all replies are
received, network process replies prepareToSuspend message. With our current implementation, if network process
receives messages in order { PrepareToSuspend, ProcessDidResume, PrepareToSuspend },
WebResourceLoadStatisticsStore and StorageManagerSet may dispatch two suspend tasks to background thread and
get suspended in the first task. In this case, the second PrepareToSuspend message will not be replied, and
UI process will be waiting on reply for the latest PrepareToSuspend message to release assertion. To solve this,
background thread should only execute latest suspend task by checking task identifier.
* NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:
(WebKit::WTF_GUARDED_BY_LOCK):
(WebKit::WebResourceLoadStatisticsStore::resourceLoadStatisticsUpdated):
(WebKit::WebResourceLoadStatisticsStore::suspend):
(WebKit::WebResourceLoadStatisticsStore::resume):
(): Deleted.
* NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:
* NetworkProcess/NetworkProcess.cpp:
(WebKit::NetworkProcess::suspendIDBServers):
* NetworkProcess/NetworkProcess.h:
* NetworkProcess/WebStorage/StorageManagerSet.cpp:
(WebKit::StorageManagerSet::suspend):
(WebKit::StorageManagerSet::resume):
* NetworkProcess/WebStorage/StorageManagerSet.h:
(WebKit::StorageManagerSet::WTF_GUARDED_BY_LOCK):
(): Deleted.
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (280109 => 280110)
--- trunk/Source/WebKit/ChangeLog 2021-07-20 22:55:56 UTC (rev 280109)
+++ trunk/Source/WebKit/ChangeLog 2021-07-20 23:08:27 UTC (rev 280110)
@@ -1,3 +1,36 @@
+2021-07-20 Sihui Liu <[email protected]>
+
+ Completion handler of NetworkProcess::prepareToSuspend may not be invoked promptly
+ https://bugs.webkit.org/show_bug.cgi?id=228119
+
+ Reviewed by Chris Dumez.
+
+ WebResourceLoadStatisticsStore and StorageManagerSet dispatch task to suspend background thread on suspension.
+ When the task is finished on background thread, it dispatches a reply task to main thread. When all replies are
+ received, network process replies prepareToSuspend message. With our current implementation, if network process
+ receives messages in order { PrepareToSuspend, ProcessDidResume, PrepareToSuspend },
+ WebResourceLoadStatisticsStore and StorageManagerSet may dispatch two suspend tasks to background thread and
+ get suspended in the first task. In this case, the second PrepareToSuspend message will not be replied, and
+ UI process will be waiting on reply for the latest PrepareToSuspend message to release assertion. To solve this,
+ background thread should only execute latest suspend task by checking task identifier.
+
+ * NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:
+ (WebKit::WTF_GUARDED_BY_LOCK):
+ (WebKit::WebResourceLoadStatisticsStore::resourceLoadStatisticsUpdated):
+ (WebKit::WebResourceLoadStatisticsStore::suspend):
+ (WebKit::WebResourceLoadStatisticsStore::resume):
+ (): Deleted.
+ * NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:
+ * NetworkProcess/NetworkProcess.cpp:
+ (WebKit::NetworkProcess::suspendIDBServers):
+ * NetworkProcess/NetworkProcess.h:
+ * NetworkProcess/WebStorage/StorageManagerSet.cpp:
+ (WebKit::StorageManagerSet::suspend):
+ (WebKit::StorageManagerSet::resume):
+ * NetworkProcess/WebStorage/StorageManagerSet.h:
+ (WebKit::StorageManagerSet::WTF_GUARDED_BY_LOCK):
+ (): Deleted.
+
2021-07-20 Brent Fulgham <[email protected]>
[Cocoa] Silence telemetry on MSC_mach_wait_until
Modified: trunk/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp (280109 => 280110)
--- trunk/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp 2021-07-20 22:55:56 UTC (rev 280109)
+++ trunk/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp 2021-07-20 23:08:27 UTC (rev 280110)
@@ -59,9 +59,10 @@
namespace WebKit {
using namespace WebCore;
-WebResourceLoadStatisticsStore::State WebResourceLoadStatisticsStore::suspendedState { WebResourceLoadStatisticsStore::State::Running };
-Lock WebResourceLoadStatisticsStore::suspendedStateLock;
-Condition WebResourceLoadStatisticsStore::suspendedStateChangeCondition;
+static Lock globalSuspensionLock;
+static Condition globalSuspensionCondition;
+static bool globalShouldSuspend WTF_GUARDED_BY_LOCK(globalSuspensionLock) { false };
+static uint64_t globalSuspensionIdentifier WTF_GUARDED_BY_LOCK(globalSuspensionLock) { 0 };
const OptionSet<WebsiteDataType>& WebResourceLoadStatisticsStore::monitoredDataTypes()
{
@@ -354,13 +355,6 @@
return;
}
-#if ASSERT_ENABLED
- {
- Locker stateLocker { suspendedStateLock };
- ASSERT(suspendedState != State::Suspended);
- }
-#endif
-
m_statisticsStore->mergeStatistics(WTFMove(statistics));
postTaskReply(WTFMove(completionHandler));
// We can cancel any pending request to process statistics since we're doing it synchronously below.
@@ -1454,17 +1448,12 @@
void WebResourceLoadStatisticsStore::suspend(CompletionHandler<void()>&& completionHandler)
{
- CompletionHandlerCallingScope completionHandlerCaller(WTFMove(completionHandler));
- Locker stateLocker { suspendedStateLock };
- if (suspendedState != State::Running)
- return;
- suspendedState = State::WillSuspend;
+ Locker suspensionLocker { globalSuspensionLock };
+ globalShouldSuspend = true;
- sharedStatisticsQueue()->dispatch([completionHandler = completionHandlerCaller.release()] () mutable {
- Locker stateLocker { suspendedStateLock };
- ASSERT(suspendedState != State::Suspended);
-
- if (suspendedState != State::WillSuspend) {
+ sharedStatisticsQueue()->dispatch([suspensionIdentifier = ++globalSuspensionIdentifier, completionHandler = WTFMove(completionHandler)] () mutable {
+ Locker suspensionLocker { globalSuspensionLock };
+ if (!globalShouldSuspend || suspensionIdentifier != globalSuspensionIdentifier) {
postTaskReply(WTFMove(completionHandler));
return;
}
@@ -1472,22 +1461,20 @@
for (auto& databaseStore : ResourceLoadStatisticsDatabaseStore::allStores())
databaseStore->interrupt();
- suspendedState = State::Suspended;
postTaskReply(WTFMove(completionHandler));
- while (suspendedState == State::Suspended)
- suspendedStateChangeCondition.wait(suspendedStateLock);
- ASSERT(suspendedState != State::Suspended);
+ while (globalShouldSuspend)
+ globalSuspensionCondition.wait(globalSuspensionLock);
});
}
void WebResourceLoadStatisticsStore::resume()
{
- Locker stateLocker { suspendedStateLock };
- auto previousState = suspendedState;
- suspendedState = State::Running;
- if (previousState == State::Suspended)
- suspendedStateChangeCondition.notifyOne();
+ ASSERT(RunLoop::isMain());
+
+ Locker suspensionLocker { globalSuspensionLock };
+ globalShouldSuspend = false;
+ globalSuspensionCondition.notifyOne();
}
void WebResourceLoadStatisticsStore::insertExpiredStatisticForTesting(const RegistrableDomain& domain, unsigned numberOfOperatingDaysPassed, bool hadUserInteraction, bool isScheduledForAllButCookieDataRemoval, bool isPrevalent, CompletionHandler<void()>&& completionHandler)
Modified: trunk/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h (280109 => 280110)
--- trunk/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h 2021-07-20 22:55:56 UTC (rev 280109)
+++ trunk/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h 2021-07-20 23:08:27 UTC (rev 280110)
@@ -290,15 +290,6 @@
bool m_hasScheduledProcessStats { false };
bool m_firstNetworkProcessCreated { false };
-
- enum class State {
- Running,
- WillSuspend,
- Suspended
- };
- static Lock suspendedStateLock;
- static State suspendedState WTF_GUARDED_BY_LOCK(suspendedStateLock);
- static Condition suspendedStateChangeCondition;
};
} // namespace WebKit
Modified: trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp (280109 => 280110)
--- trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp 2021-07-20 22:55:56 UTC (rev 280109)
+++ trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp 2021-07-20 23:08:27 UTC (rev 280110)
@@ -2214,6 +2214,7 @@
void NetworkProcess::suspendIDBServers(bool isSuspensionImminent)
{
m_shouldSuspendIDBServers = true;
+ ++m_suspensionIdentifier;
bool allSuspended = true;
auto condition = isSuspensionImminent ? WebIDBServer::SuspensionCondition::Always : WebIDBServer::SuspensionCondition::IfIdle;
@@ -2223,13 +2224,13 @@
if (allSuspended)
return;
- RunLoop::main().dispatchAfter(5_s, [this, weakThis = makeWeakPtr(*this)] {
+ RunLoop::main().dispatchAfter(5_s, [this, weakThis = makeWeakPtr(*this), suspensionIdentifier = m_suspensionIdentifier] {
if (!weakThis)
return;
- if (!m_shouldSuspendIDBServers)
+ if (!m_shouldSuspendIDBServers || suspensionIdentifier != m_suspensionIdentifier)
return;
-
+
for (auto& server : m_webIDBServers.values())
server->suspend(WebIDBServer::SuspensionCondition::Always);
});
Modified: trunk/Source/WebKit/NetworkProcess/NetworkProcess.h (280109 => 280110)
--- trunk/Source/WebKit/NetworkProcess/NetworkProcess.h 2021-07-20 22:55:56 UTC (rev 280109)
+++ trunk/Source/WebKit/NetworkProcess/NetworkProcess.h 2021-07-20 23:08:27 UTC (rev 280110)
@@ -585,6 +585,7 @@
HashMap<PAL::SessionID, String> m_idbDatabasePaths;
HashMap<PAL::SessionID, RefPtr<WebIDBServer>> m_webIDBServers;
bool m_shouldSuspendIDBServers { false };
+ uint64_t m_suspensionIdentifier { 0 };
#if ENABLE(SERVICE_WORKER)
struct ServiceWorkerInfo {
Modified: trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp (280109 => 280110)
--- trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp 2021-07-20 22:55:56 UTC (rev 280109)
+++ trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp 2021-07-20 23:08:27 UTC (rev 280110)
@@ -177,17 +177,12 @@
{
ASSERT(RunLoop::isMain());
- CompletionHandlerCallingScope completionHandlerCaller(WTFMove(completionHandler));
- Locker stateLocker { m_stateLock };
- if (m_state != State::Running)
- return;
- m_state = State::WillSuspend;
+ Locker suspensionLocker { m_suspensionLock };
+ m_shouldSuspend = true;
- m_queue->dispatch([this, protectedThis = makeRef(*this), completionHandler = completionHandlerCaller.release()] () mutable {
- Locker stateLocker { m_stateLock };
- ASSERT(m_state != State::Suspended);
-
- if (m_state != State::WillSuspend) {
+ m_queue->dispatch([this, protectedThis = makeRef(*this), suspensionIdentifier = ++m_suspensionIdentifier, completionHandler = WTFMove(completionHandler)] () mutable {
+ Locker suspensionLocker { m_suspensionLock };
+ if (!m_shouldSuspend || suspensionIdentifier != m_suspensionIdentifier) {
RunLoop::main().dispatch(WTFMove(completionHandler));
return;
}
@@ -196,12 +191,10 @@
// SQL transaction has been committed.
flushLocalStorage();
- m_state = State::Suspended;
RunLoop::main().dispatch(WTFMove(completionHandler));
- while (m_state == State::Suspended)
- m_stateChangeCondition.wait(m_stateLock);
- ASSERT(m_state == State::Running);
+ while (m_shouldSuspend)
+ m_suspensionCondition.wait(m_suspensionLock);
});
}
@@ -209,11 +202,9 @@
{
ASSERT(RunLoop::isMain());
- Locker stateLocker { m_stateLock };
- auto previousState = m_state;
- m_state = State::Running;
- if (previousState == State::Suspended)
- m_stateChangeCondition.notifyOne();
+ Locker suspensionLocker { m_suspensionLock };
+ m_shouldSuspend = false;
+ m_suspensionCondition.notifyOne();
}
void StorageManagerSet::getSessionStorageOrigins(PAL::SessionID sessionID, GetOriginsCallback&& completionHandler)
Modified: trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.h (280109 => 280110)
--- trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.h 2021-07-20 22:55:56 UTC (rev 280109)
+++ trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.h 2021-07-20 23:08:27 UTC (rev 280110)
@@ -100,14 +100,10 @@
HashSet<IPC::Connection::UniqueID> m_connections;
Ref<WorkQueue> m_queue;
- enum class State {
- Running,
- WillSuspend,
- Suspended
- };
- State m_state { State::Running };
- Lock m_stateLock;
- Condition m_stateChangeCondition;
+ Lock m_suspensionLock;
+ Condition m_suspensionCondition;
+ bool m_shouldSuspend WTF_GUARDED_BY_LOCK(m_suspensionLock) { false };
+ uint64_t m_suspensionIdentifier WTF_GUARDED_BY_LOCK(m_suspensionLock) { 0 };
};
} // namespace WebKit
