Modified: trunk/Source/WebKit/ChangeLog (246900 => 246901)
--- trunk/Source/WebKit/ChangeLog 2019-06-27 20:44:16 UTC (rev 246900)
+++ trunk/Source/WebKit/ChangeLog 2019-06-27 20:46:23 UTC (rev 246901)
@@ -1,3 +1,36 @@
+2019-06-27 Sihui Liu <[email protected]>
+
+ Regression(r246526): StorageManager thread hangs
+ https://bugs.webkit.org/show_bug.cgi?id=199278
+ <rdar://problem/52202948>
+
+ Reviewed by Geoffrey Garen.
+
+ r246526 adds a lock m_localStorageNamespacesMutex to protect m_localStorageNamespaces, because
+ m_localStorageNamespaces is destroyed at main thread while accesses to m_localStorageNamespaces happen in the
+ background thread.
+ After r246526, getOrCreateLocalStorageNamespace acquires lock m_localStorageNamespacesMutex when
+ m_localStorageNamespacesMutex is already acquired in cloneSessionStorageNamespace, so the StorageManager thread
+ hangs.
+ To solve this issue, we can remove the lock in getOrCreateLocalStorageNamespace, or we can remove the
+ m_localStorageNamespacesMutex. waitUntilWritesFinished() before ~StorageManager() already guarantees nothing
+ will be running in the background thread, so it is unlikely we the access to m_localStorageNamespaces in the
+ background thread would collide with the destruction of m_localStorageNamespaces. Also, we don't need
+ didDestroyStorageArea as LocalStorageNamespace can hold the last reference of StorageArea after r245881.
+
+ * NetworkProcess/WebStorage/StorageManager.cpp:
+ (WebKit::StorageManager::StorageArea::StorageArea):
+ (WebKit::StorageManager::StorageArea::~StorageArea):
+ (WebKit::StorageManager::LocalStorageNamespace::LocalStorageNamespace):
+ (WebKit::StorageManager::cloneSessionStorageNamespace):
+ (WebKit::StorageManager::getLocalStorageOrigins):
+ (WebKit::StorageManager::deleteLocalStorageEntriesForOrigin):
+ (WebKit::StorageManager::deleteLocalStorageOriginsModifiedSince):
+ (WebKit::StorageManager::deleteLocalStorageEntriesForOrigins):
+ (WebKit::StorageManager::getOrCreateLocalStorageNamespace):
+ (WebKit::StorageManager::LocalStorageNamespace::didDestroyStorageArea): Deleted.
+ * NetworkProcess/WebStorage/StorageManager.h:
+
2019-06-27 Carlos Garcia Campos <[email protected]>
WebSockets: avoid data copies when queuing tasks in WebSocketChannel
Modified: trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp (246900 => 246901)
--- trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp 2019-06-27 20:44:16 UTC (rev 246900)
+++ trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp 2019-06-27 20:46:23 UTC (rev 246901)
@@ -73,7 +73,7 @@
void dispatchEvents(IPC::Connection::UniqueID sourceConnection, uint64_t sourceStorageAreaID, const String& key, const String& oldValue, const String& newValue, const String& urlString) const;
// Will be null if the storage area belongs to a session storage namespace or the storage area is in an ephemeral session.
- LocalStorageNamespace* m_localStorageNamespace;
+ WeakPtr<LocalStorageNamespace> m_localStorageNamespace;
mutable RefPtr<LocalStorageDatabase> m_localStorageDatabase;
mutable bool m_didImportItemsFromDatabase { false };
@@ -84,7 +84,7 @@
HashSet<std::pair<IPC::Connection::UniqueID, uint64_t>> m_eventListeners;
};
-class StorageManager::LocalStorageNamespace : public ThreadSafeRefCounted<LocalStorageNamespace> {
+class StorageManager::LocalStorageNamespace : public ThreadSafeRefCounted<LocalStorageNamespace>, public CanMakeWeakPtr<LocalStorageNamespace> {
public:
static Ref<LocalStorageNamespace> create(StorageManager&, uint64_t storageManagerID);
~LocalStorageNamespace();
@@ -93,7 +93,6 @@
enum class IsEphemeral : bool { No, Yes };
Ref<StorageArea> getOrCreateStorageArea(SecurityOriginData&&, IsEphemeral);
- void didDestroyStorageArea(StorageArea*);
void clearStorageAreasMatchingOrigin(const SecurityOriginData&);
void clearAllStorageAreas();
@@ -105,7 +104,6 @@
LocalStorageNamespace(StorageManager&, uint64_t storageManagerID);
StorageManager& m_storageManager;
- uint64_t m_storageNamespaceID;
unsigned m_quotaInBytes;
HashMap<SecurityOriginData, RefPtr<StorageArea>> m_storageAreaMap;
@@ -173,7 +171,7 @@
}
StorageManager::StorageArea::StorageArea(LocalStorageNamespace* localStorageNamespace, const SecurityOriginData& securityOrigin, unsigned quotaInBytes)
- : m_localStorageNamespace(localStorageNamespace)
+ : m_localStorageNamespace(localStorageNamespace ? makeWeakPtr(*localStorageNamespace) : nullptr)
, m_securityOrigin(securityOrigin)
, m_quotaInBytes(quotaInBytes)
, m_storageMap(StorageMap::create(m_quotaInBytes))
@@ -183,12 +181,10 @@
StorageManager::StorageArea::~StorageArea()
{
ASSERT(m_eventListeners.isEmpty());
+ ASSERT(!m_localStorageNamespace);
if (m_localStorageDatabase)
m_localStorageDatabase->close();
-
- if (m_localStorageNamespace)
- m_localStorageNamespace->didDestroyStorageArea(this);
}
void StorageManager::StorageArea::addListener(IPC::Connection::UniqueID connectionID, uint64_t storageMapID)
@@ -350,7 +346,6 @@
// We should investigate a way to share it with WebCore.
StorageManager::LocalStorageNamespace::LocalStorageNamespace(StorageManager& storageManager, uint64_t storageNamespaceID)
: m_storageManager(storageManager)
- , m_storageNamespaceID(storageNamespaceID)
, m_quotaInBytes(localStorageDatabaseQuotaInBytes)
{
}
@@ -368,19 +363,6 @@
}).iterator->value;
}
-void StorageManager::LocalStorageNamespace::didDestroyStorageArea(StorageArea* storageArea)
-{
- ASSERT(m_storageAreaMap.contains(storageArea->securityOrigin()));
-
- m_storageAreaMap.remove(storageArea->securityOrigin());
- if (!m_storageAreaMap.isEmpty())
- return;
-
- std::lock_guard<Lock> lock(m_storageManager.m_localStorageNamespacesMutex);
- ASSERT(m_storageManager.m_localStorageNamespaces.contains(m_storageNamespaceID));
- m_storageManager.m_localStorageNamespaces.remove(m_storageNamespaceID);
-}
-
void StorageManager::LocalStorageNamespace::clearStorageAreasMatchingOrigin(const SecurityOriginData& securityOrigin)
{
auto originAndStorageArea = m_storageAreaMap.find(securityOrigin);
@@ -573,7 +555,6 @@
sessionStorageNamespace->cloneTo(*newSessionStorageNamespace);
if (!m_localStorageDatabaseTracker) {
- std::lock_guard<Lock> lock(m_localStorageNamespacesMutex);
if (auto* localStorageNamespace = m_localStorageNamespaces.get(storageNamespaceID)) {
LocalStorageNamespace* newlocalStorageNamespace = getOrCreateLocalStorageNamespace(newStorageNamespaceID);
localStorageNamespace->cloneTo(*newlocalStorageNamespace);
@@ -664,7 +645,6 @@
for (auto& origin : m_localStorageDatabaseTracker->origins())
origins.add(origin);
} else {
- std::lock_guard<Lock> lock(m_localStorageNamespacesMutex);
for (const auto& localStorageNameSpace : m_localStorageNamespaces.values()) {
for (auto& origin : localStorageNameSpace->ephemeralOrigins())
origins.add(origin);
@@ -698,11 +678,8 @@
void StorageManager::deleteLocalStorageEntriesForOrigin(const SecurityOriginData& securityOrigin)
{
m_queue->dispatch([this, protectedThis = makeRef(*this), copiedOrigin = securityOrigin.isolatedCopy()]() mutable {
- {
- std::lock_guard<Lock> lock(m_localStorageNamespacesMutex);
- for (auto& localStorageNamespace : m_localStorageNamespaces.values())
- localStorageNamespace->clearStorageAreasMatchingOrigin(copiedOrigin);
- }
+ for (auto& localStorageNamespace : m_localStorageNamespaces.values())
+ localStorageNamespace->clearStorageAreasMatchingOrigin(copiedOrigin);
for (auto& transientLocalStorageNamespace : m_transientLocalStorageNamespaces.values())
transientLocalStorageNamespace->clearStorageAreasMatchingOrigin(copiedOrigin);
@@ -722,16 +699,12 @@
transientLocalStorageNamespace->clearAllStorageAreas();
for (const auto& origin : originsToDelete) {
- {
- std::lock_guard<Lock> lock(m_localStorageNamespacesMutex);
- for (auto& localStorageNamespace : m_localStorageNamespaces.values())
- localStorageNamespace->clearStorageAreasMatchingOrigin(origin);
- }
+ for (auto& localStorageNamespace : m_localStorageNamespaces.values())
+ localStorageNamespace->clearStorageAreasMatchingOrigin(origin);
m_localStorageDatabaseTracker->deleteDatabaseWithOrigin(origin);
}
} else {
- std::lock_guard<Lock> lock(m_localStorageNamespacesMutex);
for (auto& localStorageNamespace : m_localStorageNamespaces.values())
localStorageNamespace->clearAllStorageAreas();
}
@@ -750,11 +723,8 @@
m_queue->dispatch([this, protectedThis = makeRef(*this), copiedOrigins = WTFMove(copiedOrigins), completionHandler = WTFMove(completionHandler)]() mutable {
for (auto& origin : copiedOrigins) {
- {
- std::lock_guard<Lock> lock(m_localStorageNamespacesMutex);
- for (auto& localStorageNamespace : m_localStorageNamespaces.values())
- localStorageNamespace->clearStorageAreasMatchingOrigin(origin);
- }
+ for (auto& localStorageNamespace : m_localStorageNamespaces.values())
+ localStorageNamespace->clearStorageAreasMatchingOrigin(origin);
for (auto& transientLocalStorageNamespace : m_transientLocalStorageNamespaces.values())
transientLocalStorageNamespace->clearStorageAreasMatchingOrigin(origin);
@@ -1012,7 +982,6 @@
StorageManager::LocalStorageNamespace* StorageManager::getOrCreateLocalStorageNamespace(uint64_t storageNamespaceID)
{
- std::lock_guard<Lock> lock(m_localStorageNamespacesMutex);
if (!m_localStorageNamespaces.isValidKey(storageNamespaceID))
return nullptr;
Modified: trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h (246900 => 246901)
--- trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h 2019-06-27 20:44:16 UTC (rev 246900)
+++ trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h 2019-06-27 20:46:23 UTC (rev 246901)
@@ -105,7 +105,6 @@
RefPtr<LocalStorageDatabaseTracker> m_localStorageDatabaseTracker;
HashMap<uint64_t, RefPtr<LocalStorageNamespace>> m_localStorageNamespaces;
- Lock m_localStorageNamespacesMutex;
HashMap<std::pair<uint64_t, WebCore::SecurityOriginData>, RefPtr<TransientLocalStorageNamespace>> m_transientLocalStorageNamespaces;
