Protect StorageManager::m_localStorageNamespaces with a Lock
authorachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 18 Jun 2019 00:17:24 +0000 (00:17 +0000)
committerachristensen@apple.com <achristensen@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 18 Jun 2019 00:17:24 +0000 (00:17 +0000)
https://bugs.webkit.org/show_bug.cgi?id=198939
<rdar://problem/51784225>

Reviewed by Geoff Garen.

StorageManager::LocalStorageNamespace::didDestroyStorageArea is called from StorageArea::~StorageArea which is called on the main thread.
All other access of m_localStorageNamespaces is from the non-main thread.  Sometimes this causes hash table corruption, so wait for a mutex
before accessing this member variable.

* NetworkProcess/WebStorage/StorageManager.cpp:
(WebKit::StorageManager::LocalStorageNamespace::didDestroyStorageArea):
(WebKit::StorageManager::cloneSessionStorageNamespace):
(WebKit::StorageManager::getLocalStorageOrigins):
(WebKit::StorageManager::deleteLocalStorageEntriesForOrigin):
(WebKit::StorageManager::deleteLocalStorageOriginsModifiedSince):
(WebKit::StorageManager::deleteLocalStorageEntriesForOrigins):
(WebKit::StorageManager::getOrCreateLocalStorageNamespace):
* NetworkProcess/WebStorage/StorageManager.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@246526 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp
Source/WebKit/NetworkProcess/WebStorage/StorageManager.h

index 5a8c0e6..ce34490 100644 (file)
@@ -1,5 +1,27 @@
 2019-06-17  Alex Christensen  <achristensen@webkit.org>
 
+        Protect StorageManager::m_localStorageNamespaces with a Lock
+        https://bugs.webkit.org/show_bug.cgi?id=198939
+        <rdar://problem/51784225>
+
+        Reviewed by Geoff Garen.
+
+        StorageManager::LocalStorageNamespace::didDestroyStorageArea is called from StorageArea::~StorageArea which is called on the main thread.
+        All other access of m_localStorageNamespaces is from the non-main thread.  Sometimes this causes hash table corruption, so wait for a mutex
+        before accessing this member variable.
+
+        * NetworkProcess/WebStorage/StorageManager.cpp:
+        (WebKit::StorageManager::LocalStorageNamespace::didDestroyStorageArea):
+        (WebKit::StorageManager::cloneSessionStorageNamespace):
+        (WebKit::StorageManager::getLocalStorageOrigins):
+        (WebKit::StorageManager::deleteLocalStorageEntriesForOrigin):
+        (WebKit::StorageManager::deleteLocalStorageOriginsModifiedSince):
+        (WebKit::StorageManager::deleteLocalStorageEntriesForOrigins):
+        (WebKit::StorageManager::getOrCreateLocalStorageNamespace):
+        * NetworkProcess/WebStorage/StorageManager.h:
+
+2019-06-17  Alex Christensen  <achristensen@webkit.org>
+
         Add null check in WebFrameLoaderClient::assignIdentifierToInitialRequest
         https://bugs.webkit.org/show_bug.cgi?id=198926
         <rdar://problem/50079713>
index 9f3e681..df55e04 100644 (file)
@@ -376,6 +376,7 @@ void StorageManager::LocalStorageNamespace::didDestroyStorageArea(StorageArea* s
     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);
 }
@@ -572,6 +573,7 @@ void StorageManager::cloneSessionStorageNamespace(uint64_t storageNamespaceID, u
         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);
@@ -662,6 +664,7 @@ void StorageManager::getLocalStorageOrigins(Function<void(HashSet<WebCore::Secur
             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);
@@ -695,8 +698,11 @@ void StorageManager::getLocalStorageOriginDetails(Function<void(Vector<LocalStor
 void StorageManager::deleteLocalStorageEntriesForOrigin(const SecurityOriginData& securityOrigin)
 {
     m_queue->dispatch([this, protectedThis = makeRef(*this), copiedOrigin = securityOrigin.isolatedCopy()]() mutable {
-        for (auto& localStorageNamespace : m_localStorageNamespaces.values())
-            localStorageNamespace->clearStorageAreasMatchingOrigin(copiedOrigin);
+        {
+            std::lock_guard<Lock> lock(m_localStorageNamespacesMutex);
+            for (auto& localStorageNamespace : m_localStorageNamespaces.values())
+                localStorageNamespace->clearStorageAreasMatchingOrigin(copiedOrigin);
+        }
 
         for (auto& transientLocalStorageNamespace : m_transientLocalStorageNamespaces.values())
             transientLocalStorageNamespace->clearStorageAreasMatchingOrigin(copiedOrigin);
@@ -716,12 +722,16 @@ void StorageManager::deleteLocalStorageOriginsModifiedSince(WallTime time, Funct
                 transientLocalStorageNamespace->clearAllStorageAreas();
 
             for (const auto& origin : originsToDelete) {
-                for (auto& localStorageNamespace : m_localStorageNamespaces.values())
-                    localStorageNamespace->clearStorageAreasMatchingOrigin(origin);
+                {
+                    std::lock_guard<Lock> lock(m_localStorageNamespacesMutex);
+                    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();
         }
@@ -740,8 +750,11 @@ void StorageManager::deleteLocalStorageEntriesForOrigins(const Vector<WebCore::S
 
     m_queue->dispatch([this, protectedThis = makeRef(*this), copiedOrigins = WTFMove(copiedOrigins), completionHandler = WTFMove(completionHandler)]() mutable {
         for (auto& origin : copiedOrigins) {
-            for (auto& localStorageNamespace : m_localStorageNamespaces.values())
-                localStorageNamespace->clearStorageAreasMatchingOrigin(origin);
+            {
+                std::lock_guard<Lock> lock(m_localStorageNamespacesMutex);
+                for (auto& localStorageNamespace : m_localStorageNamespaces.values())
+                    localStorageNamespace->clearStorageAreasMatchingOrigin(origin);
+            }
 
             for (auto& transientLocalStorageNamespace : m_transientLocalStorageNamespaces.values())
                 transientLocalStorageNamespace->clearStorageAreasMatchingOrigin(origin);
@@ -999,6 +1012,7 @@ StorageManager::StorageArea* StorageManager::findStorageArea(IPC::Connection& co
 
 StorageManager::LocalStorageNamespace* StorageManager::getOrCreateLocalStorageNamespace(uint64_t storageNamespaceID)
 {
+    std::lock_guard<Lock> lock(m_localStorageNamespacesMutex);
     if (!m_localStorageNamespaces.isValidKey(storageNamespaceID))
         return nullptr;
 
index ef19d5b..c71cc68 100644 (file)
@@ -105,6 +105,7 @@ private:
 
     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;