WebKit Bugzilla
Attachment 373039 Details for
Bug 199278
: Regression(r246526): StorageManager thread hangs
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[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:
Patch
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; >
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 199278
: 373039