Changeset 246901 in webkit


Ignore:
Timestamp:
Jun 27, 2019 1:46:23 PM (5 years ago)
Author:
sihui_liu@apple.com
Message:

Regression(r246526): StorageManager thread hangs
​https://bugs.webkit.org/show_bug.cgi?id=199278
<rdar://problem/52202948>

Reviewed by Geoffrey Garen.

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:
Location:
trunk/Source/WebKit
Files:
3 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r246896 r246901  
     12019-06-27  Sihui Liu  <sihui_liu@apple.com>
     2
     3        Regression(r246526): StorageManager thread hangs
     4        https://bugs.webkit.org/show_bug.cgi?id=199278
     5        <rdar://problem/52202948>
     6
     7        Reviewed by Geoffrey Garen.
     8
     9        r246526 adds a lock m_localStorageNamespacesMutex to protect m_localStorageNamespaces, because
     10        m_localStorageNamespaces is destroyed at main thread while accesses to m_localStorageNamespaces happen in the
     11        background thread.
     12        After r246526, getOrCreateLocalStorageNamespace acquires lock m_localStorageNamespacesMutex when
     13        m_localStorageNamespacesMutex is already acquired in cloneSessionStorageNamespace, so the StorageManager thread
     14        hangs.
     15        To solve this issue, we can remove the lock in getOrCreateLocalStorageNamespace, or we can remove the
     16        m_localStorageNamespacesMutex. waitUntilWritesFinished() before ~StorageManager() already guarantees nothing
     17        will be running in the background thread, so it is unlikely we the access to m_localStorageNamespaces in the
     18        background thread would collide with the destruction of m_localStorageNamespaces. Also, we don't need
     19        didDestroyStorageArea as LocalStorageNamespace can hold the last reference of StorageArea after r245881.
     20
     21        * NetworkProcess/WebStorage/StorageManager.cpp:
     22        (WebKit::StorageManager::StorageArea::StorageArea):
     23        (WebKit::StorageManager::StorageArea::~StorageArea):
     24        (WebKit::StorageManager::LocalStorageNamespace::LocalStorageNamespace):
     25        (WebKit::StorageManager::cloneSessionStorageNamespace):
     26        (WebKit::StorageManager::getLocalStorageOrigins):
     27        (WebKit::StorageManager::deleteLocalStorageEntriesForOrigin):
     28        (WebKit::StorageManager::deleteLocalStorageOriginsModifiedSince):
     29        (WebKit::StorageManager::deleteLocalStorageEntriesForOrigins):
     30        (WebKit::StorageManager::getOrCreateLocalStorageNamespace):
     31        (WebKit::StorageManager::LocalStorageNamespace::didDestroyStorageArea): Deleted.
     32        * NetworkProcess/WebStorage/StorageManager.h:
     33
    1342019-06-27  Carlos Garcia Campos  <cgarcia@igalia.com>
    235

  • trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp

    r246552 r246901  
    7474
    7575    // Will be null if the storage area belongs to a session storage namespace or the storage area is in an ephemeral session.
    76     LocalStorageNamespace* m_localStorageNamespace;
     76    WeakPtr<LocalStorageNamespace> m_localStorageNamespace;
    7777    mutable RefPtr<LocalStorageDatabase> m_localStorageDatabase;
    7878    mutable bool m_didImportItemsFromDatabase { false };
    … …  
    8585};
    8686
    87 class StorageManager::LocalStorageNamespace : public ThreadSafeRefCounted<LocalStorageNamespace> {
     87class StorageManager::LocalStorageNamespace : public ThreadSafeRefCounted<LocalStorageNamespace>, public CanMakeWeakPtr<LocalStorageNamespace> {
    8888public:
    8989    static Ref<LocalStorageNamespace> create(StorageManager&, uint64_t storageManagerID);
    … …  
    9494    enum class IsEphemeral : bool { No, Yes };
    9595    Ref<StorageArea> getOrCreateStorageArea(SecurityOriginData&&, IsEphemeral);
    96     void didDestroyStorageArea(StorageArea*);
    9796
    9897    void clearStorageAreasMatchingOrigin(const SecurityOriginData&);
    … …  
    106105
    107106    StorageManager& m_storageManager;
    108     uint64_t m_storageNamespaceID;
    109107    unsigned m_quotaInBytes;
    110108
    … …  
    174172
    175173StorageManager::StorageArea::StorageArea(LocalStorageNamespace* localStorageNamespace, const SecurityOriginData& securityOrigin, unsigned quotaInBytes)
    176     : m_localStorageNamespace(localStorageNamespace)
     174    : m_localStorageNamespace(localStorageNamespace ? makeWeakPtr(*localStorageNamespace) : nullptr)
    177175    , m_securityOrigin(securityOrigin)
    178176    , m_quotaInBytes(quotaInBytes)
    … …  
    184182{
    185183    ASSERT(m_eventListeners.isEmpty());
     184    ASSERT(!m_localStorageNamespace);
    186185
    187186    if (m_localStorageDatabase)
    188187        m_localStorageDatabase->close();
    189 
    190     if (m_localStorageNamespace)
    191         m_localStorageNamespace->didDestroyStorageArea(this);
    192188}
    193189
    … …  
    351347StorageManager::LocalStorageNamespace::LocalStorageNamespace(StorageManager& storageManager, uint64_t storageNamespaceID)
    352348    : m_storageManager(storageManager)
    353     , m_storageNamespaceID(storageNamespaceID)
    354349    , m_quotaInBytes(localStorageDatabaseQuotaInBytes)
    355350{
    … …  
    367362        return protectedStorageArea.get();
    368363    }).iterator->value;
    369 }
    370 
    371 void StorageManager::LocalStorageNamespace::didDestroyStorageArea(StorageArea* storageArea)
    372 {
    373     ASSERT(m_storageAreaMap.contains(storageArea->securityOrigin()));
    374 
    375     m_storageAreaMap.remove(storageArea->securityOrigin());
    376     if (!m_storageAreaMap.isEmpty())
    377         return;
    378 
    379     std::lock_guard<Lock> lock(m_storageManager.m_localStorageNamespacesMutex);
    380     ASSERT(m_storageManager.m_localStorageNamespaces.contains(m_storageNamespaceID));
    381     m_storageManager.m_localStorageNamespaces.remove(m_storageNamespaceID);
    382364}
    383365
    … …  
    574556
    575557        if (!m_localStorageDatabaseTracker) {
    576             std::lock_guard<Lock> lock(m_localStorageNamespacesMutex);
    577558            if (auto* localStorageNamespace = m_localStorageNamespaces.get(storageNamespaceID)) {
    578559                LocalStorageNamespace* newlocalStorageNamespace = getOrCreateLocalStorageNamespace(newStorageNamespaceID);
    … …  
    665646                origins.add(origin);
    666647        } else {
    667             std::lock_guard<Lock> lock(m_localStorageNamespacesMutex);
    668648            for (const auto& localStorageNameSpace : m_localStorageNamespaces.values()) {
    669649                for (auto& origin : localStorageNameSpace->ephemeralOrigins())
    … …  
    699679{
    700680    m_queue->dispatch([this, protectedThis = makeRef(*this), copiedOrigin = securityOrigin.isolatedCopy()]() mutable {
    701         {
    702             std::lock_guard<Lock> lock(m_localStorageNamespacesMutex);
    703             for (auto& localStorageNamespace : m_localStorageNamespaces.values())
    704                 localStorageNamespace->clearStorageAreasMatchingOrigin(copiedOrigin);
    705         }
     681        for (auto& localStorageNamespace : m_localStorageNamespaces.values())
     682            localStorageNamespace->clearStorageAreasMatchingOrigin(copiedOrigin);
    706683
    707684        for (auto& transientLocalStorageNamespace : m_transientLocalStorageNamespaces.values())
    … …  
    723700
    724701            for (const auto& origin : originsToDelete) {
    725                 {
    726                     std::lock_guard<Lock> lock(m_localStorageNamespacesMutex);
    727                     for (auto& localStorageNamespace : m_localStorageNamespaces.values())
    728                         localStorageNamespace->clearStorageAreasMatchingOrigin(origin);
    729                 }
     702                for (auto& localStorageNamespace : m_localStorageNamespaces.values())
     703                    localStorageNamespace->clearStorageAreasMatchingOrigin(origin);
    730704               
    731705                m_localStorageDatabaseTracker->deleteDatabaseWithOrigin(origin);
    732706            }
    733707        } else {
    734             std::lock_guard<Lock> lock(m_localStorageNamespacesMutex);
    735708            for (auto& localStorageNamespace : m_localStorageNamespaces.values())
    736709                localStorageNamespace->clearAllStorageAreas();
    … …  
    751724    m_queue->dispatch([this, protectedThis = makeRef(*this), copiedOrigins = WTFMove(copiedOrigins), completionHandler = WTFMove(completionHandler)]() mutable {
    752725        for (auto& origin : copiedOrigins) {
    753             {
    754                 std::lock_guard<Lock> lock(m_localStorageNamespacesMutex);
    755                 for (auto& localStorageNamespace : m_localStorageNamespaces.values())
    756                     localStorageNamespace->clearStorageAreasMatchingOrigin(origin);
    757             }
     726            for (auto& localStorageNamespace : m_localStorageNamespaces.values())
     727                localStorageNamespace->clearStorageAreasMatchingOrigin(origin);
    758728
    759729            for (auto& transientLocalStorageNamespace : m_transientLocalStorageNamespaces.values())
    … …  
    1013983StorageManager::LocalStorageNamespace* StorageManager::getOrCreateLocalStorageNamespace(uint64_t storageNamespaceID)
    1014984{
    1015     std::lock_guard<Lock> lock(m_localStorageNamespacesMutex);
    1016985    if (!m_localStorageNamespaces.isValidKey(storageNamespaceID))
    1017986        return nullptr;

  • trunk/Source/WebKit/NetworkProcess/WebStorage/StorageManager.h

    r246526 r246901  
    106106    RefPtr<LocalStorageDatabaseTracker> m_localStorageDatabaseTracker;
    107107    HashMap<uint64_t, RefPtr<LocalStorageNamespace>> m_localStorageNamespaces;
    108     Lock m_localStorageNamespacesMutex;
    109108
    110109    HashMap<std::pair<uint64_t, WebCore::SecurityOriginData>, RefPtr<TransientLocalStorageNamespace>> m_transientLocalStorageNamespaces;
Note: See TracChangeset for help on using the changeset viewer.