- Revision
- 290623
- Author
- [email protected]
- Date
- 2022-02-28 18:34:32 -0800 (Mon, 28 Feb 2022)
Log Message
[macOS] TestWebKitAPI.WebKit.MigrateLocalStorageDataToGeneralStorageDirectory is a flaky failure
https://bugs.webkit.org/show_bug.cgi?id=237065
<rdar://problem/89324250>
Reviewed by Darin Adler.
Revert r290544 as it does not fully fix the flaky test. The flakiness is caused by network process exiting
before transactions of SQLiteStorageArea are committed. In NetworkProcess::destroySession, we schedule a
background task to commit transactions in the session, and remove session. In NetworkProcess::didClose, for
existing sessions, we schedule tasks to commit transaction and wait until the tasks are finished before exiting
network process. That means for a session, when NetworkProcess::destroySession is called before
NetworkProcess::didClose, process may exit before commit task (scheduled by NetworkProcess::destroySession) is
finished (NetworkProcess::didClose does not wait because session is already removed).
To solve this issue, let NetworkProcess::didClose check existing NetworkStorageManagers, instead of
NetworkSessions. NetworkStorageManager can outlive NetworkSession when it's finishing cleanup task
(including committing transactions) on its background thread; we want to ensure process exits after the cleanup
task is done.
* NetworkProcess/NetworkProcess.cpp:
(WebKit::NetworkProcess::didClose):
* NetworkProcess/storage/NetworkStorageManager.cpp:
(WebKit::allNetworkStorageManagers):
(WebKit::NetworkStorageManager::forEach):
(WebKit::NetworkStorageManager::NetworkStorageManager):
(WebKit::NetworkStorageManager::~NetworkStorageManager):
* NetworkProcess/storage/NetworkStorageManager.h:
* NetworkProcess/storage/SQLiteStorageArea.cpp:
(WebKit::SQLiteStorageArea::SQLiteStorageArea):
(WebKit::SQLiteStorageArea::startTransactionIfNecessary):
(WebKit::SQLiteStorageArea::commitTransactionIfNecessary):
(WebKit::commitTransactionsAtExit): Deleted.
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (290622 => 290623)
--- trunk/Source/WebKit/ChangeLog 2022-03-01 01:03:10 UTC (rev 290622)
+++ trunk/Source/WebKit/ChangeLog 2022-03-01 02:34:32 UTC (rev 290623)
@@ -1,3 +1,38 @@
+2022-02-28 Sihui Liu <[email protected]>
+
+ [macOS] TestWebKitAPI.WebKit.MigrateLocalStorageDataToGeneralStorageDirectory is a flaky failure
+ https://bugs.webkit.org/show_bug.cgi?id=237065
+ <rdar://problem/89324250>
+
+ Reviewed by Darin Adler.
+
+ Revert r290544 as it does not fully fix the flaky test. The flakiness is caused by network process exiting
+ before transactions of SQLiteStorageArea are committed. In NetworkProcess::destroySession, we schedule a
+ background task to commit transactions in the session, and remove session. In NetworkProcess::didClose, for
+ existing sessions, we schedule tasks to commit transaction and wait until the tasks are finished before exiting
+ network process. That means for a session, when NetworkProcess::destroySession is called before
+ NetworkProcess::didClose, process may exit before commit task (scheduled by NetworkProcess::destroySession) is
+ finished (NetworkProcess::didClose does not wait because session is already removed).
+
+ To solve this issue, let NetworkProcess::didClose check existing NetworkStorageManagers, instead of
+ NetworkSessions. NetworkStorageManager can outlive NetworkSession when it's finishing cleanup task
+ (including committing transactions) on its background thread; we want to ensure process exits after the cleanup
+ task is done.
+
+ * NetworkProcess/NetworkProcess.cpp:
+ (WebKit::NetworkProcess::didClose):
+ * NetworkProcess/storage/NetworkStorageManager.cpp:
+ (WebKit::allNetworkStorageManagers):
+ (WebKit::NetworkStorageManager::forEach):
+ (WebKit::NetworkStorageManager::NetworkStorageManager):
+ (WebKit::NetworkStorageManager::~NetworkStorageManager):
+ * NetworkProcess/storage/NetworkStorageManager.h:
+ * NetworkProcess/storage/SQLiteStorageArea.cpp:
+ (WebKit::SQLiteStorageArea::SQLiteStorageArea):
+ (WebKit::SQLiteStorageArea::startTransactionIfNecessary):
+ (WebKit::SQLiteStorageArea::commitTransactionIfNecessary):
+ (WebKit::commitTransactionsAtExit): Deleted.
+
2022-02-28 Jer Noble <[email protected]>
[Cocoa] Adopt -streamDataParser:didProvideContentKeySpecifier:forTrackID: delegate callback
Modified: trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp (290622 => 290623)
--- trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp 2022-03-01 01:03:10 UTC (rev 290622)
+++ trunk/Source/WebKit/NetworkProcess/NetworkProcess.cpp 2022-03-01 02:34:32 UTC (rev 290623)
@@ -261,11 +261,13 @@
stopRunLoop();
});
- forEachNetworkSession([&] (auto& session) {
+ forEachNetworkSession([&](auto& session) {
platformFlushCookies(session.sessionID(), [callbackAggregator] { });
- if (auto* storageManager = session.storageManager())
- storageManager->syncLocalStorage([callbackAggregator] { });
});
+
+ NetworkStorageManager::forEach([&](auto& manager) {
+ manager.syncLocalStorage([callbackAggregator] { });
+ });
}
void NetworkProcess::didCreateDownload()
Modified: trunk/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp (290622 => 290623)
--- trunk/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp 2022-03-01 01:03:10 UTC (rev 290622)
+++ trunk/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.cpp 2022-03-01 02:34:32 UTC (rev 290623)
@@ -108,6 +108,18 @@
FileSystem::deleteFile(filePath);
}
+static WeakHashSet<NetworkStorageManager>& allNetworkStorageManagers()
+{
+ static MainThreadNeverDestroyed<WeakHashSet<NetworkStorageManager>> managers;
+ return managers;
+}
+
+void NetworkStorageManager::forEach(const Function<void(NetworkStorageManager&)>& apply)
+{
+ for (auto& manager : allNetworkStorageManagers())
+ apply(manager);
+}
+
Ref<NetworkStorageManager> NetworkStorageManager::create(PAL::SessionID sessionID, IPC::Connection::UniqueID connection, const String& path, const String& customLocalStoragePath, const String& customIDBStoragePath, const String& customCacheStoragePath, uint64_t defaultOriginQuota, uint64_t defaultThirdPartyOriginQuota, bool shouldUseCustomPaths)
{
return adoptRef(*new NetworkStorageManager(sessionID, connection, path, customLocalStoragePath, customIDBStoragePath, customCacheStoragePath, defaultOriginQuota, defaultThirdPartyOriginQuota, shouldUseCustomPaths));
@@ -122,6 +134,7 @@
{
ASSERT(RunLoop::isMain());
+ allNetworkStorageManagers().add(*this);
m_queue->dispatch([this, protectedThis = Ref { *this }, path = path.isolatedCopy(), customLocalStoragePath = crossThreadCopy(customLocalStoragePath), customIDBStoragePath = crossThreadCopy(customIDBStoragePath), customCacheStoragePath = crossThreadCopy(customCacheStoragePath), shouldUseCustomPaths]() mutable {
m_fileSystemStorageHandleRegistry = makeUnique<FileSystemStorageHandleRegistry>();
m_storageAreaRegistry = makeUnique<StorageAreaRegistry>();
@@ -142,6 +155,8 @@
{
ASSERT(RunLoop::isMain());
ASSERT(m_closed);
+
+ allNetworkStorageManagers().remove(*this);
}
bool NetworkStorageManager::canHandleTypes(OptionSet<WebsiteDataType> types)
Modified: trunk/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h (290622 => 290623)
--- trunk/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h 2022-03-01 01:03:10 UTC (rev 290622)
+++ trunk/Source/WebKit/NetworkProcess/storage/NetworkStorageManager.h 2022-03-01 02:34:32 UTC (rev 290623)
@@ -40,6 +40,7 @@
#include <WebCore/IndexedDB.h>
#include <pal/SessionID.h>
#include <wtf/Forward.h>
+#include <wtf/WeakHashSet.h>
namespace IPC {
class SharedFileHandle;
@@ -70,6 +71,7 @@
class NetworkStorageManager final : public IPC::Connection::WorkQueueMessageReceiver {
public:
+ static void forEach(const Function<void(NetworkStorageManager&)>&);
static Ref<NetworkStorageManager> create(PAL::SessionID, IPC::Connection::UniqueID, const String& path, const String& customLocalStoragePath, const String& customIDBStoragePath, const String& customCacheStoragePath, uint64_t defaultOriginQuota, uint64_t defaultThirdPartyOriginQuota, bool shouldUseCustomPaths);
static bool canHandleTypes(OptionSet<WebsiteDataType>);
Modified: trunk/Source/WebKit/NetworkProcess/storage/SQLiteStorageArea.cpp (290622 => 290623)
--- trunk/Source/WebKit/NetworkProcess/storage/SQLiteStorageArea.cpp 2022-03-01 01:03:10 UTC (rev 290622)
+++ trunk/Source/WebKit/NetworkProcess/storage/SQLiteStorageArea.cpp 2022-03-01 02:34:32 UTC (rev 290623)
@@ -62,20 +62,6 @@
return ""_s;
}
-static Lock allTransactionsLock;
-static HashSet<WebCore::SQLiteTransaction*>& allTransactions() WTF_REQUIRES_LOCK(allTransactionsLock)
-{
- static NeverDestroyed<HashSet<WebCore::SQLiteTransaction*>> transactions;
- return transactions;
-}
-
-static void commitTransactionsAtExit()
-{
- Locker lock { allTransactionsLock };
- for (auto* transaction : allTransactions())
- transaction->commit();
-}
-
SQLiteStorageArea::SQLiteStorageArea(unsigned quota, const WebCore::ClientOrigin& origin, const String& path, Ref<WorkQueue>&& workQueue)
: StorageAreaBase(quota, origin)
, m_path(path)
@@ -83,11 +69,6 @@
, m_cachedStatements(static_cast<size_t>(StatementType::Invalid))
{
ASSERT(!isMainRunLoop());
-
- static std::once_flag once;
- std::call_once(once, [] {
- std::atexit(commitTransactionsAtExit);
- });
}
void SQLiteStorageArea::close()
@@ -205,15 +186,10 @@
if (!m_transaction)
m_transaction = makeUnique<WebCore::SQLiteTransaction>(*m_database);
- {
- Locker lock { allTransactionsLock };
- if (m_transaction->inProgress())
- return;
+ if (m_transaction->inProgress())
+ return;
+ m_transaction->begin();
- m_transaction->begin();
- allTransactions().add(m_transaction.get());
- }
-
m_queue->dispatchAfter(transactionDuration, [weakThis = WeakPtr { *this }] {
if (weakThis)
weakThis->commitTransactionIfNecessary();
@@ -419,13 +395,8 @@
void SQLiteStorageArea::commitTransactionIfNecessary()
{
- auto transaction = std::exchange(m_transaction, nullptr);
- if (!transaction)
- return;
-
- Locker lock { allTransactionsLock };
- transaction->commit();
- allTransactions().remove(transaction.get());
+ if (auto transaction = std::exchange(m_transaction, nullptr))
+ transaction->commit();
}
void SQLiteStorageArea::handleLowMemoryWarning()
