Attachment 343857 Details for Bug 187143 – Patch

[patch] Patch

bug-187143-20180628150904.patch (text/plain), 8.03 KB, created by Chris Dumez on 2018-06-28 15:08:39 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-06-28 15:08:39 PDT

Size: 8.03 KB

patch

obsolete

>Subversion Revision: 233320
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index c4e00376c2044e8239c3a347a7e62986bb682cef..b8a0e8ad08b8e9ee66956412fc30639e5607add5 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,27 @@
>+2018-06-28  Chris Dumez  <cdumez@apple.com>
>+
>+        Make sure the WebResourceLoadStatisticsStore gets destroyed on the main thread
>+        https://bugs.webkit.org/show_bug.cgi?id=187143
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Have WebResourceLoadStatisticsStore subclass ThreadSafeRefCounted<WebResourceLoadStatisticsStore, WTF::DestructionThread::Main>
>+        instead of IPC::Connection::WorkQueueMessageReceiver. This makes sure that the WebResourceLoadStatisticsStore
>+        objects get destroyed on the main thread, even if the last ref was held by a background thread.
>+
>+        Also, methods called by IPC are now called on the main thread instead of the background queue. I think it is clearer for all
>+        of WebResourceLoadStatisticsStore usage to be on the main thread. Expensive work is still done on the background queue, inside
>+        the persistent / memory store classes.
>+
>+        * UIProcess/WebResourceLoadStatisticsStore.cpp:
>+        (WebKit::WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore):
>+        (WebKit::WebResourceLoadStatisticsStore::flushAndDestroyPersistentStore):
>+        (WebKit::WebResourceLoadStatisticsStore::resourceLoadStatisticsUpdated):
>+        (WebKit::WebResourceLoadStatisticsStore::requestStorageAccessUnderOpener):
>+        (WebKit::WebResourceLoadStatisticsStore::processWillOpenConnection):
>+        (WebKit::WebResourceLoadStatisticsStore::processDidCloseConnection):
>+        * UIProcess/WebResourceLoadStatisticsStore.h:
>+
> 2018-06-28  Chris Dumez  <cdumez@apple.com>
> 
>         Split memory store logic out of WebResourceLoadStatisticsStore to clarify threading model
>diff --git a/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp b/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp
>index f36450126adadf4dffe892f741a46c7a50e8346d..382d154454088c317e888f0cbb23af5fb843438c 100644
>--- a/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp
>+++ b/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp
>@@ -138,11 +138,15 @@ WebResourceLoadStatisticsStore::WebResourceLoadStatisticsStore(const String& res
> 
> WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore()
> {
>+    ASSERT(RunLoop::isMain());
>+
>     flushAndDestroyPersistentStore();
> }
> 
> void WebResourceLoadStatisticsStore::flushAndDestroyPersistentStore()
> {
>+    ASSERT(RunLoop::isMain());
>+
>     if (!m_persistentStorage && !m_memoryStore)
>         return;
> 
>@@ -177,22 +181,23 @@ void WebResourceLoadStatisticsStore::scheduleStatisticsAndDataRecordsProcessing(
>     });
> }
> 
>-// On background queue due to IPC.
> void WebResourceLoadStatisticsStore::resourceLoadStatisticsUpdated(Vector<WebCore::ResourceLoadStatistics>&& origins)
> {
>-    ASSERT(!RunLoop::isMain());
>+    ASSERT(RunLoop::isMain());
> 
>-    if (!m_memoryStore)
>-        return;
>+    m_statisticsQueue->dispatch([this, protectedThis = makeRef(*this), origins = WTFMove(origins)]() mutable {
>+        if (!m_memoryStore)
>+            return;
> 
>-    m_memoryStore->mergeStatistics(WTFMove(origins));
>+        m_memoryStore->mergeStatistics(WTFMove(origins));
> 
>-    // We can cancel any pending request to process statistics since we're doing it synchronously below.
>-    m_memoryStore->cancelPendingStatisticsProcessingRequest();
>+        // We can cancel any pending request to process statistics since we're doing it synchronously below.
>+        m_memoryStore->cancelPendingStatisticsProcessingRequest();
> 
>-    // Fire before processing statistics to propagate user interaction as fast as possible to the network process.
>-    m_memoryStore->updateCookiePartitioning([]() { });
>-    m_memoryStore->processStatisticsAndDataRecords();
>+        // Fire before processing statistics to propagate user interaction as fast as possible to the network process.
>+        m_memoryStore->updateCookiePartitioning([]() { });
>+        m_memoryStore->processStatisticsAndDataRecords();
>+    });
> }
> 
> void WebResourceLoadStatisticsStore::hasStorageAccess(String&& subFrameHost, String&& topFrameHost, uint64_t frameID, uint64_t pageID, CompletionHandler<void (bool)>&& completionHandler)
>@@ -250,12 +255,14 @@ void WebResourceLoadStatisticsStore::requestStorageAccess(String&& subFrameHost,
>     });
> }
> 
>-// On background queue due to IPC.
> void WebResourceLoadStatisticsStore::requestStorageAccessUnderOpener(String&& primaryDomainInNeedOfStorageAccess, uint64_t openerPageID, String&& openerPrimaryDomain, bool isTriggeredByUserGesture)
> {
>-    ASSERT(!RunLoop::isMain());
>-    if (m_memoryStore)
>-        m_memoryStore->requestStorageAccessUnderOpener(WTFMove(primaryDomainInNeedOfStorageAccess), openerPageID, WTFMove(openerPrimaryDomain), isTriggeredByUserGesture);
>+    ASSERT(RunLoop::isMain());
>+
>+    m_statisticsQueue->dispatch([this, protectedThis = makeRef(*this), primaryDomainInNeedOfStorageAccess = WTFMove(primaryDomainInNeedOfStorageAccess), openerPageID, openerPrimaryDomain = WTFMove(openerPrimaryDomain), isTriggeredByUserGesture]() mutable {
>+        if (m_memoryStore)
>+            m_memoryStore->requestStorageAccessUnderOpener(WTFMove(primaryDomainInNeedOfStorageAccess), openerPageID, WTFMove(openerPrimaryDomain), isTriggeredByUserGesture);
>+    });
> }
> 
> void WebResourceLoadStatisticsStore::grantStorageAccess(String&& subFrameHost, String&& topFrameHost, uint64_t frameID, uint64_t pageID, bool userWasPromptedNow, CompletionHandler<void(bool)>&& completionHandler)
>@@ -290,15 +297,16 @@ void WebResourceLoadStatisticsStore::removeAllStorageAccess()
> 
>     m_removeAllStorageAccessHandler();
> }
>-    
>-void WebResourceLoadStatisticsStore::processWillOpenConnection(WebProcessProxy&, IPC::Connection& connection)
>+
>+
>+void WebResourceLoadStatisticsStore::processWillOpenConnection(WebProcessProxy& process, IPC::Connection&)
> {
>-    connection.addWorkQueueMessageReceiver(Messages::WebResourceLoadStatisticsStore::messageReceiverName(), m_statisticsQueue.get(), this);
>+    process.addMessageReceiver(Messages::WebResourceLoadStatisticsStore::messageReceiverName(), *this);
> }
> 
>-void WebResourceLoadStatisticsStore::processDidCloseConnection(WebProcessProxy&, IPC::Connection& connection)
>+void WebResourceLoadStatisticsStore::processDidCloseConnection(WebProcessProxy& process, IPC::Connection&)
> {
>-    connection.removeWorkQueueMessageReceiver(Messages::WebResourceLoadStatisticsStore::messageReceiverName());
>+    process.removeMessageReceiver(Messages::WebResourceLoadStatisticsStore::messageReceiverName());
> }
> 
> void WebResourceLoadStatisticsStore::applicationWillTerminate()
>diff --git a/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h b/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h
>index 4504e89071478d97e238878cd81f22e9030a8a21..d258f06e6506d68c2e382329e657b0d9ecf8608d 100644
>--- a/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h
>+++ b/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h
>@@ -29,6 +29,7 @@
> #include "WebsiteDataType.h"
> #include <wtf/CompletionHandler.h>
> #include <wtf/RunLoop.h>
>+#include <wtf/ThreadSafeRefCounted.h>
> #include <wtf/Vector.h>
> #include <wtf/WallTime.h>
> #include <wtf/text/WTFString.h>
>@@ -57,7 +58,7 @@ enum class StorageAccessStatus {
>     HasAccess
> };
> 
>-class WebResourceLoadStatisticsStore final : public IPC::Connection::WorkQueueMessageReceiver {
>+class WebResourceLoadStatisticsStore final : public ThreadSafeRefCounted<WebResourceLoadStatisticsStore, WTF::DestructionThread::Main>, private IPC::MessageReceiver {
> public:
>     using UpdatePrevalentDomainsToPartitionOrBlockCookiesHandler = Function<void(const Vector<String>& domainsToPartition, const Vector<String>& domainsToBlock, const Vector<String>& domainsToNeitherPartitionNorBlock, ShouldClearFirst, CompletionHandler<void()>&&)>;
>     using HasStorageAccessForFrameHandler = Function<void(const String& resourceDomain, const String& firstPartyDomain, uint64_t frameID, uint64_t pageID, Function<void(bool hasAccess)>&& callback)>;

Actions: View | Formatted Diff | Diff

Attachments on bug 187143: 343857 | 343868