Diff
Modified: trunk/Source/WebKit/ChangeLog (248778 => 248779)
--- trunk/Source/WebKit/ChangeLog 2019-08-16 17:55:25 UTC (rev 248778)
+++ trunk/Source/WebKit/ChangeLog 2019-08-16 18:15:21 UTC (rev 248779)
@@ -1,5 +1,39 @@
2019-08-16 Chris Dumez <cdu...@apple.com>
+ Clarify StorageManagerSet / StorageManager threading model after r248734
+ https://bugs.webkit.org/show_bug.cgi?id=200817
+
+ Reviewed by Geoffrey Garen.
+
+ Clarify StorageManagerSet / StorageManager threading model after r248734. StorageManager is now
+ a background thread object but it still calls its completion handlers on the main thread, which
+ is very error prone. The pattern in WebKit for thread safety is that methods should always call
+ their completion handler of the thread / queue they were called on themselves. Doing differently
+ has caused so many thread-safety bugs in the past.
+
+ * NetworkProcess/WebStorage/StorageManager.cpp:
+ (WebKit::StorageManager::getSessionStorageOrigins const):
+ (WebKit::StorageManager::deleteSessionStorageOrigins):
+ (WebKit::StorageManager::deleteSessionStorageEntriesForOrigins):
+ (WebKit::StorageManager::getLocalStorageOrigins const):
+ (WebKit::StorageManager::getLocalStorageOriginDetails const):
+ (WebKit::StorageManager::deleteLocalStorageOriginsModifiedSince):
+ (WebKit::StorageManager::deleteLocalStorageEntriesForOrigins):
+ (WebKit::StorageManager::getSessionStorageOrigins): Deleted.
+ (WebKit::StorageManager::getLocalStorageOrigins): Deleted.
+ (WebKit::StorageManager::getLocalStorageOriginDetails): Deleted.
+ * NetworkProcess/WebStorage/StorageManager.h:
+ * NetworkProcess/WebStorage/StorageManagerSet.cpp:
+ (WebKit::StorageManagerSet::getSessionStorageOrigins):
+ (WebKit::StorageManagerSet::deleteSessionStorage):
+ (WebKit::StorageManagerSet::deleteSessionStorageForOrigins):
+ (WebKit::StorageManagerSet::getLocalStorageOrigins):
+ (WebKit::StorageManagerSet::deleteLocalStorageModifiedSince):
+ (WebKit::StorageManagerSet::deleteLocalStorageForOrigins):
+ (WebKit::StorageManagerSet::getLocalStorageOriginDetails):
+
+2019-08-16 Chris Dumez <cdu...@apple.com>
+
StorageManagerSet constructor should not be public
https://bugs.webkit.org/show_bug.cgi?id=200816
Modified: trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.cpp (248778 => 248779)
--- trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.cpp 2019-08-16 17:55:25 UTC (rev 248778)
+++ trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.cpp 2019-08-16 18:15:21 UTC (rev 248779)
@@ -131,7 +131,7 @@
return databaseOrigins;
}
-Vector<LocalStorageDatabaseTracker::OriginDetails> LocalStorageDatabaseTracker::originDetails()
+Vector<LocalStorageDatabaseTracker::OriginDetails> LocalStorageDatabaseTracker::originDetailsCrossThreadCopy()
{
Vector<OriginDetails> result;
auto databaseOrigins = origins();
@@ -141,10 +141,10 @@
String path = databasePath(origin);
OriginDetails details;
- details.originIdentifier = origin.databaseIdentifier();
+ details.originIdentifier = crossThreadCopy(origin.databaseIdentifier());
details.creationTime = SQLiteFileSystem::databaseCreationTime(path);
details.modificationTime = SQLiteFileSystem::databaseModificationTime(path);
- result.uncheckedAppend(details);
+ result.uncheckedAppend(WTFMove(details));
}
return result;
Modified: trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.h (248778 => 248779)
--- trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.h 2019-08-16 17:55:25 UTC (rev 248778)
+++ trunk/Source/WebKit/NetworkProcess/WebStorage/LocalStorageDatabaseTracker.h 2019-08-16 18:15:21 UTC (rev 248779)
@@ -61,7 +61,7 @@
OriginDetails isolatedCopy() const { return { originIdentifier.isolatedCopy(), creationTime, modificationTime }; }
};
- Vector<OriginDetails> originDetails();
+ Vector<OriginDetails> originDetailsCrossThreadCopy();
private:
LocalStorageDatabaseTracker(String&& localStorageDirectory);
Modified: trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp (248778 => 248779)
--- trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp 2019-08-16 17:55:25 UTC (rev 248778)
+++ trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp 2019-08-16 18:15:21 UTC (rev 248779)
@@ -92,7 +92,7 @@
sessionStorageNamespace->cloneTo(*newSessionStorageNamespace);
}
-void StorageManager::getSessionStorageOrigins(Function<void(HashSet<WebCore::SecurityOriginData>&&)>&& completionHandler)
+HashSet<WebCore::SecurityOriginData> StorageManager::getSessionStorageOriginsCrossThreadCopy() const
{
ASSERT(!RunLoop::isMain());
@@ -99,25 +99,21 @@
HashSet<SecurityOriginData> origins;
for (const auto& sessionStorageNamespace : m_sessionStorageNamespaces.values()) {
for (auto& origin : sessionStorageNamespace->origins())
- origins.add(crossThreadCopy(origin));
+ origins.add(crossThreadCopy(origin));
}
- RunLoop::main().dispatch([origins = WTFMove(origins), completionHandler = WTFMove(completionHandler)]() mutable {
- completionHandler(WTFMove(origins));
- });
+ return origins;
}
-void StorageManager::deleteSessionStorageOrigins(Function<void()>&& completionHandler)
+void StorageManager::deleteSessionStorageOrigins()
{
ASSERT(!RunLoop::isMain());
for (auto& sessionStorageNamespace : m_sessionStorageNamespaces.values())
sessionStorageNamespace->clearAllStorageAreas();
-
- RunLoop::main().dispatch(WTFMove(completionHandler));
}
-void StorageManager::deleteSessionStorageEntriesForOrigins(const Vector<WebCore::SecurityOriginData>& origins, Function<void()>&& completionHandler)
+void StorageManager::deleteSessionStorageEntriesForOrigins(const Vector<WebCore::SecurityOriginData>& origins)
{
ASSERT(!RunLoop::isMain());
@@ -125,11 +121,9 @@
for (auto& sessionStorageNamespace : m_sessionStorageNamespaces.values())
sessionStorageNamespace->clearStorageAreasMatchingOrigin(origin);
}
-
- RunLoop::main().dispatch(WTFMove(completionHandler));
}
-void StorageManager::getLocalStorageOrigins(Function<void(HashSet<WebCore::SecurityOriginData>&&)>&& completionHandler)
+HashSet<WebCore::SecurityOriginData> StorageManager::getLocalStorageOriginsCrossThreadCopy() const
{
ASSERT(!RunLoop::isMain());
@@ -149,25 +143,19 @@
origins.add(origin.isolatedCopy());
}
- RunLoop::main().dispatch([origins = WTFMove(origins), completionHandler = WTFMove(completionHandler)]() mutable {
- completionHandler(WTFMove(origins));
- });
+ return origins;
}
-void StorageManager::getLocalStorageOriginDetails(Function<void(Vector<LocalStorageDatabaseTracker::OriginDetails>&&)>&& completionHandler)
+Vector<LocalStorageDatabaseTracker::OriginDetails> StorageManager::getLocalStorageOriginDetailsCrossThreadCopy() const
{
ASSERT(!RunLoop::isMain());
- Vector<LocalStorageDatabaseTracker::OriginDetails> originDetails;
if (m_localStorageDatabaseTracker)
- originDetails = crossThreadCopy(m_localStorageDatabaseTracker->originDetails());
-
- RunLoop::main().dispatch([originDetails = WTFMove(originDetails), completionHandler = WTFMove(completionHandler)]() mutable {
- completionHandler(WTFMove(originDetails));
- });
+ return m_localStorageDatabaseTracker->originDetailsCrossThreadCopy();
+ return { };
}
-void StorageManager::deleteLocalStorageOriginsModifiedSince(WallTime time, Function<void()>&& completionHandler)
+void StorageManager::deleteLocalStorageOriginsModifiedSince(WallTime time)
{
ASSERT(!RunLoop::isMain());
@@ -186,11 +174,9 @@
for (auto& localStorageNamespace : m_localStorageNamespaces.values())
localStorageNamespace->clearAllStorageAreas();
}
-
- RunLoop::main().dispatch(WTFMove(completionHandler));
}
-void StorageManager::deleteLocalStorageEntriesForOrigins(const Vector<WebCore::SecurityOriginData>& origins, Function<void()>&& completionHandler)
+void StorageManager::deleteLocalStorageEntriesForOrigins(const Vector<WebCore::SecurityOriginData>& origins)
{
ASSERT(!RunLoop::isMain());
@@ -204,8 +190,6 @@
if (m_localStorageDatabaseTracker)
m_localStorageDatabaseTracker->deleteDatabaseWithOrigin(origin);
}
-
- RunLoop::main().dispatch(WTFMove(completionHandler));
}
StorageArea* StorageManager::createLocalStorageArea(uint64_t storageNamespaceID, WebCore::SecurityOriginData&& origin)
Modified: trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h (248778 => 248779)
--- trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h 2019-08-16 17:55:25 UTC (rev 248778)
+++ trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h 2019-08-16 18:15:21 UTC (rev 248779)
@@ -30,7 +30,6 @@
#include <WebCore/SecurityOriginData.h>
#include <WebCore/StorageMap.h>
#include <wtf/Forward.h>
-#include <wtf/Function.h>
#include <wtf/HashSet.h>
#include <wtf/text/StringHash.h>
@@ -62,14 +61,14 @@
void destroySessionStorageNamespace(uint64_t storageNamespaceID);
void cloneSessionStorageNamespace(uint64_t storageNamespaceID, uint64_t newStorageNamespaceID);
- void getSessionStorageOrigins(Function<void(HashSet<WebCore::SecurityOriginData>&&)>&&);
- void deleteSessionStorageOrigins(Function<void()>&& completionHandler);
- void deleteSessionStorageEntriesForOrigins(const Vector<WebCore::SecurityOriginData>&, Function<void()>&&);
+ HashSet<WebCore::SecurityOriginData> getSessionStorageOriginsCrossThreadCopy() const;
+ void deleteSessionStorageOrigins();
+ void deleteSessionStorageEntriesForOrigins(const Vector<WebCore::SecurityOriginData>&);
- void getLocalStorageOrigins(Function<void(HashSet<WebCore::SecurityOriginData>&&)>&&);
- void deleteLocalStorageOriginsModifiedSince(WallTime, Function<void()>&&);
- void deleteLocalStorageEntriesForOrigins(const Vector<WebCore::SecurityOriginData>&, Function<void()>&&);
- void getLocalStorageOriginDetails(Function<void(Vector<LocalStorageDatabaseTracker::OriginDetails>&&)>&&);
+ HashSet<WebCore::SecurityOriginData> getLocalStorageOriginsCrossThreadCopy() const;
+ void deleteLocalStorageOriginsModifiedSince(WallTime);
+ void deleteLocalStorageEntriesForOrigins(const Vector<WebCore::SecurityOriginData>&);
+ Vector<LocalStorageDatabaseTracker::OriginDetails> getLocalStorageOriginDetailsCrossThreadCopy() const;
void clearStorageNamespaces();
Modified: trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp (248778 => 248779)
--- trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp 2019-08-16 17:55:25 UTC (rev 248778)
+++ trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManagerSet.cpp 2019-08-16 18:15:21 UTC (rev 248779)
@@ -185,73 +185,100 @@
m_stateChangeCondition.notifyOne();
}
-void StorageManagerSet::getSessionStorageOrigins(PAL::SessionID sessionID, CompletionHandler<void(HashSet<WebCore::SecurityOriginData>&&)>&& completionHandler)
+void StorageManagerSet::getSessionStorageOrigins(PAL::SessionID sessionID, GetOriginsCallback&& completionHandler)
{
+ ASSERT(RunLoop::isMain());
+
m_queue->dispatch([this, protectedThis = makeRef(*this), sessionID, completionHandler = WTFMove(completionHandler)]() mutable {
auto* storageManager = m_storageManagers.get(sessionID);
ASSERT(storageManager);
- storageManager->getSessionStorageOrigins(WTFMove(completionHandler));
+ auto origins = storageManager->getSessionStorageOriginsCrossThreadCopy();
+ RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler), origins = WTFMove(origins)]() mutable {
+ completionHandler(WTFMove(origins));
+ });
});
}
-void StorageManagerSet::deleteSessionStorage(PAL::SessionID sessionID, CompletionHandler<void()>&& completionHandler)
+void StorageManagerSet::deleteSessionStorage(PAL::SessionID sessionID, DeleteCallback&& completionHandler)
{
+ ASSERT(RunLoop::isMain());
+
m_queue->dispatch([this, protectedThis = makeRef(*this), sessionID, completionHandler = WTFMove(completionHandler)]() mutable {
auto* storageManager = m_storageManagers.get(sessionID);
ASSERT(storageManager);
- storageManager->deleteSessionStorageOrigins(WTFMove(completionHandler));
+ storageManager->deleteSessionStorageOrigins();
+ RunLoop::main().dispatch(WTFMove(completionHandler));
});
}
void StorageManagerSet::deleteSessionStorageForOrigins(PAL::SessionID sessionID, const Vector<WebCore::SecurityOriginData>& originDatas, DeleteCallback&& completionHandler)
{
+ ASSERT(RunLoop::isMain());
+
m_queue->dispatch([this, protectedThis = makeRef(*this), sessionID, copiedOriginDatas = crossThreadCopy(originDatas), completionHandler = WTFMove(completionHandler)]() mutable {
auto* storageManager = m_storageManagers.get(sessionID);
ASSERT(storageManager);
- storageManager->deleteSessionStorageEntriesForOrigins(copiedOriginDatas, WTFMove(completionHandler));
+ storageManager->deleteSessionStorageEntriesForOrigins(copiedOriginDatas);
+ RunLoop::main().dispatch(WTFMove(completionHandler));
});
}
void StorageManagerSet::getLocalStorageOrigins(PAL::SessionID sessionID, GetOriginsCallback&& completionHandler)
{
+ ASSERT(RunLoop::isMain());
+
m_queue->dispatch([this, protectedThis = makeRef(*this), sessionID, completionHandler = WTFMove(completionHandler)]() mutable {
auto* storageManager = m_storageManagers.get(sessionID);
ASSERT(storageManager);
- storageManager->getLocalStorageOrigins(WTFMove(completionHandler));
+ auto origins = storageManager->getLocalStorageOriginsCrossThreadCopy();
+ RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler), origins = WTFMove(origins)]() mutable {
+ completionHandler(WTFMove(origins));
+ });
});
}
void StorageManagerSet::deleteLocalStorageModifiedSince(PAL::SessionID sessionID, WallTime time, DeleteCallback&& completionHandler)
{
+ ASSERT(RunLoop::isMain());
+
m_queue->dispatch([this, protectedThis = makeRef(*this), sessionID, time, completionHandler = WTFMove(completionHandler)]() mutable {
auto* storageManager = m_storageManagers.get(sessionID);
ASSERT(storageManager);
- storageManager->deleteLocalStorageOriginsModifiedSince(time, WTFMove(completionHandler));
+ storageManager->deleteLocalStorageOriginsModifiedSince(time);
+ RunLoop::main().dispatch(WTFMove(completionHandler));
});
}
void StorageManagerSet::deleteLocalStorageForOrigins(PAL::SessionID sessionID, const Vector<WebCore::SecurityOriginData>& originDatas, DeleteCallback&& completionHandler)
{
+ ASSERT(RunLoop::isMain());
+
m_queue->dispatch([this, protectedThis = makeRef(*this), sessionID, copiedOriginDatas = crossThreadCopy(originDatas), completionHandler = WTFMove(completionHandler)]() mutable {
auto* storageManager = m_storageManagers.get(sessionID);
ASSERT(storageManager);
- storageManager->deleteLocalStorageEntriesForOrigins(copiedOriginDatas, WTFMove(completionHandler));
+ storageManager->deleteLocalStorageEntriesForOrigins(copiedOriginDatas);
+ RunLoop::main().dispatch(WTFMove(completionHandler));
});
}
-void StorageManagerSet::getLocalStorageOriginDetails(PAL::SessionID sessionID, CompletionHandler<void(Vector<LocalStorageDatabaseTracker::OriginDetails>&&)>&& completionHandler)
+void StorageManagerSet::getLocalStorageOriginDetails(PAL::SessionID sessionID, GetOriginDetailsCallback&& completionHandler)
{
+ ASSERT(RunLoop::isMain());
+
m_queue->dispatch([this, protectedThis = makeRef(*this), sessionID, completionHandler = WTFMove(completionHandler)]() mutable {
auto* storageManager = m_storageManagers.get(sessionID);
ASSERT(storageManager);
- storageManager->getLocalStorageOriginDetails(WTFMove(completionHandler));
+ auto originDetails = storageManager->getLocalStorageOriginDetailsCrossThreadCopy();
+ RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler), originDetails = WTFMove(originDetails)]() mutable {
+ completionHandler(WTFMove(originDetails));
+ });
});
}
