Attachment 343868 Details for Bug 187143 – Patch

[patch] Patch

bug-187143-20180628155810.patch (text/plain), 10.47 KB, created by Chris Dumez on 2018-06-28 15:57:46 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-06-28 15:57:46 PDT

Size: 10.47 KB

patch

obsolete

>Subversion Revision: 233320
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index c4e00376c2044e8239c3a347a7e62986bb682cef..edd7526c9a1d908d43826f63c343cab51a9ce548 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 Youenn Fablet.
>+
>+        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..6f98e751e59431c04e99987118b19c005696c70e 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,26 @@ 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;
>+    // It is safe to move the origins to the background queue without isolated copy here because this is an r-value
>+    // coming from IPC. ResourceLoadStatistics only contains strings which are safe to move to other threads as long
>+    // as nobody on this thread holds a reference to those strings.
>+    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 +258,17 @@ 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());
>+
>+    // It is safe to move the strings to the background queue without isolated copy here because they are r-value references
>+    // coming from IPC. Strings which are safe to move to other threads as long as nobody on this thread holds a reference
>+    // to those strings.
>+    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 +303,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..8a019a4ac6071d4ebde28b8ca4e494908be6d078 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)>;
>@@ -79,11 +80,8 @@ public:
>     void setShouldClassifyResourcesBeforeDataRecordsRemoval(bool);
>     void setShouldSubmitTelemetry(bool);
> 
>-    void resourceLoadStatisticsUpdated(Vector<WebCore::ResourceLoadStatistics>&& origins);
>-
>     void hasStorageAccess(String&& subFrameHost, String&& topFrameHost, uint64_t frameID, uint64_t pageID, CompletionHandler<void(bool)>&& callback);
>     void requestStorageAccess(String&& subFrameHost, String&& topFrameHost, uint64_t frameID, uint64_t pageID, bool promptEnabled, CompletionHandler<void(StorageAccessStatus)>&&);
>-    void requestStorageAccessUnderOpener(String&& primaryDomainInNeedOfStorageAccess, uint64_t openerPageID, String&& openerPrimaryDomain, bool isTriggeredByUserGesture);
>     void grantStorageAccess(String&& subFrameHost, String&& topFrameHost, uint64_t frameID, uint64_t pageID, bool userWasPromptedNow, CompletionHandler<void(bool)>&&);
>     void requestStorageAccessCallback(bool wasGranted, uint64_t contextId);
> 
>@@ -150,9 +148,13 @@ public:
> private:
>     WebResourceLoadStatisticsStore(const String&, Function<void(const String&)>&& testingCallback, bool isEphemeral, UpdatePrevalentDomainsToPartitionOrBlockCookiesHandler&&, HasStorageAccessForFrameHandler&&, GrantStorageAccessHandler&&, RemoveAllStorageAccessHandler&&, RemovePrevalentDomainsHandler&&);
> 
>-    // IPC::MessageReceiver
>+    // IPC::MessageReceiver.
>     void didReceiveMessage(IPC::Connection&, IPC::Decoder&) override;
> 
>+    // IPC message handlers.
>+    void resourceLoadStatisticsUpdated(Vector<WebCore::ResourceLoadStatistics>&& origins);
>+    void requestStorageAccessUnderOpener(String&& primaryDomainInNeedOfStorageAccess, uint64_t openerPageID, String&& openerPrimaryDomain, bool isTriggeredByUserGesture);
>+
>     void performDailyTasks();
> 
>     StorageAccessStatus storageAccessStatus(const String& subFramePrimaryDomain, const String& topFramePrimaryDomain);

Actions: View | Formatted Diff | Diff

Attachments on bug 187143: 343857 | 343868