Changeset 247112 in webkit


Ignore:
Timestamp:
Jul 3, 2019 3:16:06 PM (5 years ago)
Author:
Chris Dumez
Message:

Fix a couple of thread safety issues in ResourceLoadStatisticsStore
https://bugs.webkit.org/show_bug.cgi?id=199463

Reviewed by Alex Christensen.

The ResourceLoadStatisticsStore object is constructed / used / destroyed on a background queue.
It is therefore not safe to use a WeakPtr to the ResourceLoadStatisticsStore on the main thread.

The safe pattern is to have the ResourceLoadStatisticsStore capture a Ref<> of its m_store before
dispatching to the main thread and use this store on the main thread instead of weakThis->m_store.
ResourceLoadStatisticsStore's m_store is constructed / used / destroyed on the main thread.

  • NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:

(WebKit::ResourceLoadStatisticsStore::removeDataRecords):
(WebKit::ResourceLoadStatisticsStore::processStatisticsAndDataRecords):

Location:
trunk/Source/WebKit
Files:
2 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebKit/ChangeLog

    r247111 r247112  
     12019-07-03  Chris Dumez  <cdumez@apple.com>
     2
     3        Fix a couple of thread safety issues in ResourceLoadStatisticsStore
     4        https://bugs.webkit.org/show_bug.cgi?id=199463
     5
     6        Reviewed by Alex Christensen.
     7
     8        The ResourceLoadStatisticsStore object is constructed / used / destroyed on a background queue.
     9        It is therefore not safe to use a WeakPtr to the ResourceLoadStatisticsStore on the main thread.
     10
     11        The safe pattern is to have the ResourceLoadStatisticsStore capture a Ref<> of its m_store before
     12        dispatching to the main thread and use this store on the main thread instead of weakThis->m_store.
     13        ResourceLoadStatisticsStore's m_store is constructed / used / destroyed on the main thread.
     14
     15        * NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:
     16        (WebKit::ResourceLoadStatisticsStore::removeDataRecords):
     17        (WebKit::ResourceLoadStatisticsStore::processStatisticsAndDataRecords):
     18
    1192019-07-03  Youenn Fablet  <youenn@apple.com>
    220
  • trunk/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp

    r246569 r247112  
    206206    setDataRecordsBeingRemoved(true);
    207207
    208     RunLoop::main().dispatch([domainsToRemoveWebsiteDataFor = crossThreadCopy(domainsToRemoveWebsiteDataFor), completionHandler = WTFMove(completionHandler), weakThis = makeWeakPtr(*this), shouldNotifyPagesWhenDataRecordsWereScanned = m_parameters.shouldNotifyPagesWhenDataRecordsWereScanned, workQueue = m_workQueue.copyRef()] () mutable {
    209         if (!weakThis) {
    210             completionHandler();
    211             return;
    212         }
    213 
    214         weakThis->m_store.deleteWebsiteDataForRegistrableDomains(WebResourceLoadStatisticsStore::monitoredDataTypes(), WTFMove(domainsToRemoveWebsiteDataFor), shouldNotifyPagesWhenDataRecordsWereScanned, [completionHandler = WTFMove(completionHandler), weakThis = WTFMove(weakThis), workQueue = workQueue.copyRef()](const HashSet<RegistrableDomain>& domainsWithDeletedWebsiteData) mutable {
     208    RunLoop::main().dispatch([store = makeRef(m_store), domainsToRemoveWebsiteDataFor = crossThreadCopy(domainsToRemoveWebsiteDataFor), completionHandler = WTFMove(completionHandler), weakThis = makeWeakPtr(*this), shouldNotifyPagesWhenDataRecordsWereScanned = m_parameters.shouldNotifyPagesWhenDataRecordsWereScanned, workQueue = m_workQueue.copyRef()] () mutable {
     209        store->deleteWebsiteDataForRegistrableDomains(WebResourceLoadStatisticsStore::monitoredDataTypes(), WTFMove(domainsToRemoveWebsiteDataFor), shouldNotifyPagesWhenDataRecordsWereScanned, [completionHandler = WTFMove(completionHandler), weakThis = WTFMove(weakThis), workQueue = workQueue.copyRef()](const HashSet<RegistrableDomain>& domainsWithDeletedWebsiteData) mutable {
    215210            workQueue->dispatch([domainsWithDeletedWebsiteData = crossThreadCopy(domainsWithDeletedWebsiteData), completionHandler = WTFMove(completionHandler), weakThis = WTFMove(weakThis)] () mutable {
    216211                if (!weakThis) {
     
    248243            return;
    249244
    250         RunLoop::main().dispatch([this, weakThis = WTFMove(weakThis)] {
    251             ASSERT(RunLoop::isMain());
    252             if (!weakThis)
    253                 return;
    254 
    255             m_store.notifyResourceLoadStatisticsProcessed();
     245        RunLoop::main().dispatch([store = makeRef(m_store)] {
     246            store->notifyResourceLoadStatisticsProcessed();
    256247        });
    257248    });
Note: See TracChangeset for help on using the changeset viewer.