Attachment 373415 Details for Bug 199463 – Patch

[patch] Patch

bug-199463-20190703143227.patch (text/plain), 4.22 KB, created by Chris Dumez on 2019-07-03 14:32:27 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2019-07-03 14:32:27 PDT

Size: 4.22 KB

patch

obsolete

>Subversion Revision: 247103
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 842920bf15c8fe261e64a25b903836e6db0b0f92..7a391906332cdb13c01dccb2bcc8ec8fcbbcc242 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,21 @@
>+2019-07-03  Chris Dumez  <cdumez@apple.com>
>+
>+        Fix a couple of thread safety issues in ResourceLoadStatisticsStore
>+        https://bugs.webkit.org/show_bug.cgi?id=199463
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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):
>+
> 2019-07-03  Alex Christensen  <achristensen@webkit.org>
> 
>         Use smarter pointers in WKDownloadProgress
>diff --git a/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp b/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp
>index 26e23723fffc8d612248536bb38874c25a9d4391..753c294373bc4b7b8c69883c8c9c1b085de91424 100644
>--- a/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp
>+++ b/Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp
>@@ -205,13 +205,8 @@ void ResourceLoadStatisticsStore::removeDataRecords(CompletionHandler<void()>&&
> 
>     setDataRecordsBeingRemoved(true);
> 
>-    RunLoop::main().dispatch([domainsToRemoveWebsiteDataFor = crossThreadCopy(domainsToRemoveWebsiteDataFor), completionHandler = WTFMove(completionHandler), weakThis = makeWeakPtr(*this), shouldNotifyPagesWhenDataRecordsWereScanned = m_parameters.shouldNotifyPagesWhenDataRecordsWereScanned, workQueue = m_workQueue.copyRef()] () mutable {
>-        if (!weakThis) {
>-            completionHandler();
>-            return;
>-        }
>-
>-        weakThis->m_store.deleteWebsiteDataForRegistrableDomains(WebResourceLoadStatisticsStore::monitoredDataTypes(), WTFMove(domainsToRemoveWebsiteDataFor), shouldNotifyPagesWhenDataRecordsWereScanned, [completionHandler = WTFMove(completionHandler), weakThis = WTFMove(weakThis), workQueue = workQueue.copyRef()](const HashSet<RegistrableDomain>& domainsWithDeletedWebsiteData) mutable {
>+    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 {
>+        store->deleteWebsiteDataForRegistrableDomains(WebResourceLoadStatisticsStore::monitoredDataTypes(), WTFMove(domainsToRemoveWebsiteDataFor), shouldNotifyPagesWhenDataRecordsWereScanned, [completionHandler = WTFMove(completionHandler), weakThis = WTFMove(weakThis), workQueue = workQueue.copyRef()](const HashSet<RegistrableDomain>& domainsWithDeletedWebsiteData) mutable {
>             workQueue->dispatch([domainsWithDeletedWebsiteData = crossThreadCopy(domainsWithDeletedWebsiteData), completionHandler = WTFMove(completionHandler), weakThis = WTFMove(weakThis)] () mutable {
>                 if (!weakThis) {
>                     completionHandler();
>@@ -247,12 +242,8 @@ void ResourceLoadStatisticsStore::processStatisticsAndDataRecords()
>         if (!m_parameters.shouldNotifyPagesWhenDataRecordsWereScanned)
>             return;
> 
>-        RunLoop::main().dispatch([this, weakThis = WTFMove(weakThis)] {
>-            ASSERT(RunLoop::isMain());
>-            if (!weakThis)
>-                return;
>-
>-            m_store.notifyResourceLoadStatisticsProcessed();
>+        RunLoop::main().dispatch([store = makeRef(m_store)] {
>+            store->notifyResourceLoadStatisticsProcessed();
>         });
>     });
> }

Actions: View | Formatted Diff | Diff

Attachments on bug 199463: 373415