Skip to content

Commit

Permalink
Register source eligible data host when the beacon is initiated
Browse files Browse the repository at this point in the history 
Fenced frame beacons are pending until the reporting script completes
running assuming it completes running before the frame that ran the
auction is destroyed. Otherwise they hang out in the reporter until
the main frame of the frame running the auction is destroyed. Usually
the reporting script runs fast.

This CL simplifies the interface in AttributionDataHostManager that
integrates with FencedFrameReporter. The source eligible data host is
registered when the beacon is initiated. In the case that the beacon
failed due to data validation or dropped due to reporting script not
completed, NotifyFencedFrameReportingBeaconData is invoked with an
opaque reporting origin, and AttributionDataHostManager will stop
tracking this beacon. Currently the beacons hang out in
AttributionDataHostManager in those cases and is a memory leak.

Registering source eligible data host when the beacon is initiated
ensures that triggers registered on the landing page are buffered
properly even if the reporting script completes later than a quick
navigation.

Bug: 1428040
Change-Id: I9c55a43394a9dd0e73aab5819a777ee059b2b80c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4368332
Reviewed-by: Andrew Paseltiner <apaseltiner@chromium.org>
Commit-Queue: Nan Lin <linnan@chromium.org>
Reviewed-by: Matt Menke <mmenke@chromium.org>
Reviewed-by: Garrett Tanzer <gtanzer@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1122415}
  • Loading branch information
@linnan-github
linnan-github authored and Chromium LUCI CQ committed Mar 27, 2023
1 parent 60510d1 commit b4a9672
Show file tree
Hide file tree
Showing 11 changed files with 252 additions and 134 deletions.
5 changes: 2 additions & 3 deletions content/browser/attribution_reporting/attribution_data_host_manager.h
View file
Expand Up @@ -101,14 +101,13 @@ class AttributionDataHostManager
AttributionInputEvent input_event,
GlobalRenderFrameHostId render_frame_id) = 0;

// Notifies the manager that a beacon has been sent.
virtual void NotifyFencedFrameReportingBeaconSent(BeaconId beacon_id) = 0;

// Notifies the manager whenever a response has been received to a beacon HTTP
// request. Must be invoked for each redirect received, as well as the final
// response. `reporting_origin` is the origin that sent `headers` that may
// contain attribution source registration. `is_final_response` indicates
// whether this is a redirect or a final response.
// An opaque origin will be set for `reporting_origin` if the beacon failed to
// be sent.
virtual void NotifyFencedFrameReportingBeaconData(
BeaconId beacon_id,
url::Origin reporting_origin,
Expand Down
58 changes: 17 additions & 41 deletions content/browser/attribution_reporting/attribution_data_host_manager_impl.cc
View file
Expand Up @@ -238,13 +238,11 @@ class AttributionDataHostManagerImpl::SourceRegistrations {
using Data = absl::variant<NavigationRedirect, BeaconId>;

SourceRegistrations(SuitableOrigin source_origin,
base::TimeTicks register_time,
bool is_within_fenced_frame,
AttributionInputEvent input_event,
GlobalRenderFrameHostId render_frame_id,
Data data)
: source_origin_(std::move(source_origin)),
register_time_(register_time),
is_within_fenced_frame_(is_within_fenced_frame),
input_event_(std::move(input_event)),
render_frame_id_(render_frame_id),
Expand Down Expand Up @@ -281,11 +279,6 @@ class AttributionDataHostManagerImpl::SourceRegistrations {
registrations_complete_ = true;
}

void set_register_time() {
DCHECK(register_time_.is_null());
register_time_ = base::TimeTicks::Now();
}

void IncrementPendingSourceData() { ++pending_source_data_; }

void DecrementPendingSourceData() {
Expand Down Expand Up @@ -327,9 +320,9 @@ class AttributionDataHostManagerImpl::SourceRegistrations {
// True if navigation or beacon has completed.
bool registrations_complete_ = false;

// The time the first registration header was received. Will be null when the
// beacon was started but no data was received yet.
base::TimeTicks register_time_;
// The time the first registration header was received for navigation
// redirects; the time the beacon was initiated for beacons.
base::TimeTicks register_time_ = base::TimeTicks::Now();

// Whether the registration was initiated within a fenced frame.
bool is_within_fenced_frame_;
Expand Down Expand Up @@ -449,14 +442,13 @@ void AttributionDataHostManagerImpl::NotifyNavigationRedirectRegistration(
return;
}

auto [it, inserted] = registrations_.emplace(
source_origin,
/*register_time=*/base::TimeTicks::Now(), is_within_fenced_frame,
std::move(input_event), render_frame_id,
SourceRegistrations::NavigationRedirect{
.attribution_src_token = attribution_src_token,
.nav_type = nav_type,
});
auto [it, inserted] =
registrations_.emplace(source_origin, is_within_fenced_frame,
std::move(input_event), render_frame_id,
SourceRegistrations::NavigationRedirect{
.attribution_src_token = attribution_src_token,
.nav_type = nav_type,
});
DCHECK(!it->registrations_complete());

// Treat ongoing redirect registrations within a chain as a data host for the
Expand Down Expand Up @@ -760,9 +752,7 @@ void AttributionDataHostManagerImpl::OnReceiverDisconnected() {

void AttributionDataHostManagerImpl::OnSourceEligibleDataHostFinished(
base::TimeTicks register_time) {
if (register_time.is_null()) {
return;
}
DCHECK(!register_time.is_null());

// Decrement the number of receivers in source mode and flush triggers if
// applicable.
Expand Down Expand Up @@ -810,29 +800,15 @@ void AttributionDataHostManagerImpl::NotifyFencedFrameReportingBeaconStarted(
AttributionInputEvent input_event,
GlobalRenderFrameHostId render_frame_id) {
auto [it, inserted] = registrations_.emplace(
std::move(source_origin),
/*register_time=*/base::TimeTicks(), is_within_fenced_frame,
std::move(input_event), render_frame_id, beacon_id);
std::move(source_origin), is_within_fenced_frame, std::move(input_event),
render_frame_id, beacon_id);
DCHECK(inserted);
}

void AttributionDataHostManagerImpl::NotifyFencedFrameReportingBeaconSent(
BeaconId beacon_id) {
auto it = registrations_.find(beacon_id);

// The registration may no longer be tracked in the event the navigation
// failed.
if (it == registrations_.end()) {
return;
}

it->set_register_time();

// Treat ongoing beacon registrations as a data host for the purpose of
// trigger queuing. Navigation beacon is sent before the navigation commits,
// therefore registering source eligible data host when the beacon is sent
// ensures that triggers registered on the landing page are properly queued in
// the case that the beacon response is delivered late.
// trigger queuing. Navigation beacon is started before the navigation
// commits, therefore registering source eligible data host when the beacon is
// started ensures that triggers registered on the landing page are properly
// queued in the case that the beacon response is delivered late.
data_hosts_in_source_mode_++;
}

Expand Down
1 change: 0 additions & 1 deletion content/browser/attribution_reporting/attribution_data_host_manager_impl.h
View file
Expand Up @@ -108,7 +108,6 @@ class CONTENT_EXPORT AttributionDataHostManagerImpl
bool is_within_fenced_frame,
AttributionInputEvent input_event,
GlobalRenderFrameHostId render_frame_id) override;
void NotifyFencedFrameReportingBeaconSent(BeaconId beacon_id) override;
void NotifyFencedFrameReportingBeaconData(
BeaconId beacon_id,
url::Origin reporting_origin,
Expand Down
42 changes: 33 additions & 9 deletions content/browser/attribution_reporting/attribution_data_host_manager_impl_unittest.cc
View file
Expand Up @@ -1671,7 +1671,6 @@ TEST_F(AttributionDataHostManagerImplTest,
data_host_manager_.NotifyFencedFrameReportingBeaconStarted(
navigation_id, std::move(source_origin), /*is_within_fenced_frame=*/false,
AttributionInputEvent(), kFrameId);
data_host_manager_.NotifyFencedFrameReportingBeaconSent(navigation_id);

auto headers = base::MakeRefCounted<net::HttpResponseHeaders>("");
headers->SetHeader(kAttributionReportingRegisterSourceHeader,
Expand Down Expand Up @@ -1705,7 +1704,6 @@ TEST_F(AttributionDataHostManagerImplTest,
data_host_manager_.NotifyFencedFrameReportingBeaconStarted(
navigation_id, std::move(source_origin), /*is_within_fenced_frame=*/false,
AttributionInputEvent(), kFrameId);
data_host_manager_.NotifyFencedFrameReportingBeaconSent(navigation_id);

auto headers = base::MakeRefCounted<net::HttpResponseHeaders>("");
headers->SetHeader(kAttributionReportingRegisterSourceHeader,
Expand Down Expand Up @@ -1743,7 +1741,6 @@ TEST_F(AttributionDataHostManagerImplTest,
data_host_manager_.NotifyFencedFrameReportingBeaconStarted(
navigation_id, source_origin, /*is_within_fenced_frame=*/false,
AttributionInputEvent(), kFrameId);
data_host_manager_.NotifyFencedFrameReportingBeaconSent(navigation_id);

auto headers = base::MakeRefCounted<net::HttpResponseHeaders>("");
headers->SetHeader(kAttributionReportingRegisterSourceHeader,
Expand Down Expand Up @@ -1840,7 +1837,6 @@ TEST_F(AttributionDataHostManagerImplTest,
data_host_manager_.NotifyFencedFrameReportingBeaconStarted(
navigation_id, source_origin, /*is_within_fenced_frame=*/false,
AttributionInputEvent(), kFrameId);
data_host_manager_.NotifyFencedFrameReportingBeaconSent(navigation_id);

auto headers = base::MakeRefCounted<net::HttpResponseHeaders>("");
headers->SetHeader(kAttributionReportingRegisterSourceHeader,
Expand Down Expand Up @@ -1880,7 +1876,6 @@ TEST_F(AttributionDataHostManagerImplTest,
data_host_manager_.NotifyFencedFrameReportingBeaconStarted(
navigation_id, source_origin, /*is_within_fenced_frame=*/false,
AttributionInputEvent(), kFrameId);
data_host_manager_.NotifyFencedFrameReportingBeaconSent(navigation_id);

auto headers = base::MakeRefCounted<net::HttpResponseHeaders>("");
headers->SetHeader(kAttributionReportingRegisterSourceHeader,
Expand Down Expand Up @@ -1917,7 +1912,6 @@ TEST_F(AttributionDataHostManagerImplTest,
data_host_manager_.NotifyFencedFrameReportingBeaconStarted(
navigation_id, source_origin,
/*is_within_fenced_frame=*/false, AttributionInputEvent(), kFrameId);
data_host_manager_.NotifyFencedFrameReportingBeaconSent(navigation_id);

auto headers = base::MakeRefCounted<net::HttpResponseHeaders>("");
headers->SetHeader(kAttributionReportingRegisterSourceHeader,
Expand Down Expand Up @@ -1957,7 +1951,6 @@ TEST_F(AttributionDataHostManagerImplTest,
navigation_id,
/*source_origin=*/*SuitableOrigin::Deserialize("https://report.test"),
/*is_within_fenced_frame=*/false, AttributionInputEvent(), kFrameId);
data_host_manager_.NotifyFencedFrameReportingBeaconSent(navigation_id);

mojo::Remote<blink::mojom::AttributionDataHost> trigger_data_host_remote;
data_host_manager_.RegisterDataHost(
Expand Down Expand Up @@ -2008,7 +2001,6 @@ TEST_F(AttributionDataHostManagerImplTest,
data_host_manager_.NotifyFencedFrameReportingBeaconStarted(
navigation_id, std::move(source_origin), /*is_within_fenced_frame=*/false,
AttributionInputEvent(), kFrameId);
data_host_manager_.NotifyFencedFrameReportingBeaconSent(navigation_id);

auto headers = base::MakeRefCounted<net::HttpResponseHeaders>("");
headers->SetHeader(kAttributionReportingRegisterSourceHeader,
Expand Down Expand Up @@ -2042,6 +2034,39 @@ TEST_F(AttributionDataHostManagerImplTest,
CheckTriggerQueueHistograms(histograms, {.skipped_queue = 1});
}

TEST_F(AttributionDataHostManagerImplTest,
NavigationBeaconSource_NavigationBeaconFailedQueueSkipped) {
base::HistogramTester histograms;

EXPECT_CALL(mock_manager_, HandleTrigger);

auto reporting_origin = url::Origin::Create(GURL("https://report.test"));
auto source_origin = *SuitableOrigin::Deserialize("https://source.test");

NavigationBeaconId navigation_id(123);
data_host_manager_.NotifyFencedFrameReportingBeaconStarted(
navigation_id, std::move(source_origin), /*is_within_fenced_frame=*/false,
AttributionInputEvent(), kFrameId);

data_host_manager_.NotifyFencedFrameReportingBeaconData(
navigation_id, /*reporting_origin=*/url::Origin(), /*headers=*/nullptr,
/*is_final_response=*/true);

mojo::Remote<blink::mojom::AttributionDataHost> trigger_data_host_remote;
data_host_manager_.RegisterDataHost(
trigger_data_host_remote.BindNewPipeAndPassReceiver(),
*SuitableOrigin::Deserialize("https://page2.example"),
/*is_within_fenced_frame=*/false, RegistrationType::kSourceOrTrigger,
kFrameId);

trigger_data_host_remote->TriggerDataAvailable(
*SuitableOrigin::Create(std::move(reporting_origin)),
TriggerRegistration(), /*attestation=*/absl::nullopt);
trigger_data_host_remote.FlushForTesting();

CheckTriggerQueueHistograms(histograms, {.skipped_queue = 1});
}

TEST_F(AttributionDataHostManagerImplTest, EventBeaconSource_DataReceived) {
EXPECT_CALL(mock_manager_,
HandleSource(AllOf(SourceTypeIs(SourceType::kEvent),
Expand All @@ -2054,7 +2079,6 @@ TEST_F(AttributionDataHostManagerImplTest, EventBeaconSource_DataReceived) {
/*source_origin=*/*SuitableOrigin::Deserialize("https://source.test"),
/*is_within_fenced_frame=*/true,
/*input_event=*/AttributionInputEvent(), kFrameId);
data_host_manager_.NotifyFencedFrameReportingBeaconSent(event_id);

auto headers = base::MakeRefCounted<net::HttpResponseHeaders>("");
headers->SetHeader(kAttributionReportingRegisterSourceHeader,
Expand Down
79 changes: 4 additions & 75 deletions content/browser/attribution_reporting/attribution_host_unittest.cc
View file
Expand Up @@ -18,6 +18,7 @@
#include "content/browser/attribution_reporting/attribution_input_event.h"
#include "content/browser/attribution_reporting/attribution_manager.h"
#include "content/browser/attribution_reporting/attribution_test_utils.h"
#include "content/browser/attribution_reporting/test/mock_attribution_data_host_manager.h"
#include "content/browser/attribution_reporting/test/mock_attribution_manager.h"
#include "content/browser/storage_partition_impl.h"
#include "content/browser/web_contents/web_contents_impl.h"
Expand Down Expand Up @@ -69,78 +70,6 @@ using ::blink::mojom::AttributionNavigationType;

const char kConversionUrl[] = "https://b.com";

class MockDataHostManager : public AttributionDataHostManager {
public:
MockDataHostManager() = default;
~MockDataHostManager() override = default;

MOCK_METHOD(
void,
RegisterDataHost,
(mojo::PendingReceiver<blink::mojom::AttributionDataHost> data_host,
SuitableOrigin context_origin,
bool is_within_fenced_frame,
RegistrationType,
GlobalRenderFrameHostId),
(override));

MOCK_METHOD(
bool,
RegisterNavigationDataHost,
(mojo::PendingReceiver<blink::mojom::AttributionDataHost> data_host,
const blink::AttributionSrcToken& attribution_src_token,
AttributionInputEvent input_event),
(override));

MOCK_METHOD(void,
NotifyNavigationRedirectRegistration,
(const blink::AttributionSrcToken& attribution_src_token,
const net::HttpResponseHeaders* headers,
SuitableOrigin reporting_origin,
const SuitableOrigin& source_origin,
AttributionInputEvent input_event,
AttributionNavigationType,
bool is_within_fenced_frame,
GlobalRenderFrameHostId),
(override));

MOCK_METHOD(void,
NotifyNavigationForDataHost,
(const blink::AttributionSrcToken& attribution_src_token,
const SuitableOrigin& source_origin,
AttributionNavigationType,
bool is_within_fenced_frame,
GlobalRenderFrameHostId),
(override));

MOCK_METHOD(void,
NotifyNavigationFailure,
(const blink::AttributionSrcToken& attribution_src_token),
(override));

MOCK_METHOD(void,
NotifyFencedFrameReportingBeaconStarted,
(BeaconId beacon_id,
SuitableOrigin source_origin,
bool is_within_fenced_frame,
AttributionInputEvent input_event,
GlobalRenderFrameHostId),
(override));

MOCK_METHOD(void,
NotifyFencedFrameReportingBeaconSent,
(BeaconId beacon_id),
(override));

MOCK_METHOD(void,
NotifyFencedFrameReportingBeaconData,
(BeaconId beacon_id,
url::Origin reporting_origin,
const net::HttpResponseHeaders* headers,
bool is_final_response),
(override));
};

class AttributionHostTest : public RenderViewHostTestHarness {
public:
AttributionHostTest() = default;
Expand All @@ -151,7 +80,7 @@ class AttributionHostTest : public RenderViewHostTestHarness {

RenderViewHostTestHarness::SetUp();

auto data_host_manager = std::make_unique<MockDataHostManager>();
auto data_host_manager = std::make_unique<MockAttributionDataHostManager>();
mock_data_host_manager_ = data_host_manager.get();

auto mock_manager = std::make_unique<MockAttributionManager>();
Expand Down Expand Up @@ -189,7 +118,7 @@ class AttributionHostTest : public RenderViewHostTestHarness {
OverrideAttributionManager(nullptr);
}

MockDataHostManager* mock_data_host_manager() {
MockAttributionDataHostManager* mock_data_host_manager() {
return mock_data_host_manager_;
}

Expand All @@ -200,7 +129,7 @@ class AttributionHostTest : public RenderViewHostTestHarness {
->OverrideAttributionManagerForTesting(std::move(manager));
}

raw_ptr<MockDataHostManager> mock_data_host_manager_;
raw_ptr<MockAttributionDataHostManager> mock_data_host_manager_;

base::test::ScopedFeatureList feature_list_;
};
Expand Down
13 changes: 13 additions & 0 deletions content/browser/attribution_reporting/test/mock_attribution_data_host_manager.cc
View file
@@ -0,0 +1,13 @@
// Copyright 2020 The Chromium Authors
// Use of this source code is governed by a BSD-style license that can be
// found in the LICENSE file.

#include "content/browser/attribution_reporting/test/mock_attribution_data_host_manager.h"

namespace content {

MockAttributionDataHostManager::MockAttributionDataHostManager() = default;

MockAttributionDataHostManager::~MockAttributionDataHostManager() = default;

} // namespace content

0 comments on commit b4a9672

Please sign in to comment.