Attachment 372295 Details for Bug 198939 – Patch

[patch] Patch

bug-198939-20190617170106.patch (text/plain), 6.92 KB, created by Alex Christensen on 2019-06-17 17:01:07 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Alex Christensen

Created: 2019-06-17 17:01:07 PDT

Size: 6.92 KB

patch

obsolete

>Index: Source/WebKit/ChangeLog
>===================================================================
>--- Source/WebKit/ChangeLog	(revision 246522)
>+++ Source/WebKit/ChangeLog	(working copy)
>@@ -1,3 +1,25 @@
>+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 NOBODY (OOPS!).
>+
>+        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
>Index: Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp
>===================================================================
>--- Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp	(revision 246516)
>+++ Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp	(working copy)
>@@ -376,6 +376,7 @@ void StorageManager::LocalStorageNamespa
>     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::cloneSessionStorage
>         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::getLocalStorageOrig
>             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::getLocalStorageOrig
> 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::deleteLocalStorageO
>                 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::deleteLocalStorageE
> 
>     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* StorageMana
> 
> StorageManager::LocalStorageNamespace* StorageManager::getOrCreateLocalStorageNamespace(uint64_t storageNamespaceID)
> {
>+    std::lock_guard<Lock> lock(m_localStorageNamespacesMutex);
>     if (!m_localStorageNamespaces.isValidKey(storageNamespaceID))
>         return nullptr;
> 
>Index: Source/WebKit/NetworkProcess/WebStorage/StorageManager.h
>===================================================================
>--- Source/WebKit/NetworkProcess/WebStorage/StorageManager.h	(revision 246516)
>+++ Source/WebKit/NetworkProcess/WebStorage/StorageManager.h	(working copy)
>@@ -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;
>

Flags:

ggaren: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 198939: 372286 | 372295