Changeset 247784 in webkit


Ignore:
Timestamp:
Jul 24, 2019 12:21:49 PM (5 years ago)
Author:
Chris Dumez
Message:

Crash under WebKit:WTF::Detail::CallableWrapper<WebKit::ResourceLoadStatisticsMemoryStore::updateCookieBlocking(WTF::CompletionHandler<void ()>&&)::$_32::operator()()::'lambda'(), void>::call
https://bugs.webkit.org/show_bug.cgi?id=200071
<rdar://problem/53335583>

Reviewed by Brent Fulgham and Youenn Fablet.

The WebResourceLoadStatisticsStore is a main thread object. In its destructor, it was dispatching
to the background queue to destroy the m_statisticsStore / m_persistentStorage data members, which
live on the background queue. It would then synchronously wait for the background queue to finish
destroying them. The idea was to guarantee that the ResourceLoadStatisticsMemoryStore and the
ResourceLoadStatisticsPersistentStorage would never outlive the WebResourceLoadStatisticsStore,
given that they keep a raw pointer back to the WebResourceLoadStatisticsStore (via m_store data
member).

The issue is that *while* the WebResourceLoadStatisticsStore destructor is running on the main
thread, the background queue may be running code in ResourceLoadStatisticsMemoryStore or
ResourceLoadStatisticsPersistentStorage which refs the WebResourceLoadStatisticsStore, even
though its ref count has already reached 0. It is actually a common pattern in
ResourceLoadStatisticsMemoryStore to call RunLoop::main().dispatch() and ref their m_store in
the lambda, so that they can interact with the WebResourceLoadStatisticsStore.

To address the issue, we now destroy m_statisticsStore / m_persistentStorage *before* the
WebResourceLoadStatisticsStore destructor runs. The NetworkSession destructor now calls
WebResourceLoadStatisticsStore::didDestroyNetworkSession() which takes care of destroying
m_statisticsStore / m_persistentStorage on the background queue, synchronously. The
WebResourceLoadStatisticsStore destructor will only run later, once all remaining references
to it are gone.

  • NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:

(WebKit::WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore):
(WebKit::WebResourceLoadStatisticsStore::didDestroyNetworkSession):

  • NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:
  • NetworkProcess/NetworkSession.cpp:

(WebKit::NetworkSession::~NetworkSession):

Location:
trunk/Source/WebKit
Files:
5 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebKit/ChangeLog

    r247782 r247784  
     12019-07-24  Chris Dumez  <cdumez@apple.com>
     2
     3        Crash under WebKit:WTF::Detail::CallableWrapper<WebKit::ResourceLoadStatisticsMemoryStore::updateCookieBlocking(WTF::CompletionHandler<void ()>&&)::$_32::operator()()::'lambda'(), void>::call
     4        https://bugs.webkit.org/show_bug.cgi?id=200071
     5        <rdar://problem/53335583>
     6
     7        Reviewed by Brent Fulgham and Youenn Fablet.
     8
     9        The WebResourceLoadStatisticsStore is a main thread object. In its destructor, it was dispatching
     10        to the background queue to destroy the m_statisticsStore / m_persistentStorage data members, which
     11        live on the background queue. It would then synchronously wait for the background queue to finish
     12        destroying them. The idea was to guarantee that the ResourceLoadStatisticsMemoryStore and the
     13        ResourceLoadStatisticsPersistentStorage would never outlive the WebResourceLoadStatisticsStore,
     14        given that they keep a raw pointer back to the WebResourceLoadStatisticsStore (via m_store data
     15        member).
     16
     17        The issue is that *while* the WebResourceLoadStatisticsStore destructor is running on the main
     18        thread, the background queue may be running code in ResourceLoadStatisticsMemoryStore or
     19        ResourceLoadStatisticsPersistentStorage which refs the WebResourceLoadStatisticsStore, even
     20        though its ref count has already reached 0. It is actually a common pattern in
     21        ResourceLoadStatisticsMemoryStore to call RunLoop::main().dispatch() and ref their m_store in
     22        the lambda, so that they can interact with the WebResourceLoadStatisticsStore.
     23
     24        To address the issue, we now destroy m_statisticsStore / m_persistentStorage *before* the
     25        WebResourceLoadStatisticsStore destructor runs. The NetworkSession destructor now calls
     26        WebResourceLoadStatisticsStore::didDestroyNetworkSession() which takes care of destroying
     27        m_statisticsStore / m_persistentStorage on the background queue, synchronously. The
     28        WebResourceLoadStatisticsStore destructor will only run later, once all remaining references
     29        to it are gone.
     30
     31        * NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:
     32        (WebKit::WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore):
     33        (WebKit::WebResourceLoadStatisticsStore::didDestroyNetworkSession):
     34        * NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:
     35        * NetworkProcess/NetworkSession.cpp:
     36        (WebKit::NetworkSession::~NetworkSession):
     37
    1382019-07-24  Youenn Fablet  <youenn@apple.com>
    239
  • trunk/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp

    r247769 r247784  
    181181{
    182182    ASSERT(RunLoop::isMain());
    183 
     183    ASSERT(!m_statisticsStore);
     184    ASSERT(!m_persistentStorage);
     185}
     186
     187void WebResourceLoadStatisticsStore::didDestroyNetworkSession()
     188{
     189    ASSERT(RunLoop::isMain());
     190
     191    m_networkSession = nullptr;
    184192    flushAndDestroyPersistentStore();
    185193}
     
    336344        case StorageAccessStatus::RequiresUserPrompt:
    337345            {
     346            if (!m_networkSession)
     347                return completionHandler(StorageAccessWasGranted::No, StorageAccessPromptWasShown::No);
     348
    338349            CompletionHandler<void(bool)> requestConfirmationCompletionHandler = [this, protectedThis = protectedThis.copyRef(), subFrameDomain, topFrameDomain, frameID, pageID, completionHandler = WTFMove(completionHandler)] (bool userDidGrantAccess) mutable {
    339350                if (userDidGrantAccess)
     
    359370        }
    360371
    361         if (m_statisticsStore) {
    362             m_statisticsStore->requestStorageAccess(WTFMove(subFrameDomain), WTFMove(topFrameDomain), frameID, pageID, [statusHandler = WTFMove(statusHandler)](StorageAccessStatus status) mutable {
    363                 postTaskReply([statusHandler = WTFMove(statusHandler), status]() mutable {
    364                     statusHandler(status);
    365                 });
     372        m_statisticsStore->requestStorageAccess(WTFMove(subFrameDomain), WTFMove(topFrameDomain), frameID, pageID, [statusHandler = WTFMove(statusHandler)](StorageAccessStatus status) mutable {
     373            postTaskReply([statusHandler = WTFMove(statusHandler), status]() mutable {
     374                statusHandler(status);
    366375            });
    367         }
     376        });
    368377    });
    369378}
  • trunk/Source/WebKit/NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h

    r247769 r247784  
    9494
    9595    ~WebResourceLoadStatisticsStore();
     96
     97    void didDestroyNetworkSession();
    9698
    9799    static const OptionSet<WebsiteDataType>& monitoredDataTypes();
  • trunk/Source/WebKit/NetworkProcess/NetworkSession.cpp

    r247769 r247784  
    108108NetworkSession::~NetworkSession()
    109109{
     110#if ENABLE(RESOURCE_LOAD_STATISTICS)
     111    destroyResourceLoadStatistics();
     112#endif
     113
    110114    m_storageManager->resume();
    111115    m_storageManager->waitUntilTasksFinished();
    112116}
    113117
     118#if ENABLE(RESOURCE_LOAD_STATISTICS)
     119void NetworkSession::destroyResourceLoadStatistics()
     120{
     121    if (!m_resourceLoadStatistics)
     122        return;
     123
     124    m_resourceLoadStatistics->didDestroyNetworkSession();
     125    m_resourceLoadStatistics = nullptr;
     126}
     127#endif
     128
    114129void NetworkSession::invalidateAndCancel()
    115130{
     
    130145    ASSERT(!m_isInvalidated);
    131146    if (!enable) {
    132         m_resourceLoadStatistics = nullptr;
     147        destroyResourceLoadStatistics();
    133148        return;
    134149    }
  • trunk/Source/WebKit/NetworkProcess/NetworkSession.h

    r247567 r247784  
    117117    NetworkSession(NetworkProcess&, const NetworkSessionCreationParameters&);
    118118
     119#if ENABLE(RESOURCE_LOAD_STATISTICS)
     120    void destroyResourceLoadStatistics();
     121#endif
     122
    119123    PAL::SessionID m_sessionID;
    120124    Ref<NetworkProcess> m_networkProcess;
Note: See TracChangeset for help on using the changeset viewer.