Diff
Modified: trunk/Source/WebKit/ChangeLog (279513 => 279514)
--- trunk/Source/WebKit/ChangeLog 2021-07-02 20:33:30 UTC (rev 279513)
+++ trunk/Source/WebKit/ChangeLog 2021-07-02 20:35:05 UTC (rev 279514)
@@ -1,5 +1,68 @@
2021-07-02 Chris Dumez <cdu...@apple.com>
+ Take a process assertion in the network process when holding locked files
+ https://bugs.webkit.org/show_bug.cgi?id=227552
+
+ Reviewed by Sihui Liu.
+
+ Take a process assertion in the network process when holding locked files. Previously, we'd
+ send an IPC to the UIProcess and the UIProcess would take the assertion on behalf of the
+ network process. However, our previous approach was racy and could cause the network process
+ to still get suspended while still holding locked files (which would get it killed).
+
+ We have evidence that the network process is getting killed a lot for this reason
+ (rdar://79787123).
+
+ * NetworkProcess/NetworkProcess.cpp:
+ (WebKit::NetworkProcess::NetworkProcess):
+ Call setIsHoldingLockedFiles() when the "isHoldingLockedFiles" state changes instead of sending
+ an IPC to the UIProcess. setIsHoldingLockedFiles() takes care of taking or releasing a
+ RunningBoard assertion.
+
+ (WebKit::NetworkProcess::prepareToSuspend):
+ (WebKit::NetworkProcess::resume):
+ Stop calling setSuspended() on the WebSQLiteDatabaseTracker. The purpose was to avoid sending
+ the SetIsHoldingLockedFiles IPC to the UIProcess after receiving the PrepareToSuspend IPC.
+ However, it is now acceptable to take a process assertion after PrepareToSuspend IPC has been
+ received.
+
+ * NetworkProcess/NetworkProcess.h:
+
+ * NetworkProcess/ios/NetworkProcessIOS.mm:
+ (WebKit::NetworkProcess::setIsHoldingLockedFiles):
+ When setIsHoldingLockedFiles(true) gets called, we take a runningboard assertion to prevent
+ suspension. Note that setIsHoldingLockedFiles(true) gets on the storage thread (which started
+ the SQLite transaction) so we take the assertion synchronously to make sure we cannot get
+ suspended during a SQLite transaction.
+ When setIsHoldingLockedFiles(false) gets called, we release the runningboard assertion.
+
+ * Shared/WebSQLiteDatabaseTracker.cpp:
+ (WebKit::WebSQLiteDatabaseTracker::WebSQLiteDatabaseTracker):
+ (WebKit::WebSQLiteDatabaseTracker::~WebSQLiteDatabaseTracker):
+ (WebKit::WebSQLiteDatabaseTracker::setIsSuspended):
+ (WebKit::WebSQLiteDatabaseTracker::willBeginFirstTransaction):
+ (WebKit::WebSQLiteDatabaseTracker::didFinishLastTransaction):
+ (WebKit::WebSQLiteDatabaseTracker::setIsHoldingLockedFiles):
+ * Shared/WebSQLiteDatabaseTracker.h:
+ Update WebSQLiteDatabaseTracker so that it called its isHoldingLockedFilesHandler lambda
+ directly on the storage thread when the first transaction starts, instead of dispatching
+ to the main thread. Dispatching to the main thread would be racy as it would mean taking
+ the process assertion asynchronously (on the main thread), after the SQLite transaction
+ has begun on the storage thread.
+ As a result, of this change, I am not using the HysterisActivity class anymore and using
+ my own thread-safe hysteresis activity using RunLoop::dispatchAfter() with an integer
+ and a lock.
+
+ * UIProcess/Network/NetworkProcessProxy.cpp:
+ (WebKit::NetworkProcessProxy::networkProcessDidTerminate):
+ (WebKit::NetworkProcessProxy::sendProcessDidResume):
+ * UIProcess/Network/NetworkProcessProxy.h:
+ * UIProcess/Network/NetworkProcessProxy.messages.in:
+ Drop the SetIsHoldingLockedFiles IPC and its handling on UIProcess side since the network
+ process now takes the assertion by itself.
+
+2021-07-02 Chris Dumez <cdu...@apple.com>
+
Regression(r278786) LocalStorageDatabase's transaction may be remain active when process gets suspended
https://bugs.webkit.org/show_bug.cgi?id=227632
Modified: trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp (279513 => 279514)
--- trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp 2021-07-02 20:33:30 UTC (rev 279513)
+++ trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp 2021-07-02 20:35:05 UTC (rev 279514)
@@ -159,7 +159,7 @@
, m_networkContentRuleListManager(*this)
#endif
#if PLATFORM(IOS_FAMILY)
- , m_webSQLiteDatabaseTracker([this](bool isHoldingLockedFiles) { parentProcessConnection()->send(Messages::NetworkProcessProxy::SetIsHoldingLockedFiles(isHoldingLockedFiles), 0); })
+ , m_webSQLiteDatabaseTracker([this](bool isHoldingLockedFiles) { setIsHoldingLockedFiles(isHoldingLockedFiles); })
#endif
, m_messagePortChannelRegistry(createMessagePortChannelRegistry(*this))
{
@@ -2219,10 +2219,6 @@
server->suspend();
#endif
-#if PLATFORM(IOS_FAMILY)
- m_webSQLiteDatabaseTracker.setIsSuspended(true);
-#endif
-
lowMemoryHandler(Critical::Yes);
RefPtr<CallbackAggregator> callbackAggregator = CallbackAggregator::create([this, completionHandler = WTFMove(completionHandler)]() mutable {
@@ -2270,10 +2266,6 @@
void NetworkProcess::resume()
{
-#if PLATFORM(IOS_FAMILY)
- m_webSQLiteDatabaseTracker.setIsSuspended(false);
-#endif
-
for (auto& connection : m_webProcessConnections.values())
connection->endSuspension();
Modified: trunk/Source/WebKit/NetworkProcess/NetworkProcess.h (279513 => 279514)
--- trunk/Source/WebKit/NetworkProcess/NetworkProcess.h 2021-07-02 20:33:30 UTC (rev 279513)
+++ trunk/Source/WebKit/NetworkProcess/NetworkProcess.h 2021-07-02 20:35:05 UTC (rev 279514)
@@ -105,6 +105,7 @@
class NetworkProcessSupplement;
class NetworkProximityManager;
class NetworkResourceLoader;
+class ProcessAssertion;
class StorageManagerSet;
class WebPageNetworkParameters;
class WebSWServerConnection;
@@ -497,6 +498,10 @@
void addServiceWorkerSession(PAL::SessionID, bool processTerminationDelayEnabled, String&& serviceWorkerRegistrationDirectory, const SandboxExtension::Handle&);
#endif
+#if PLATFORM(IOS_FAMILY)
+ void setIsHoldingLockedFiles(bool);
+#endif
+
void firePrivateClickMeasurementTimerImmediately(PAL::SessionID);
class SessionStorageQuotaManager {
@@ -572,6 +577,7 @@
#if PLATFORM(IOS_FAMILY)
WebSQLiteDatabaseTracker m_webSQLiteDatabaseTracker;
+ std::unique_ptr<ProcessAssertion> m_holdingLockedFileAssertion;
#endif
HashMap<PAL::SessionID, String> m_idbDatabasePaths;
Modified: trunk/Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm (279513 => 279514)
--- trunk/Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm 2021-07-02 20:33:30 UTC (rev 279513)
+++ trunk/Source/WebKit/NetworkProcess/ios/NetworkProcessIOS.mm 2021-07-02 20:35:05 UTC (rev 279514)
@@ -30,6 +30,7 @@
#import "NetworkCache.h"
#import "NetworkProcessCreationParameters.h"
+#import "ProcessAssertion.h"
#import "SandboxInitializationParameters.h"
#import "SecItemShim.h"
#import <WebCore/CertificateInfo.h>
@@ -96,6 +97,21 @@
completionHandler();
}
+void NetworkProcess::setIsHoldingLockedFiles(bool isHoldingLockedFiles)
+{
+ if (!isHoldingLockedFiles) {
+ m_holdingLockedFileAssertion = nullptr;
+ return;
+ }
+
+ if (m_holdingLockedFileAssertion && m_holdingLockedFileAssertion->isValid())
+ return;
+
+ // We synchronously take a process assertion when beginning a SQLite transaction so that we don't get suspended
+ // while holding a locked file. We would get killed if suspended while holding locked files.
+ m_holdingLockedFileAssertion = makeUnique<ProcessAssertion>(getCurrentProcessID(), "Network Process is holding locked files"_s, ProcessAssertionType::FinishTaskUninterruptable);
+}
+
} // namespace WebKit
#endif // PLATFORM(IOS_FAMILY)
Modified: trunk/Source/WebKit/Shared/WebSQLiteDatabaseTracker.cpp (279513 => 279514)
--- trunk/Source/WebKit/Shared/WebSQLiteDatabaseTracker.cpp 2021-07-02 20:33:30 UTC (rev 279513)
+++ trunk/Source/WebKit/Shared/WebSQLiteDatabaseTracker.cpp 2021-07-02 20:35:05 UTC (rev 279514)
@@ -26,10 +26,6 @@
#include "config.h"
#include "WebSQLiteDatabaseTracker.h"
-#include "NetworkProcess.h"
-#include "NetworkProcessProxyMessages.h"
-#include "WebProcess.h"
-#include "WebProcessProxyMessages.h"
#include <WebCore/SQLiteDatabaseTracker.h>
namespace WebKit {
@@ -37,7 +33,6 @@
WebSQLiteDatabaseTracker::WebSQLiteDatabaseTracker(IsHoldingLockedFilesHandler&& isHoldingLockedFilesHandler)
: m_isHoldingLockedFilesHandler(WTFMove(isHoldingLockedFilesHandler))
- , m_hysteresis([this](PAL::HysteresisState state) { setIsHoldingLockedFiles(state == PAL::HysteresisState::Started); }, 1_s)
{
ASSERT(RunLoop::isMain());
SQLiteDatabaseTracker::setClient(this);
@@ -48,7 +43,7 @@
ASSERT(RunLoop::isMain());
SQLiteDatabaseTracker::setClient(nullptr);
- if (m_hysteresis.state() == PAL::HysteresisState::Started)
+ if (m_currentHystererisID)
setIsHoldingLockedFiles(false);
}
@@ -55,29 +50,41 @@
void WebSQLiteDatabaseTracker::setIsSuspended(bool isSuspended)
{
ASSERT(RunLoop::isMain());
+
+ Locker locker { m_lock };
m_isSuspended = isSuspended;
}
void WebSQLiteDatabaseTracker::willBeginFirstTransaction()
{
- RunLoop::main().dispatch([weakThis = makeWeakPtr(*this)] {
- if (weakThis)
- weakThis->m_hysteresis.start();
- });
+ Locker locker { m_lock };
+ if (m_currentHystererisID) {
+ // Cancel previous hysteresis task.
+ ++m_currentHystererisID;
+ return;
+ }
+
+ setIsHoldingLockedFiles(true);
}
void WebSQLiteDatabaseTracker::didFinishLastTransaction()
{
- RunLoop::main().dispatch([weakThis = makeWeakPtr(*this)] {
- if (weakThis)
- weakThis->m_hysteresis.stop();
+ Locker locker { m_lock };
+ RunLoop::main().dispatchAfter(1_s, [this, weakThis = makeWeakPtr(*this), hystererisID = ++m_currentHystererisID] {
+ if (!weakThis)
+ return;
+
+ Locker locker { m_lock };
+ if (m_currentHystererisID != hystererisID)
+ return; // Cancelled.
+
+ m_currentHystererisID = 0;
+ setIsHoldingLockedFiles(false);
});
}
void WebSQLiteDatabaseTracker::setIsHoldingLockedFiles(bool isHoldingLockedFiles)
{
- ASSERT(RunLoop::isMain());
-
if (m_isSuspended)
return;
Modified: trunk/Source/WebKit/Shared/WebSQLiteDatabaseTracker.h (279513 => 279514)
--- trunk/Source/WebKit/Shared/WebSQLiteDatabaseTracker.h 2021-07-02 20:33:30 UTC (rev 279513)
+++ trunk/Source/WebKit/Shared/WebSQLiteDatabaseTracker.h 2021-07-02 20:35:05 UTC (rev 279514)
@@ -26,8 +26,10 @@
#pragma once
#include <WebCore/SQLiteDatabaseTrackerClient.h>
-#include <pal/HysteresisActivity.h>
+#include <wtf/Forward.h>
+#include <wtf/Lock.h>
#include <wtf/Noncopyable.h>
+#include <wtf/RunLoop.h>
#include <wtf/WeakPtr.h>
namespace WebKit {
@@ -36,6 +38,7 @@
class WebSQLiteDatabaseTracker final : public WebCore::SQLiteDatabaseTrackerClient, public CanMakeWeakPtr<WebSQLiteDatabaseTracker, WeakPtrFactoryInitialization::Eager> {
WTF_MAKE_NONCOPYABLE(WebSQLiteDatabaseTracker)
public:
+ // IsHoldingLockedFilesHandler may get called on a non-main thread, but while holding a Lock.
using IsHoldingLockedFilesHandler = Function<void(bool)>;
explicit WebSQLiteDatabaseTracker(IsHoldingLockedFilesHandler&&);
@@ -44,7 +47,7 @@
void setIsSuspended(bool);
private:
- void setIsHoldingLockedFiles(bool);
+ void setIsHoldingLockedFiles(bool) WTF_REQUIRES_LOCK(m_lock);
// WebCore::SQLiteDatabaseTrackerClient.
void willBeginFirstTransaction() final;
@@ -51,8 +54,9 @@
void didFinishLastTransaction() final;
IsHoldingLockedFilesHandler m_isHoldingLockedFilesHandler;
- PAL::HysteresisActivity m_hysteresis;
- bool m_isSuspended { false };
+ Lock m_lock;
+ uint64_t m_currentHystererisID WTF_GUARDED_BY_LOCK(m_lock) { 0 };
+ bool m_isSuspended WTF_GUARDED_BY_LOCK(m_lock) { false };
};
} // namespace WebKit
Modified: trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp (279513 => 279514)
--- trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp 2021-07-02 20:33:30 UTC (rev 279513)
+++ trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp 2021-07-02 20:35:05 UTC (rev 279514)
@@ -316,8 +316,6 @@
m_customProtocolManagerProxy.invalidate();
#endif
- m_activityForHoldingLockedFiles = nullptr;
-
m_uploadActivity = std::nullopt;
if (defaultNetworkProcess() == this)
@@ -1256,19 +1254,6 @@
if (canSendMessage())
send(Messages::NetworkProcess::ProcessDidResume(), 0);
}
-
-void NetworkProcessProxy::setIsHoldingLockedFiles(bool isHoldingLockedFiles)
-{
- if (!isHoldingLockedFiles) {
- RELEASE_LOG(ProcessSuspension, "UIProcess is releasing a background assertion because the Network process is no longer holding locked files");
- m_activityForHoldingLockedFiles = nullptr;
- return;
- }
- if (!m_activityForHoldingLockedFiles) {
- RELEASE_LOG(ProcessSuspension, "UIProcess is taking a background assertion because the Network process is holding locked files");
- m_activityForHoldingLockedFiles = m_throttler.backgroundActivity("Holding locked files"_s).moveToUniquePtr();
- }
-}
void NetworkProcessProxy::flushCookies(PAL::SessionID sessionID, CompletionHandler<void()>&& completionHandler)
{
Modified: trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h (279513 => 279514)
--- trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h 2021-07-02 20:33:30 UTC (rev 279513)
+++ trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h 2021-07-02 20:35:05 UTC (rev 279514)
@@ -208,8 +208,6 @@
void synthesizeAppIsBackground(bool background);
- void setIsHoldingLockedFiles(bool);
-
void flushCookies(PAL::SessionID, CompletionHandler<void()>&&);
void testProcessIncomingSyncMessagesWhenWaitingForSyncReply(WebPageProxyIdentifier, Messages::NetworkProcessProxy::TestProcessIncomingSyncMessagesWhenWaitingForSyncReplyDelayedReply&&);
@@ -339,7 +337,6 @@
#endif
ProcessThrottler m_throttler;
- std::unique_ptr<ProcessThrottler::BackgroundActivity> m_activityForHoldingLockedFiles;
ProcessThrottler::ActivityVariant m_activityFromWebProcesses;
#if ENABLE(CONTENT_EXTENSIONS)
Modified: trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.messages.in (279513 => 279514)
--- trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.messages.in 2021-07-02 20:33:30 UTC (rev 279513)
+++ trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.messages.in 2021-07-02 20:35:05 UTC (rev 279514)
@@ -28,8 +28,6 @@
TestProcessIncomingSyncMessagesWhenWaitingForSyncReply(WebKit::WebPageProxyIdentifier pageID) -> (bool handled) Synchronous
TerminateUnresponsiveServiceWorkerProcesses(WebCore::ProcessIdentifier processIdentifier)
- SetIsHoldingLockedFiles(bool isHoldingLockedFiles)
-
# Diagnostic messages logging
LogDiagnosticMessage(WebKit::WebPageProxyIdentifier pageID, String message, String description, enum:bool WebCore::ShouldSample shouldSample)
LogDiagnosticMessageWithResult(WebKit::WebPageProxyIdentifier pageID, String message, String description, uint32_t result, enum:bool WebCore::ShouldSample shouldSample)
Modified: trunk/Source/WebKit/UIProcess/ProcessAssertion.cpp (279513 => 279514)
--- trunk/Source/WebKit/UIProcess/ProcessAssertion.cpp 2021-07-02 20:33:30 UTC (rev 279513)
+++ trunk/Source/WebKit/UIProcess/ProcessAssertion.cpp 2021-07-02 20:35:05 UTC (rev 279514)
@@ -32,9 +32,10 @@
namespace WebKit {
-ProcessAssertion::ProcessAssertion(ProcessID pid, const String&, ProcessAssertionType assertionType)
+ProcessAssertion::ProcessAssertion(ProcessID pid, const String& reason, ProcessAssertionType assertionType)
: m_assertionType(assertionType)
, m_pid(pid)
+ , m_reason(reason)
{
}
Modified: trunk/Source/WebKit/UIProcess/ProcessAssertion.h (279513 => 279514)
--- trunk/Source/WebKit/UIProcess/ProcessAssertion.h 2021-07-02 20:33:30 UTC (rev 279513)
+++ trunk/Source/WebKit/UIProcess/ProcessAssertion.h 2021-07-02 20:35:05 UTC (rev 279514)
@@ -49,6 +49,7 @@
UnboundedNetworking,
Foreground,
MediaPlayback,
+ FinishTaskUninterruptable,
};
class ProcessAssertion : public CanMakeWeakPtr<ProcessAssertion> {
@@ -72,6 +73,7 @@
private:
const ProcessAssertionType m_assertionType;
const ProcessID m_pid;
+ const String m_reason;
#if PLATFORM(IOS_FAMILY)
RetainPtr<RBSAssertion> m_rbsAssertion;
RetainPtr<WKRBSAssertionDelegate> m_delegate;
Modified: trunk/Source/WebKit/UIProcess/ProcessThrottler.cpp (279513 => 279514)
--- trunk/Source/WebKit/UIProcess/ProcessThrottler.cpp 2021-07-02 20:33:30 UTC (rev 279513)
+++ trunk/Source/WebKit/UIProcess/ProcessThrottler.cpp 2021-07-02 20:35:05 UTC (rev 279514)
@@ -112,6 +112,7 @@
return "Suspended"_s;
case ProcessAssertionType::UnboundedNetworking:
case ProcessAssertionType::MediaPlayback:
+ case ProcessAssertionType::FinishTaskUninterruptable:
ASSERT_NOT_REACHED(); // These other assertion types are not used by the ProcessThrottler.
break;
}
Modified: trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp (279513 => 279514)
--- trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp 2021-07-02 20:33:30 UTC (rev 279513)
+++ trunk/Source/WebKit/UIProcess/WebProcessProxy.cpp 2021-07-02 20:35:05 UTC (rev 279514)
@@ -1476,6 +1476,7 @@
case ProcessAssertionType::MediaPlayback:
case ProcessAssertionType::UnboundedNetworking:
+ case ProcessAssertionType::FinishTaskUninterruptable:
ASSERT_NOT_REACHED();
}
Modified: trunk/Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm (279513 => 279514)
--- trunk/Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm 2021-07-02 20:33:30 UTC (rev 279513)
+++ trunk/Source/WebKit/UIProcess/ios/ProcessAssertionIOS.mm 2021-07-02 20:35:05 UTC (rev 279514)
@@ -291,12 +291,29 @@
return @"Foreground";
case ProcessAssertionType::MediaPlayback:
return @"MediaPlayback";
+ case ProcessAssertionType::FinishTaskUninterruptable:
+ return @"FinishTaskUninterruptable";
}
}
+static NSString *runningBoardDomainForAssertionType(ProcessAssertionType assertionType)
+{
+ switch (assertionType) {
+ case ProcessAssertionType::Suspended:
+ case ProcessAssertionType::Background:
+ case ProcessAssertionType::UnboundedNetworking:
+ case ProcessAssertionType::Foreground:
+ case ProcessAssertionType::MediaPlayback:
+ return @"com.apple.webkit";
+ case ProcessAssertionType::FinishTaskUninterruptable:
+ return @"com.apple.common";
+ }
+}
+
ProcessAssertion::ProcessAssertion(pid_t pid, const String& reason, ProcessAssertionType assertionType)
: m_assertionType(assertionType)
, m_pid(pid)
+ , m_reason(reason)
{
NSString *runningBoardAssertionName = runningBoardNameForAssertionType(assertionType);
ASSERT(runningBoardAssertionName);
@@ -306,7 +323,7 @@
}
RBSTarget *target = [RBSTarget targetWithPid:pid];
- RBSDomainAttribute *domainAttribute = [RBSDomainAttribute attributeWithDomain:@"com.apple.webkit" name:runningBoardAssertionName];
+ RBSDomainAttribute *domainAttribute = [RBSDomainAttribute attributeWithDomain:runningBoardDomainForAssertionType(assertionType) name:runningBoardAssertionName];
m_rbsAssertion = adoptNS([[RBSAssertion alloc] initWithExplanation:reason target:target attributes:@[domainAttribute]]);
m_delegate = adoptNS([[WKRBSAssertionDelegate alloc] init]);
@@ -329,7 +346,7 @@
ProcessAssertion::~ProcessAssertion()
{
- RELEASE_LOG(ProcessSuspension, "%p - ~ProcessAssertion() Releasing process assertion for process with PID=%d", this, m_pid);
+ RELEASE_LOG(ProcessSuspension, "%p - ~ProcessAssertion() Releasing process assertion '%{public}s' for process with PID=%d", this, m_reason.utf8().data(), m_pid);
if (m_rbsAssertion) {
m_delegate.get().invalidationCallback = nil;
