- Revision
- 233342
- Author
- [email protected]
- Date
- 2018-06-28 17:05:59 -0700 (Thu, 28 Jun 2018)
Log Message
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:
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (233341 => 233342)
--- trunk/Source/WebKit/ChangeLog 2018-06-28 23:57:57 UTC (rev 233341)
+++ trunk/Source/WebKit/ChangeLog 2018-06-29 00:05:59 UTC (rev 233342)
@@ -1,3 +1,27 @@
+2018-06-28 Chris Dumez <[email protected]>
+
+ 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 Jiewen Tan <[email protected]>
Add nullptr check for xpc_connection_t in AuthenticationManager::initializeConnection
Modified: trunk/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp (233341 => 233342)
--- trunk/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp 2018-06-28 23:57:57 UTC (rev 233341)
+++ trunk/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.cpp 2018-06-29 00:05:59 UTC (rev 233342)
@@ -138,11 +138,15 @@
WebResourceLoadStatisticsStore::~WebResourceLoadStatisticsStore()
{
+ ASSERT(RunLoop::isMain());
+
flushAndDestroyPersistentStore();
}
void WebResourceLoadStatisticsStore::flushAndDestroyPersistentStore()
{
+ ASSERT(RunLoop::isMain());
+
if (!m_persistentStorage && !m_memoryStore)
return;
@@ -177,22 +181,26 @@
});
}
-// 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 @@
});
}
-// 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 @@
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()
Modified: trunk/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h (233341 => 233342)
--- trunk/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h 2018-06-28 23:57:57 UTC (rev 233341)
+++ trunk/Source/WebKit/UIProcess/WebResourceLoadStatisticsStore.h 2018-06-29 00:05:59 UTC (rev 233342)
@@ -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 @@
HasAccess
};
-class WebResourceLoadStatisticsStore final : public IPC::Connection::WorkQueueMessageReceiver {
+class WebResourceLoadStatisticsStore final : public ThreadSafeRefCounted<WebResourceLoadStatisticsStore, WTF::DestructionThread::Main>, private IPC::MessageReceiver {
public:
using UpdatePrevalentDomainsToPartitionOrBlockCookiesHandler = WTF::Function<void(const WTF::Vector<String>& domainsToPartition, const WTF::Vector<String>& domainsToBlock, const WTF::Vector<String>& domainsToNeitherPartitionNorBlock, ShouldClearFirst, CompletionHandler<void()>&&)>;
using HasStorageAccessForFrameHandler = WTF::Function<void(const String& resourceDomain, const String& firstPartyDomain, uint64_t frameID, uint64_t pageID, WTF::Function<void(bool hasAccess)>&& callback)>;
@@ -79,11 +80,8 @@
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 @@
private:
WebResourceLoadStatisticsStore(const String&, WTF::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);
