Attachment 373039 Details for Bug 199278 – Patch

[patch] Patch

bug-199278-20190627111637.patch (text/plain), 11.86 KB, created by Sihui Liu on 2019-06-27 11:16:37 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Sihui Liu

Created: 2019-06-27 11:16:37 PDT

Size: 11.86 KB

patch

obsolete

>Subversion Revision: 246840
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 0b692e71fc4dd6bb1949f7b33aa2da7a177a29c3..a8a150cb0a91b3db7cd71a34f552736ad943df1e 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,36 @@
>+2019-06-27  Sihui Liu  <sihui_liu@apple.com>
>+
>+        Regression(r246526): StorageManager thread hangs
>+        https://bugs.webkit.org/show_bug.cgi?id=199278
>+        <rdar://problem/52202948>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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-26  Zalan Bujtas  <zalan@apple.com>
> 
>         [ContentChangeObserver] Dispatch synthetic mouse event asynchronously in completePendingSyntheticClickForContentChangeObserver
>diff --git a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp
>index 50fde90d615ed58896015c69a2b09152fd2fb27b..211f823046dd876c5357c89009968d692f4e4c0e 100644
>--- a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp
>+++ b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp
>@@ -73,7 +73,7 @@ private:
>     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 @@ private:
>     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 @@ public:
> 
>     enum class IsEphemeral : bool { No, Yes };
>     Ref<StorageArea> getOrCreateStorageArea(SecurityOriginData&&, IsEphemeral);
>-    void didDestroyStorageArea(StorageArea*);
> 
>     void clearStorageAreasMatchingOrigin(const SecurityOriginData&);
>     void clearAllStorageAreas();
>@@ -105,7 +104,6 @@ private:
>     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 @@ auto StorageManager::StorageArea::create(LocalStorageNamespace* localStorageName
> }
> 
> 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(LocalStorageNamespace* localStorageName
> 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 @@ Ref<StorageManager::LocalStorageNamespace> StorageManager::LocalStorageNamespace
> // 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 @@ auto StorageManager::LocalStorageNamespace::getOrCreateStorageArea(SecurityOrigi
>     }).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 @@ 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);
>@@ -664,7 +645,6 @@ 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);
>@@ -698,11 +678,8 @@ void StorageManager::getLocalStorageOriginDetails(Function<void(Vector<LocalStor
> 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 @@ void StorageManager::deleteLocalStorageOriginsModifiedSince(WallTime time, Funct
>                 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 @@ 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) {
>-            {
>-                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::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;
> 
>diff --git a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h
>index c71cc68633d79584390cc7676bddc8db268de8dc..ef19d5b80e6ab9a824f5f62ea424998493e482f4 100644
>--- a/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h
>+++ b/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h
>@@ -105,7 +105,6 @@ 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;
>

Actions: View | Formatted Diff | Diff

Attachments on bug 199278: 373039