Attachment 373036 Details for Bug 199277 – Patch

[patch] Patch

bug-199277-20190627105159.patch (text/plain), 9.10 KB, created by Alex Christensen on 2019-06-27 10:52:00 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Alex Christensen

Created: 2019-06-27 10:52:00 PDT

Size: 9.10 KB

patch

obsolete

>Subversion Revision: 246860
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index a7082a9f981244ee3fe681ef6e5d04066c4b5035..192581b113c7a48862e56222783c61950c1cefb3 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,29 @@
>+2019-06-27  Alex Christensen  <achristensen@webkit.org>
>+
>+        Protect StorageManager::LocalStorageNamespace::m_storageAreaMap with a mutex
>+        https://bugs.webkit.org/show_bug.cgi?id=199277
>+        <rdar://problem/52098995>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        LocalStorageNamespace::didDestroyStorageArea is called from the main thread, but other uses of m_storageAreaMap are from StorageManager's m_queue.
>+        To be extra clear and careful, I added mutexes on all m_storageAreaMaps in this file.
>+
>+        * NetworkProcess/WebStorage/StorageManager.cpp:
>+        (WebKit::StorageManager::TransientLocalStorageNamespace::getOrCreateStorageArea):
>+        (WebKit::StorageManager::TransientLocalStorageNamespace::origins const):
>+        (WebKit::StorageManager::TransientLocalStorageNamespace::clearStorageAreasMatchingOrigin):
>+        (WebKit::StorageManager::TransientLocalStorageNamespace::clearAllStorageAreas):
>+        (WebKit::StorageManager::LocalStorageNamespace::getOrCreateStorageArea):
>+        (WebKit::StorageManager::LocalStorageNamespace::didDestroyStorageArea):
>+        (WebKit::StorageManager::LocalStorageNamespace::clearStorageAreasMatchingOrigin):
>+        (WebKit::StorageManager::LocalStorageNamespace::clearAllStorageAreas):
>+        (WebKit::StorageManager::LocalStorageNamespace::ephemeralOrigins const):
>+        (WebKit::StorageManager::LocalStorageNamespace::cloneTo):
>+        (WebKit::StorageManager::SessionStorageNamespace::isEmpty const):
>+        (WebKit::StorageManager::SessionStorageNamespace::getOrCreateStorageArea):
>+        (WebKit::StorageManager::SessionStorageNamespace::cloneTo):
>+
> 2019-06-26  Wenson Hsieh  <wenson_hsieh@apple.com>
> 
>         [iPadOS] Fix another crash in -[UIPreviewTarget initWithContainer:center:transform:] when generating a fallback targeted preview
>diff --git a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp
>index 50fde90d615ed58896015c69a2b09152fd2fb27b..02ed4e7bfb6e50b14e45941e92f9674876d8661e 100644
>--- a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp
>+++ b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp
>@@ -109,6 +109,7 @@ private:
>     unsigned m_quotaInBytes;
> 
>     HashMap<SecurityOriginData, RefPtr<StorageArea>> m_storageAreaMap;
>+    mutable Lock m_storageAreaMapMutex;
> };
> 
> // Suggested by https://www.w3.org/TR/webstorage/#disk-space
>@@ -127,6 +128,7 @@ public:
> 
>     Ref<StorageArea> getOrCreateStorageArea(SecurityOriginData&& securityOrigin)
>     {
>+        std::lock_guard<Lock> lock(m_storageAreaMapMutex);
>         return *m_storageAreaMap.ensure(securityOrigin, [this, &securityOrigin]() mutable {
>             return StorageArea::create(nullptr, WTFMove(securityOrigin), m_quotaInBytes);
>         }).iterator->value.copyRef();
>@@ -136,6 +138,7 @@ public:
>     {
>         Vector<SecurityOriginData> origins;
> 
>+        std::lock_guard<Lock> lock(m_storageAreaMapMutex);
>         for (const auto& storageArea : m_storageAreaMap.values()) {
>             if (!storageArea->items().isEmpty())
>                 origins.append(storageArea->securityOrigin());
>@@ -146,6 +149,7 @@ public:
> 
>     void clearStorageAreasMatchingOrigin(const SecurityOriginData& securityOrigin)
>     {
>+        std::lock_guard<Lock> lock(m_storageAreaMapMutex);
>         auto originAndStorageArea = m_storageAreaMap.find(securityOrigin);
>         if (originAndStorageArea != m_storageAreaMap.end())
>             originAndStorageArea->value->clear();
>@@ -153,6 +157,7 @@ public:
> 
>     void clearAllStorageAreas()
>     {
>+        std::lock_guard<Lock> lock(m_storageAreaMapMutex);
>         for (auto& storageArea : m_storageAreaMap.values())
>             storageArea->clear();
>     }
>@@ -165,6 +170,7 @@ private:
>     const unsigned m_quotaInBytes = localStorageDatabaseQuotaInBytes;
> 
>     HashMap<SecurityOriginData, RefPtr<StorageArea>> m_storageAreaMap;
>+    mutable Lock m_storageAreaMapMutex;
> };
> 
> auto StorageManager::StorageArea::create(LocalStorageNamespace* localStorageNamespace, const SecurityOriginData& securityOrigin, unsigned quotaInBytes) -> Ref<StorageManager::StorageArea>
>@@ -362,6 +368,7 @@ StorageManager::LocalStorageNamespace::~LocalStorageNamespace()
> auto StorageManager::LocalStorageNamespace::getOrCreateStorageArea(SecurityOriginData&& securityOrigin, IsEphemeral isEphemeral) -> Ref<StorageArea>
> {
>     RefPtr<StorageArea> protectedStorageArea;
>+    std::lock_guard<Lock> lock(m_storageAreaMapMutex);
>     return *m_storageAreaMap.ensure(securityOrigin, [&]() mutable {
>         protectedStorageArea = StorageArea::create(isEphemeral == IsEphemeral::Yes ? nullptr : this, WTFMove(securityOrigin), m_quotaInBytes);
>         return protectedStorageArea.get();
>@@ -370,11 +377,14 @@ auto StorageManager::LocalStorageNamespace::getOrCreateStorageArea(SecurityOrigi
> 
> void StorageManager::LocalStorageNamespace::didDestroyStorageArea(StorageArea* storageArea)
> {
>-    ASSERT(m_storageAreaMap.contains(storageArea->securityOrigin()));
>+    {
>+        std::lock_guard<Lock> lock(m_storageAreaMapMutex);
>+        ASSERT(m_storageAreaMap.contains(storageArea->securityOrigin()));
> 
>-    m_storageAreaMap.remove(storageArea->securityOrigin());
>-    if (!m_storageAreaMap.isEmpty())
>-        return;
>+        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));
>@@ -383,6 +393,7 @@ void StorageManager::LocalStorageNamespace::didDestroyStorageArea(StorageArea* s
> 
> void StorageManager::LocalStorageNamespace::clearStorageAreasMatchingOrigin(const SecurityOriginData& securityOrigin)
> {
>+    std::lock_guard<Lock> lock(m_storageAreaMapMutex);
>     auto originAndStorageArea = m_storageAreaMap.find(securityOrigin);
>     if (originAndStorageArea != m_storageAreaMap.end())
>         originAndStorageArea->value->clear();
>@@ -390,6 +401,7 @@ void StorageManager::LocalStorageNamespace::clearStorageAreasMatchingOrigin(cons
> 
> void StorageManager::LocalStorageNamespace::clearAllStorageAreas()
> {
>+    std::lock_guard<Lock> lock(m_storageAreaMapMutex);
>     for (auto storageArea : m_storageAreaMap.values())
>         storageArea->clear();
> }
>@@ -397,6 +409,7 @@ void StorageManager::LocalStorageNamespace::clearAllStorageAreas()
> Vector<SecurityOriginData> StorageManager::LocalStorageNamespace::ephemeralOrigins() const
> {
>     Vector<SecurityOriginData> origins;
>+    std::lock_guard<Lock> lock(m_storageAreaMapMutex);
>     for (const auto& storageArea : m_storageAreaMap.values()) {
>         if (!storageArea->items().isEmpty())
>             origins.append(storageArea->securityOrigin());
>@@ -406,6 +419,7 @@ Vector<SecurityOriginData> StorageManager::LocalStorageNamespace::ephemeralOrigi
> 
> void StorageManager::LocalStorageNamespace::cloneTo(LocalStorageNamespace& newLocalStorageNamespace)
> {
>+    std::lock_guard<Lock> lock(m_storageAreaMapMutex);
>     for (auto& pair : m_storageAreaMap)
>         newLocalStorageNamespace.m_storageAreaMap.add(pair.key, pair.value->clone());
> }
>@@ -415,7 +429,7 @@ public:
>     static Ref<SessionStorageNamespace> create(unsigned quotaInBytes);
>     ~SessionStorageNamespace();
> 
>-    bool isEmpty() const { return m_storageAreaMap.isEmpty(); }
>+    bool isEmpty() const;
> 
>     HashSet<IPC::Connection::UniqueID> allowedConnections() const { return m_allowedConnections; }
>     void addAllowedConnection(IPC::Connection::UniqueID);
>@@ -457,8 +471,15 @@ private:
>     unsigned m_quotaInBytes;
> 
>     HashMap<SecurityOriginData, RefPtr<StorageArea>> m_storageAreaMap;
>+    mutable Lock m_storageAreaMapMutex;
> };
> 
>+bool StorageManager::SessionStorageNamespace::isEmpty() const
>+{
>+    std::lock_guard<Lock> lock(m_storageAreaMapMutex);
>+    return m_storageAreaMap.isEmpty();
>+}
>+
> Ref<StorageManager::SessionStorageNamespace> StorageManager::SessionStorageNamespace::create(unsigned quotaInBytes)
> {
>     return adoptRef(*new SessionStorageNamespace(quotaInBytes));
>@@ -486,6 +507,7 @@ void StorageManager::SessionStorageNamespace::removeAllowedConnection(IPC::Conne
> }
> auto StorageManager::SessionStorageNamespace::getOrCreateStorageArea(SecurityOriginData&& securityOrigin) -> Ref<StorageArea>
> {
>+    std::lock_guard<Lock> lock(m_storageAreaMapMutex);
>     return *m_storageAreaMap.ensure(securityOrigin, [this, &securityOrigin]() mutable {
>         return StorageArea::create(nullptr, WTFMove(securityOrigin), m_quotaInBytes);
>     }).iterator->value.copyRef();
>@@ -495,6 +517,7 @@ void StorageManager::SessionStorageNamespace::cloneTo(SessionStorageNamespace& n
> {
>     ASSERT_UNUSED(newSessionStorageNamespace, newSessionStorageNamespace.isEmpty());
> 
>+    std::lock_guard<Lock> lock(m_storageAreaMapMutex);
>     for (auto& pair : m_storageAreaMap)
>         newSessionStorageNamespace.m_storageAreaMap.add(pair.key, pair.value->clone());
> }

Flags:

ggaren: review-

Actions: View | Formatted Diff | Diff

Attachments on bug 199277: 373036