Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
Resource Load Statistics data summary does not report data which is h…
…eld up in the web content process.

https://bugs.webkit.org/show_bug.cgi?id=215822
<rdar://problem/66682044>

Reviewed by Chris Dumez.

Source/WebCore:

Send empty lambda when calling updateCentralStatisticsStore() because
in these cases we don't care about timing.

* loader/ResourceLoadObserver.h:
(WebCore::ResourceLoadObserver::updateCentralStatisticsStore):
* page/DOMWindow.cpp:
(WebCore::DOMWindow::close):
* testing/Internals.cpp:
(WebCore::Internals::notifyResourceLoadObserver):

Source/WebKit:

No new tests, this fixes a timing bug that is flaky to reproduce, so I
was unable to write a test case. Non-regressed behavior is confirmed
with existing API testing.

* NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:
(WebKit::WebResourceLoadStatisticsStore::resourceLoadStatisticsUpdated):
Most of the time, the completion handler will be an empty function,
but we should handle it in the network process so we fix the
case where we wait to send the full data summary until the update has
finished.

(WebKit::WebResourceLoadStatisticsStore::aggregatedThirdPartyData):
Delete extra space.

* NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:
* NetworkProcess/NetworkConnectionToWebProcess.cpp:
(WebKit::NetworkConnectionToWebProcess::resourceLoadStatisticsUpdated):
* NetworkProcess/NetworkConnectionToWebProcess.h:
* NetworkProcess/NetworkConnectionToWebProcess.messages.in:
* UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::sendResourceLoadStatisticsDataImmediately):
* UIProcess/WebProcessPool.h:
* UIProcess/WebsiteData/WebsiteDataStore.cpp:
(WebKit::WebsiteDataStore::getResourceLoadStatisticsDataSummary):
Don't ask the network process for data until any lingering data
in the web content process has been sent first.

* WebProcess/InjectedBundle/API/c/WKBundle.cpp:
(WKBundleResourceLoadStatisticsNotifyObserver):
* WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:
(WebKit::WebResourceLoadObserver::WebResourceLoadObserver):
(WebKit::WebResourceLoadObserver::~WebResourceLoadObserver):
(WebKit::WebResourceLoadObserver::updateCentralStatisticsStore):
* WebProcess/WebCoreSupport/WebResourceLoadObserver.h:
* WebProcess/WebProcess.cpp:
(WebKit::WebProcess::flushResourceLoadStatistics):
(WebKit::WebProcess::sendResourceLoadStatisticsDataImmediately):
* WebProcess/WebProcess.h:
* WebProcess/WebProcess.messages.in:


Canonical link: https://commits.webkit.org/228668@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@266214 268f45cc-cd09-0410-ab3c-d52691b4dbfc
  • Loading branch information
kcheney1 committed Aug 27, 2020
1 parent 7cadbd5 commit db9bbb9
Show file tree
Hide file tree
Showing 19 changed files with 117 additions and 24 deletions.
18 changes: 18 additions & 0 deletions Source/WebCore/ChangeLog
@@ -1,3 +1,21 @@
2020-08-26 Kate Cheney <katherine_cheney@apple.com>

Resource Load Statistics data summary does not report data which is held up in the web content process.
https://bugs.webkit.org/show_bug.cgi?id=215822
<rdar://problem/66682044>

Reviewed by Chris Dumez.

Send empty lambda when calling updateCentralStatisticsStore() because
in these cases we don't care about timing.

* loader/ResourceLoadObserver.h:
(WebCore::ResourceLoadObserver::updateCentralStatisticsStore):
* page/DOMWindow.cpp:
(WebCore::DOMWindow::close):
* testing/Internals.cpp:
(WebCore::Internals::notifyResourceLoadObserver):

2020-08-25 Ryosuke Niwa <rniwa@webkit.org>

Make it possible to create a WeakPtr to Node and use it store assigned nodes in SlotAssignment
Expand Down
3 changes: 2 additions & 1 deletion Source/WebCore/loader/ResourceLoadObserver.h
Expand Up @@ -26,6 +26,7 @@
#pragma once

#include "ResourceLoadStatistics.h"
#include <wtf/CompletionHandler.h>
#include <wtf/Forward.h>

namespace WebCore {
Expand Down Expand Up @@ -58,7 +59,7 @@ class ResourceLoadObserver {
virtual void logSubresourceLoadingForTesting(const RegistrableDomain& /* firstPartyDomain */, const RegistrableDomain& /* thirdPartyDomain */, bool /* shouldScheduleNotification */) { }

virtual String statisticsForURL(const URL&) { return { }; }
virtual void updateCentralStatisticsStore() { }
virtual void updateCentralStatisticsStore(CompletionHandler<void()>&& completionHandler) { completionHandler(); }
virtual void clearState() { }

virtual bool hasStatistics() const { return false; }
Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/page/DOMWindow.cpp
Expand Up @@ -1048,7 +1048,7 @@ void DOMWindow::close()
if (!frame->loader().shouldClose())
return;

ResourceLoadObserver::shared().updateCentralStatisticsStore();
ResourceLoadObserver::shared().updateCentralStatisticsStore([] { });

page->setIsClosing();
page->chrome().closeWindowSoon();
Expand Down
2 changes: 1 addition & 1 deletion Source/WebCore/testing/Internals.cpp
Expand Up @@ -5635,7 +5635,7 @@ size_t Internals::pluginCount()

void Internals::notifyResourceLoadObserver()
{
ResourceLoadObserver::shared().updateCentralStatisticsStore();
ResourceLoadObserver::shared().updateCentralStatisticsStore([] { });
}

unsigned Internals::primaryScreenDisplayID()
Expand Down
48 changes: 48 additions & 0 deletions Source/WebKit/ChangeLog
@@ -1,3 +1,51 @@
2020-08-26 Kate Cheney <katherine_cheney@apple.com>

Resource Load Statistics data summary does not report data which is held up in the web content process.
https://bugs.webkit.org/show_bug.cgi?id=215822
<rdar://problem/66682044>

Reviewed by Chris Dumez.

No new tests, this fixes a timing bug that is flaky to reproduce, so I
was unable to write a test case. Non-regressed behavior is confirmed
with existing API testing.

* NetworkProcess/Classifier/WebResourceLoadStatisticsStore.cpp:
(WebKit::WebResourceLoadStatisticsStore::resourceLoadStatisticsUpdated):
Most of the time, the completion handler will be an empty function,
but we should handle it in the network process so we fix the
case where we wait to send the full data summary until the update has
finished.

(WebKit::WebResourceLoadStatisticsStore::aggregatedThirdPartyData):
Delete extra space.

* NetworkProcess/Classifier/WebResourceLoadStatisticsStore.h:
* NetworkProcess/NetworkConnectionToWebProcess.cpp:
(WebKit::NetworkConnectionToWebProcess::resourceLoadStatisticsUpdated):
* NetworkProcess/NetworkConnectionToWebProcess.h:
* NetworkProcess/NetworkConnectionToWebProcess.messages.in:
* UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::sendResourceLoadStatisticsDataImmediately):
* UIProcess/WebProcessPool.h:
* UIProcess/WebsiteData/WebsiteDataStore.cpp:
(WebKit::WebsiteDataStore::getResourceLoadStatisticsDataSummary):
Don't ask the network process for data until any lingering data
in the web content process has been sent first.

* WebProcess/InjectedBundle/API/c/WKBundle.cpp:
(WKBundleResourceLoadStatisticsNotifyObserver):
* WebProcess/WebCoreSupport/WebResourceLoadObserver.cpp:
(WebKit::WebResourceLoadObserver::WebResourceLoadObserver):
(WebKit::WebResourceLoadObserver::~WebResourceLoadObserver):
(WebKit::WebResourceLoadObserver::updateCentralStatisticsStore):
* WebProcess/WebCoreSupport/WebResourceLoadObserver.h:
* WebProcess/WebProcess.cpp:
(WebKit::WebProcess::flushResourceLoadStatistics):
(WebKit::WebProcess::sendResourceLoadStatisticsDataImmediately):
* WebProcess/WebProcess.h:
* WebProcess/WebProcess.messages.in:

2020-08-26 Andres Gonzalez <andresg_22@apple.com>

Buttons with aria-haspopup attribute are not exposed to accessibility clients as form controls.
Expand Down
Expand Up @@ -353,20 +353,22 @@ void WebResourceLoadStatisticsStore::statisticsDatabaseHasAllTables(CompletionHa
});
}

void WebResourceLoadStatisticsStore::resourceLoadStatisticsUpdated(Vector<ResourceLoadStatistics>&& statistics)
void WebResourceLoadStatisticsStore::resourceLoadStatisticsUpdated(Vector<ResourceLoadStatistics>&& statistics, CompletionHandler<void()>&& completionHandler)
{
ASSERT(RunLoop::isMain());

// 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.
postTask([this, protectedThis = makeRef(*this), statistics = WTFMove(statistics)]() mutable {
if (!m_statisticsStore)
postTask([this, protectedThis = makeRef(*this), statistics = WTFMove(statistics), completionHandler = WTFMove(completionHandler)]() mutable {
if (!m_statisticsStore) {
postTaskReply(WTFMove(completionHandler));
return;
}

ASSERT(suspendedState != State::Suspended);
m_statisticsStore->mergeStatistics(WTFMove(statistics));

postTaskReply(WTFMove(completionHandler));
// We can cancel any pending request to process statistics since we're doing it synchronously below.
m_statisticsStore->cancelPendingStatisticsProcessingRequest();

Expand Down Expand Up @@ -1444,7 +1446,7 @@ void WebResourceLoadStatisticsStore::aggregatedThirdPartyData(CompletionHandler<
{
ASSERT(RunLoop::isMain());

postTask([this, completionHandler = WTFMove(completionHandler)]() mutable {
postTask([this, completionHandler = WTFMove(completionHandler)]() mutable {
if (!m_statisticsStore) {
postTaskReply([completionHandler = WTFMove(completionHandler)]() mutable {
completionHandler({ });
Expand Down
Expand Up @@ -292,7 +292,7 @@ struct ThirdPartyData {
void sendDiagnosticMessageWithValue(const String& message, const String& description, unsigned value, unsigned sigDigits, WebCore::ShouldSample) const;
void notifyPageStatisticsTelemetryFinished(unsigned numberOfPrevalentResources, unsigned numberOfPrevalentResourcesWithUserInteraction, unsigned numberOfPrevalentResourcesWithoutUserInteraction, unsigned topPrevalentResourceWithUserInteractionDaysSinceUserInteraction, unsigned medianDaysSinceUserInteractionPrevalentResourceWithUserInteraction, unsigned top3NumberOfPrevalentResourcesWithUI, unsigned top3MedianSubFrameWithoutUI, unsigned top3MedianSubResourceWithoutUI, unsigned top3MedianUniqueRedirectsWithoutUI, unsigned top3MedianDataRecordsRemovedWithoutUI) const;

void resourceLoadStatisticsUpdated(Vector<WebCore::ResourceLoadStatistics>&&);
void resourceLoadStatisticsUpdated(Vector<WebCore::ResourceLoadStatistics>&&, CompletionHandler<void()>&&);
void requestStorageAccessUnderOpener(DomainInNeedOfStorageAccess&&, WebCore::PageIdentifier openerID, OpenerDomain&&);
void aggregatedThirdPartyData(CompletionHandler<void(Vector<WebResourceLoadStatisticsStore::ThirdPartyData>&&)>&&);
static void suspend(CompletionHandler<void()>&&);
Expand Down
13 changes: 9 additions & 4 deletions Source/WebKit/NetworkProcess/NetworkConnectionToWebProcess.cpp
Expand Up @@ -870,14 +870,19 @@ void NetworkConnectionToWebProcess::logUserInteraction(const RegistrableDomain&
}
}

void NetworkConnectionToWebProcess::resourceLoadStatisticsUpdated(Vector<ResourceLoadStatistics>&& statistics)
void NetworkConnectionToWebProcess::resourceLoadStatisticsUpdated(Vector<ResourceLoadStatistics>&& statistics, CompletionHandler<void()>&& completionHandler)
{
if (auto* networkSession = this->networkSession()) {
if (networkSession->sessionID().isEphemeral())
if (networkSession->sessionID().isEphemeral()) {
completionHandler();
return;
if (auto* resourceLoadStatistics = networkSession->resourceLoadStatistics())
resourceLoadStatistics->resourceLoadStatisticsUpdated(WTFMove(statistics));
}
if (auto* resourceLoadStatistics = networkSession->resourceLoadStatistics()) {
resourceLoadStatistics->resourceLoadStatisticsUpdated(WTFMove(statistics), WTFMove(completionHandler));
return;
}
}
completionHandler();
}

void NetworkConnectionToWebProcess::hasStorageAccess(const RegistrableDomain& subFrameDomain, const RegistrableDomain& topFrameDomain, FrameIdentifier frameID, PageIdentifier pageID, CompletionHandler<void(bool)>&& completionHandler)
Expand Down
Expand Up @@ -266,7 +266,7 @@ class NetworkConnectionToWebProcess
void clearPageSpecificDataForResourceLoadStatistics(WebCore::PageIdentifier);

void logUserInteraction(const RegistrableDomain&);
void resourceLoadStatisticsUpdated(Vector<WebCore::ResourceLoadStatistics>&&);
void resourceLoadStatisticsUpdated(Vector<WebCore::ResourceLoadStatistics>&&, CompletionHandler<void()>&&);
void hasStorageAccess(const RegistrableDomain& subFrameDomain, const RegistrableDomain& topFrameDomain, WebCore::FrameIdentifier, WebCore::PageIdentifier, CompletionHandler<void(bool)>&&);
void requestStorageAccess(const RegistrableDomain& subFrameDomain, const RegistrableDomain& topFrameDomain, WebCore::FrameIdentifier, WebCore::PageIdentifier, WebPageProxyIdentifier, WebCore::StorageAccessScope, CompletionHandler<void(WebCore::RequestStorageAccessResult)>&&);
void requestStorageAccessUnderOpener(WebCore::RegistrableDomain&& domainInNeedOfStorageAccess, WebCore::PageIdentifier openerPageID, WebCore::RegistrableDomain&& openerDomain);
Expand Down
Expand Up @@ -65,7 +65,7 @@ messages -> NetworkConnectionToWebProcess LegacyReceiver {
RemoveStorageAccessForFrame(WebCore::FrameIdentifier frameID, WebCore::PageIdentifier pageID);
ClearPageSpecificDataForResourceLoadStatistics(WebCore::PageIdentifier pageID);
LogUserInteraction(WebCore::RegistrableDomain domain)
ResourceLoadStatisticsUpdated(Vector<WebCore::ResourceLoadStatistics> statistics)
ResourceLoadStatisticsUpdated(Vector<WebCore::ResourceLoadStatistics> statistics) -> () Async
HasStorageAccess(WebCore::RegistrableDomain subFrameDomain, WebCore::RegistrableDomain topFrameDomain, WebCore::FrameIdentifier frameID, WebCore::PageIdentifier pageID) -> (bool hasStorageAccess) Async
RequestStorageAccess(WebCore::RegistrableDomain subFrameDomain, WebCore::RegistrableDomain topFrameDomain, WebCore::FrameIdentifier frameID, WebCore::PageIdentifier webPageID, WebKit::WebPageProxyIdentifier webPageProxyID, enum:bool WebCore::StorageAccessScope scope) -> (struct WebCore::RequestStorageAccessResult result) Async
RequestStorageAccessUnderOpener(WebCore::RegistrableDomain domainInNeedOfStorageAccess, WebCore::PageIdentifier openerPageID, WebCore::RegistrableDomain openerDomain)
Expand Down
8 changes: 8 additions & 0 deletions Source/WebKit/UIProcess/WebProcessPool.cpp
Expand Up @@ -2408,6 +2408,14 @@ void WebProcessPool::seedResourceLoadStatisticsForTesting(const RegistrableDomai
for (auto& process : processes())
process->sendWithAsyncReply(Messages::WebProcess::SeedResourceLoadStatisticsForTesting(firstPartyDomain, thirdPartyDomain, shouldScheduleNotification), [callbackAggregator] { });
}

void WebProcessPool::sendResourceLoadStatisticsDataImmediately(CompletionHandler<void()>&& completionHandler)
{
auto callbackAggregator = CallbackAggregator::create(WTFMove(completionHandler));

for (auto& process : processes())
process->sendWithAsyncReply(Messages::WebProcess::SendResourceLoadStatisticsDataImmediately(), [callbackAggregator] { });
}
#endif

WebProcessWithAudibleMediaToken WebProcessPool::webProcessWithAudibleMediaToken() const
Expand Down
1 change: 1 addition & 0 deletions Source/WebKit/UIProcess/WebProcessPool.h
Expand Up @@ -514,6 +514,7 @@ class WebProcessPool final : public API::ObjectImpl<API::Object::Type::ProcessPo
void didCommitCrossSiteLoadWithDataTransfer(PAL::SessionID, const WebCore::RegistrableDomain& fromDomain, const WebCore::RegistrableDomain& toDomain, OptionSet<WebCore::CrossSiteNavigationDataTransfer::Flag>, WebPageProxyIdentifier, WebCore::PageIdentifier);
void setDomainsWithUserInteraction(HashSet<WebCore::RegistrableDomain>&&);
void seedResourceLoadStatisticsForTesting(const WebCore::RegistrableDomain& firstPartyDomain, const WebCore::RegistrableDomain& thirdPartyDomain, bool shouldScheduleNotification, CompletionHandler<void()>&&);
void sendResourceLoadStatisticsDataImmediately(CompletionHandler<void()>&&);
#endif

#if PLATFORM(GTK) || PLATFORM(WPE)
Expand Down
6 changes: 4 additions & 2 deletions Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
Expand Up @@ -1562,8 +1562,10 @@ void WebsiteDataStore::getResourceLoadStatisticsDataSummary(CompletionHandler<vo
RefPtr<CallbackAggregator> callbackAggregator = adoptRef(new CallbackAggregator(WTFMove(completionHandler)));

for (auto& processPool : ensureProcessPools()) {
processPool->ensureNetworkProcess(this).getResourceLoadStatisticsDataSummary(m_sessionID, [callbackAggregator, processPool](Vector<WebResourceLoadStatisticsStore::ThirdPartyData>&& data) {
callbackAggregator->addResult(WTFMove(data));
processPool->sendResourceLoadStatisticsDataImmediately([this, protectedThis = makeRef(*this), processPool, callbackAggregator] {
processPool->ensureNetworkProcess(this).getResourceLoadStatisticsDataSummary(m_sessionID, [callbackAggregator, processPool](Vector<WebResourceLoadStatisticsStore::ThirdPartyData>&& data) {
callbackAggregator->addResult(WTFMove(data));
});
});
}
}
Expand Down
2 changes: 1 addition & 1 deletion Source/WebKit/WebProcess/InjectedBundle/API/c/WKBundle.cpp
Expand Up @@ -304,7 +304,7 @@ bool WKBundleResourceLoadStatisticsNotifyObserver(WKBundleRef)
if (!WebCore::ResourceLoadObserver::shared().hasStatistics())
return false;

WebCore::ResourceLoadObserver::shared().updateCentralStatisticsStore();
WebCore::ResourceLoadObserver::shared().updateCentralStatisticsStore([] { });
return true;
}

Expand Down
Expand Up @@ -63,14 +63,14 @@ static bool is3xxRedirect(const ResourceResponse& response)

WebResourceLoadObserver::WebResourceLoadObserver(ResourceLoadStatistics::IsEphemeral isEphemeral)
: m_isEphemeral(isEphemeral)
, m_notificationTimer(*this, &WebResourceLoadObserver::updateCentralStatisticsStore)
, m_notificationTimer([this] { updateCentralStatisticsStore([] { }); })
{
}

WebResourceLoadObserver::~WebResourceLoadObserver()
{
if (hasStatistics())
updateCentralStatisticsStore();
updateCentralStatisticsStore([] { });
}

void WebResourceLoadObserver::requestStorageAccessUnderOpener(const RegistrableDomain& domainInNeedOfStorageAccess, WebPage& openerPage, Document& openerDocument)
Expand Down Expand Up @@ -109,12 +109,13 @@ void WebResourceLoadObserver::scheduleNotificationIfNeeded()
m_notificationTimer.startOneShot(minimumNotificationInterval);
}

void WebResourceLoadObserver::updateCentralStatisticsStore()
void WebResourceLoadObserver::updateCentralStatisticsStore(CompletionHandler<void()>&& completionHandler)
{
m_notificationTimer.stop();
WebProcess::singleton().ensureNetworkProcessConnection().connection().send(Messages::NetworkConnectionToWebProcess::ResourceLoadStatisticsUpdated(takeStatistics()), 0);
WebProcess::singleton().ensureNetworkProcessConnection().connection().sendWithAsyncReply(Messages::NetworkConnectionToWebProcess::ResourceLoadStatisticsUpdated(takeStatistics()), WTFMove(completionHandler));
}


String WebResourceLoadObserver::statisticsForURL(const URL& url)
{
auto iter = m_resourceStatisticsMap.find(RegistrableDomain { url });
Expand Down
Expand Up @@ -57,7 +57,7 @@ class WebResourceLoadObserver final : public WebCore::ResourceLoadObserver {
#endif

String statisticsForURL(const URL&) final;
void updateCentralStatisticsStore() final;
void updateCentralStatisticsStore(CompletionHandler<void()>&&) final;
void clearState() final;

bool hasStatistics() const final { return !m_resourceStatisticsMap.isEmpty(); }
Expand Down
7 changes: 6 additions & 1 deletion Source/WebKit/WebProcess/WebProcess.cpp
Expand Up @@ -1595,7 +1595,7 @@ void WebProcess::flushResourceLoadStatistics()
{
#if ENABLE(RESOURCE_LOAD_STATISTICS)
if (auto* observer = ResourceLoadObserver::sharedIfExists())
observer->updateCentralStatisticsStore();
observer->updateCentralStatisticsStore([] { });
#endif
}

Expand Down Expand Up @@ -1907,6 +1907,11 @@ void WebProcess::setDomainsWithUserInteraction(HashSet<WebCore::RegistrableDomai
{
ResourceLoadObserver::shared().setDomainsWithUserInteraction(WTFMove(domains));
}

void WebProcess::sendResourceLoadStatisticsDataImmediately(CompletionHandler<void()>&& completionHandler)
{
ResourceLoadObserver::shared().updateCentralStatisticsStore(WTFMove(completionHandler));
}
#endif

#if ENABLE(GPU_PROCESS)
Expand Down
1 change: 1 addition & 0 deletions Source/WebKit/WebProcess/WebProcess.h
Expand Up @@ -475,6 +475,7 @@ class WebProcess : public AuxiliaryProcess
#if ENABLE(RESOURCE_LOAD_STATISTICS)
void setThirdPartyCookieBlockingMode(WebCore::ThirdPartyCookieBlockingMode, CompletionHandler<void()>&&);
void setDomainsWithUserInteraction(HashSet<WebCore::RegistrableDomain>&&);
void sendResourceLoadStatisticsDataImmediately(CompletionHandler<void()>&&);
#endif

void platformInitializeProcess(const AuxiliaryProcessInitializationParameters&);
Expand Down
1 change: 1 addition & 0 deletions Source/WebKit/WebProcess/WebProcess.messages.in
Expand Up @@ -159,6 +159,7 @@ messages -> WebProcess LegacyReceiver NotRefCounted {
SeedResourceLoadStatisticsForTesting(WebCore::RegistrableDomain firstPartyDomain, WebCore::RegistrableDomain thirdPartyDomain, bool shouldScheduleNotification) -> () Async
SetThirdPartyCookieBlockingMode(enum:uint8_t WebCore::ThirdPartyCookieBlockingMode blockingMode) -> () Async
SetDomainsWithUserInteraction(HashSet<WebCore::RegistrableDomain> domains)
SendResourceLoadStatisticsDataImmediately() -> () Async
#endif

#if PLATFORM(IOS)
Expand Down

0 comments on commit db9bbb9

Please sign in to comment.