Attachment 348434 Details for Bug 189098 – Patch

[patch] Patch

bug-189098-20180829144613.patch (text/plain), 9.12 KB, created by Chris Dumez on 2018-08-29 14:46:13 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-08-29 14:46:13 PDT

Size: 9.12 KB

patch

obsolete

>Subversion Revision: 235479
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index b2a2b6a7800e94c17e0b59a8c67c71a2fc741b91..26c5300dd6e038e622fe1fe271c0cc3901347bd8 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,22 @@
>+2018-08-29  Chris Dumez  <cdumez@apple.com>
>+
>+        Crash under WebKit: WTF::Function<void ()>::CallableWrapper<WebKit::ResourceLoadStatisticsMemoryStore::removeDataRecords(WTF::CompletionHandler<void ()>&&)::$_1>::call()
>+        https://bugs.webkit.org/show_bug.cgi?id=189098
>+        <rdar://problem/43179891>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        The crash was caused by implicitly using |this| on the main thread by accessing member variables, even though
>+        |this| gets destroyed on the statistics queue. To address the issue, capture what we need on the statistics
>+        queue, *before* dispatching to the main thread.
>+
>+        Also stop capturing |this| in the lambdas to make this less error prone.
>+
>+        * UIProcess/ResourceLoadStatisticsMemoryStore.cpp:
>+        (WebKit::ResourceLoadStatisticsMemoryStore::removeDataRecords):
>+        (WebKit::ResourceLoadStatisticsMemoryStore::grandfatherExistingWebsiteData):
>+        (WebKit::ResourceLoadStatisticsMemoryStore::updateCookieBlocking):
>+
> 2018-08-29  Aditya Keerthi  <akeerthi@apple.com>
> 
>         Followup (r235427): Use the null string instead of std::nullopt when no suggestion is selected
>diff --git a/Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp b/Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp
>index 6dfc918a5d1da6b135982962df11ed0b39beb01d..e589f85bc750b26c7345f1f27fbf0a75bc1cbdf2 100644
>--- a/Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp
>+++ b/Source/WebKit/UIProcess/ResourceLoadStatisticsMemoryStore.cpp
>@@ -244,21 +244,21 @@ void ResourceLoadStatisticsMemoryStore::removeDataRecords(CompletionHandler<void
> 
>     setDataRecordsBeingRemoved(true);
> 
>-    RunLoop::main().dispatch([prevalentResourceDomains = crossThreadCopy(prevalentResourceDomains), callback = WTFMove(callback), this, weakThis = makeWeakPtr(*this)] () mutable {
>-        WebProcessProxy::deleteWebsiteDataForTopPrivatelyControlledDomainsInAllPersistentDataStores(WebResourceLoadStatisticsStore::monitoredDataTypes(), WTFMove(prevalentResourceDomains), m_parameters.shouldNotifyPagesWhenDataRecordsWereScanned, [callback = WTFMove(callback), this, weakThis = WTFMove(weakThis), workQueue = m_workQueue.copyRef()](const HashSet<String>& domainsWithDeletedWebsiteData) mutable {
>-            workQueue->dispatch([topDomains = crossThreadCopy(domainsWithDeletedWebsiteData), callback = WTFMove(callback), this, weakThis = WTFMove(weakThis)] () mutable {
>+    RunLoop::main().dispatch([prevalentResourceDomains = crossThreadCopy(prevalentResourceDomains), callback = WTFMove(callback), weakThis = makeWeakPtr(*this), shouldNotifyPagesWhenDataRecordsWereScanned = m_parameters.shouldNotifyPagesWhenDataRecordsWereScanned, workQueue = m_workQueue.copyRef()] () mutable {
>+        WebProcessProxy::deleteWebsiteDataForTopPrivatelyControlledDomainsInAllPersistentDataStores(WebResourceLoadStatisticsStore::monitoredDataTypes(), WTFMove(prevalentResourceDomains), shouldNotifyPagesWhenDataRecordsWereScanned, [callback = WTFMove(callback), weakThis = WTFMove(weakThis), workQueue = workQueue.copyRef()](const HashSet<String>& domainsWithDeletedWebsiteData) mutable {
>+            workQueue->dispatch([topDomains = crossThreadCopy(domainsWithDeletedWebsiteData), callback = WTFMove(callback), weakThis = WTFMove(weakThis)] () mutable {
>                 if (!weakThis) {
>                     callback();
>                     return;
>                 }
>                 for (auto& prevalentResourceDomain : topDomains) {
>-                    auto& statistic = ensureResourceStatisticsForPrimaryDomain(prevalentResourceDomain);
>+                    auto& statistic = weakThis->ensureResourceStatisticsForPrimaryDomain(prevalentResourceDomain);
>                     ++statistic.dataRecordsRemoved;
>                 }
>-                setDataRecordsBeingRemoved(false);
>+                weakThis->setDataRecordsBeingRemoved(false);
>                 callback();
> #if !RELEASE_LOG_DISABLED
>-                RELEASE_LOG_INFO_IF(m_debugLoggingEnabled, ResourceLoadStatisticsDebug, "Done removing data records.");
>+                RELEASE_LOG_INFO_IF(weakThis->m_debugLoggingEnabled, ResourceLoadStatisticsDebug, "Done removing data records.");
> #endif
>             });
>         });
>@@ -506,25 +506,25 @@ void ResourceLoadStatisticsMemoryStore::grandfatherExistingWebsiteData(Completio
> {
>     ASSERT(!RunLoop::isMain());
> 
>-    RunLoop::main().dispatch([this, weakThis = makeWeakPtr(*this), callback = WTFMove(callback)] () mutable {
>+    RunLoop::main().dispatch([weakThis = makeWeakPtr(*this), callback = WTFMove(callback), shouldNotifyPagesWhenDataRecordsWereScanned = m_parameters.shouldNotifyPagesWhenDataRecordsWereScanned, workQueue = m_workQueue.copyRef()] () mutable {
>         // FIXME: This method being a static call on WebProcessProxy is wrong.
>         // It should be on the data store that this object belongs to.
>-        WebProcessProxy::topPrivatelyControlledDomainsWithWebsiteData(WebResourceLoadStatisticsStore::monitoredDataTypes(), m_parameters.shouldNotifyPagesWhenDataRecordsWereScanned, [this, weakThis = WTFMove(weakThis), callback = WTFMove(callback), workQueue = m_workQueue.copyRef()] (HashSet<String>&& topPrivatelyControlledDomainsWithWebsiteData) mutable {
>-            workQueue->dispatch([this, weakThis = WTFMove(weakThis), topDomains = crossThreadCopy(topPrivatelyControlledDomainsWithWebsiteData), callback = WTFMove(callback)] () mutable {
>+        WebProcessProxy::topPrivatelyControlledDomainsWithWebsiteData(WebResourceLoadStatisticsStore::monitoredDataTypes(), shouldNotifyPagesWhenDataRecordsWereScanned, [weakThis = WTFMove(weakThis), callback = WTFMove(callback), workQueue = workQueue.copyRef()] (HashSet<String>&& topPrivatelyControlledDomainsWithWebsiteData) mutable {
>+            workQueue->dispatch([weakThis = WTFMove(weakThis), topDomains = crossThreadCopy(topPrivatelyControlledDomainsWithWebsiteData), callback = WTFMove(callback)] () mutable {
>                 if (!weakThis) {
>                     callback();
>                     return;
>                 }
> 
>                 for (auto& topPrivatelyControlledDomain : topDomains) {
>-                    auto& statistic = ensureResourceStatisticsForPrimaryDomain(topPrivatelyControlledDomain);
>+                    auto& statistic = weakThis->ensureResourceStatisticsForPrimaryDomain(topPrivatelyControlledDomain);
>                     statistic.grandfathered = true;
>                 }
>-                m_endOfGrandfatheringTimestamp = WallTime::now() + m_parameters.grandfatheringTime;
>-                if (m_persistentStorage)
>-                    m_persistentStorage->scheduleOrWriteMemoryStore(ResourceLoadStatisticsPersistentStorage::ForceImmediateWrite::Yes);
>+                weakThis->m_endOfGrandfatheringTimestamp = WallTime::now() + weakThis->m_parameters.grandfatheringTime;
>+                if (weakThis->m_persistentStorage)
>+                    weakThis->m_persistentStorage->scheduleOrWriteMemoryStore(ResourceLoadStatisticsPersistentStorage::ForceImmediateWrite::Yes);
>                 callback();
>-                logTestingEvent("Grandfathered"_s);
>+                weakThis->logTestingEvent("Grandfathered"_s);
>             });
>         });
>     });
>@@ -1039,14 +1039,14 @@ void ResourceLoadStatisticsMemoryStore::updateCookieBlocking(CompletionHandler<v
>     if (m_debugLoggingEnabled && !domainsToBlock.isEmpty())
>             debugLogDomainsInBatches("block", domainsToBlock);
> 
>-    RunLoop::main().dispatch([this, weakThis = makeWeakPtr(*this), store = makeRef(m_store), domainsToBlock = crossThreadCopy(domainsToBlock), completionHandler = WTFMove(completionHandler)] () mutable {
>-        store->callUpdatePrevalentDomainsToBlockCookiesForHandler(domainsToBlock, ShouldClearFirst::No, [this, weakThis = WTFMove(weakThis), store = store.copyRef(), completionHandler = WTFMove(completionHandler)]() mutable {
>-            store->statisticsQueue().dispatch([this, weakThis = WTFMove(weakThis), completionHandler = WTFMove(completionHandler)]() mutable {
>+    RunLoop::main().dispatch([weakThis = makeWeakPtr(*this), store = makeRef(m_store), domainsToBlock = crossThreadCopy(domainsToBlock), completionHandler = WTFMove(completionHandler)] () mutable {
>+        store->callUpdatePrevalentDomainsToBlockCookiesForHandler(domainsToBlock, ShouldClearFirst::No, [weakThis = WTFMove(weakThis), store = store.copyRef(), completionHandler = WTFMove(completionHandler)]() mutable {
>+            store->statisticsQueue().dispatch([weakThis = WTFMove(weakThis), completionHandler = WTFMove(completionHandler)]() mutable {
>                 completionHandler();
>                 if (!weakThis)
>                     return;
> #if !RELEASE_LOG_DISABLED
>-                RELEASE_LOG_INFO_IF(m_debugLoggingEnabled, ResourceLoadStatisticsDebug, "Done updating cookie blocking.");
>+                RELEASE_LOG_INFO_IF(weakThis->m_debugLoggingEnabled, ResourceLoadStatisticsDebug, "Done updating cookie blocking.");
> #endif
>             });
>         });

Actions: View | Formatted Diff | Diff

Attachments on bug 189098: 348434