Skip to content

Commit

Permalink
Reporting: Uploads should only contain a single origin
Browse files Browse the repository at this point in the history 
Per w3c/reporting#41, a reporting upload
should only contain reports about a single origin.  This will allow us
to send a CORS preflight for that origin, if it differs from the origin
of the collector receiving the reports.

This patch also removes the notion of an endpoint being "pending".  In
the spec, a "pending endpoint" is one that has been taken out of the
rotation because of too many failures; we're handling this with a
per-endpoint BackoffEntry.  And now that we're creating separate
uploads for each origin that uses a collector, we don't want to penalize
any of them by serializing their uploads.

Bug: 860802
Change-Id: I45cf905bd9ec3491e61aa0567c6dc0a19e957313
Reviewed-on: https://chromium-review.googlesource.com/1128599
Commit-Queue: Douglas Creager <dcreager@chromium.org>
Reviewed-by: Misha Efimov <mef@chromium.org>
Cr-Commit-Position: refs/heads/master@{#573815}
  • Loading branch information
@dcreager
dcreager authored and Commit Bot committed Jul 10, 2018
1 parent 719be54 commit c98446b
Show file tree
Hide file tree
Showing 5 changed files with 94 additions and 76 deletions.
55 changes: 30 additions & 25 deletions net/reporting/reporting_delivery_agent.cc
View file
Expand Up @@ -78,9 +78,14 @@ class ReportingDeliveryAgentImpl : public ReportingDeliveryAgent,
}

private:
using OriginGroup = std::pair<url::Origin, std::string>;
using OriginEndpoint = std::pair<url::Origin, GURL>;

class Delivery {
public:
Delivery(const GURL& endpoint) : endpoint(endpoint) {}
Delivery(const OriginEndpoint& report_origin_endpoint)
: report_origin(report_origin_endpoint.first),
endpoint(report_origin_endpoint.second) {}

~Delivery() = default;

Expand All @@ -90,13 +95,12 @@ class ReportingDeliveryAgentImpl : public ReportingDeliveryAgent,
reports.insert(reports.end(), to_add.begin(), to_add.end());
}

const url::Origin report_origin;
const GURL endpoint;
std::vector<const ReportingReport*> reports;
std::map<url::Origin, std::map<GURL, int>> reports_per_client;
};

using OriginGroup = std::pair<url::Origin, std::string>;

bool CacheHasReports() {
std::vector<const ReportingReport*> reports;
context_->cache()->GetReports(&reports);
Expand Down Expand Up @@ -126,53 +130,56 @@ class ReportingDeliveryAgentImpl : public ReportingDeliveryAgent,
cache()->SetReportsPending(reports);

// First determine which origins we're allowed to upload reports about.
std::set<url::Origin> origins;
std::set<url::Origin> report_origins;
for (const ReportingReport* report : reports) {
origins.insert(url::Origin::Create(report->url));
report_origins.insert(url::Origin::Create(report->url));
}
delegate()->CanSendReports(
std::move(origins),
std::move(report_origins),
base::BindOnce(&ReportingDeliveryAgentImpl::OnSendPermissionsChecked,
weak_factory_.GetWeakPtr(), std::move(reports)));
}

void OnSendPermissionsChecked(std::vector<const ReportingReport*> reports,
std::set<url::Origin> allowed_origins) {
std::set<url::Origin> allowed_report_origins) {
// Sort reports into (origin, group) buckets.
std::map<OriginGroup, std::vector<const ReportingReport*>>
origin_group_reports;
for (const ReportingReport* report : reports) {
url::Origin origin = url::Origin::Create(report->url);
if (allowed_origins.find(origin) == allowed_origins.end())
url::Origin report_origin = url::Origin::Create(report->url);
if (allowed_report_origins.find(report_origin) ==
allowed_report_origins.end())
continue;
OriginGroup origin_group(origin, report->group);
OriginGroup origin_group(report_origin, report->group);
origin_group_reports[origin_group].push_back(report);
}

// Find endpoint for each (origin, group) bucket and sort reports into
// Find an endpoint for each (origin, group) bucket and sort reports into
// endpoint buckets. Don't allow concurrent deliveries to the same (origin,
// group) bucket.
std::map<GURL, std::unique_ptr<Delivery>> deliveries;
std::map<OriginEndpoint, std::unique_ptr<Delivery>> deliveries;
for (auto& it : origin_group_reports) {
const OriginGroup& origin_group = it.first;
const url::Origin& report_origin = origin_group.first;
const std::string& group = origin_group.second;

if (base::ContainsKey(pending_origin_groups_, origin_group))
continue;

const ReportingClient* client =
endpoint_manager()->FindClientForOriginAndGroup(origin_group.first,
origin_group.second);
endpoint_manager()->FindClientForOriginAndGroup(report_origin, group);
if (client == nullptr) {
continue;
}
cache()->MarkClientUsed(client);
OriginEndpoint report_origin_endpoint(report_origin, client->endpoint);

Delivery* delivery;
auto delivery_it = deliveries.find(client->endpoint);
auto delivery_it = deliveries.find(report_origin_endpoint);
if (delivery_it == deliveries.end()) {
auto new_delivery = std::make_unique<Delivery>(client->endpoint);
auto new_delivery = std::make_unique<Delivery>(report_origin_endpoint);
delivery = new_delivery.get();
deliveries[client->endpoint] = std::move(new_delivery);
deliveries[report_origin_endpoint] = std::move(new_delivery);
} else {
delivery = delivery_it->second.get();
}
Expand All @@ -188,11 +195,10 @@ class ReportingDeliveryAgentImpl : public ReportingDeliveryAgent,

// Start an upload for each delivery.
for (auto& it : deliveries) {
const GURL& endpoint = it.first;
const OriginEndpoint& report_origin_endpoint = it.first;
const GURL& endpoint = report_origin_endpoint.second;
std::unique_ptr<Delivery>& delivery = it.second;

endpoint_manager()->SetEndpointPending(endpoint);

std::string json;
SerializeReports(delivery->reports, tick_clock()->NowTicks(), &json);

Expand All @@ -214,15 +220,15 @@ class ReportingDeliveryAgentImpl : public ReportingDeliveryAgent,
{undelivered_reports.begin(), undelivered_reports.end()});
}

void OnUploadComplete(const std::unique_ptr<Delivery>& delivery,
void OnUploadComplete(std::unique_ptr<Delivery> delivery,
ReportingUploader::Outcome outcome) {
for (const auto& origin_and_pair : delivery->reports_per_client) {
const url::Origin& origin = origin_and_pair.first;
const url::Origin& client_origin = origin_and_pair.first;
for (const auto& endpoint_and_count : origin_and_pair.second) {
const GURL& endpoint = endpoint_and_count.first;
int report_count = endpoint_and_count.second;
cache()->IncrementEndpointDeliveries(
origin, endpoint, report_count,
client_origin, endpoint, report_count,
outcome == ReportingUploader::Outcome::SUCCESS);
}
}
Expand All @@ -241,10 +247,9 @@ class ReportingDeliveryAgentImpl : public ReportingDeliveryAgent,

for (const ReportingReport* report : delivery->reports) {
pending_origin_groups_.erase(
OriginGroup(url::Origin::Create(report->url), report->group));
OriginGroup(delivery->report_origin, report->group));
}

endpoint_manager()->ClearEndpointPending(delivery->endpoint);
cache()->ClearReportsPending(delivery->reports);
}

Expand Down
53 changes: 44 additions & 9 deletions net/reporting/reporting_delivery_agent_unittest.cc
View file
Expand Up @@ -95,6 +95,15 @@ TEST_F(ReportingDeliveryAgentTest, SuccessfulImmediateUpload) {
cache()->GetReports(&reports);
EXPECT_TRUE(reports.empty());

{
ReportingCache::ClientStatistics stats =
cache()->GetStatisticsForOriginAndEndpoint(kOrigin_, kEndpoint_);
EXPECT_EQ(1, stats.attempted_uploads);
EXPECT_EQ(1, stats.successful_uploads);
EXPECT_EQ(1, stats.attempted_reports);
EXPECT_EQ(1, stats.successful_reports);
}

// TODO(dcreager): Check that BackoffEntry was informed of success.
}

Expand Down Expand Up @@ -134,6 +143,15 @@ TEST_F(ReportingDeliveryAgentTest, SuccessfulImmediateSubdomainUpload) {
cache()->GetReports(&reports);
EXPECT_TRUE(reports.empty());

{
ReportingCache::ClientStatistics stats =
cache()->GetStatisticsForOriginAndEndpoint(kOrigin_, kEndpoint_);
EXPECT_EQ(1, stats.attempted_uploads);
EXPECT_EQ(1, stats.successful_uploads);
EXPECT_EQ(1, stats.attempted_reports);
EXPECT_EQ(1, stats.successful_reports);
}

// TODO(dcreager): Check that BackoffEntry was informed of success.
}

Expand All @@ -155,6 +173,15 @@ TEST_F(ReportingDeliveryAgentTest,
ReportingClient::Subdomains::EXCLUDE);
pending_uploads()[0]->Complete(ReportingUploader::Outcome::SUCCESS);

{
ReportingCache::ClientStatistics stats =
cache()->GetStatisticsForOriginAndEndpoint(kOrigin_, kEndpoint_);
EXPECT_EQ(1, stats.attempted_uploads);
EXPECT_EQ(1, stats.successful_uploads);
EXPECT_EQ(1, stats.attempted_reports);
EXPECT_EQ(1, stats.successful_reports);
}

// Successful upload should remove delivered reports.
std::vector<const ReportingReport*> reports;
cache()->GetReports(&reports);
Expand Down Expand Up @@ -416,31 +443,39 @@ TEST_F(ReportingDeliveryAgentTest,
SetClient(kOrigin_, kEndpoint_, kGroup_);
SetClient(kDifferentOrigin, kEndpoint_, kGroup_);

// Trigger and complete an upload to start the delivery timer.
cache()->AddReport(kUrl_, kGroup_, kType_,
std::make_unique<base::DictionaryValue>(), 0,
tick_clock()->NowTicks(), 0);
pending_uploads()[0]->Complete(ReportingUploader::Outcome::SUCCESS);

// Now that the delivery timer is running, these reports won't be immediately
// uploaded.
cache()->AddReport(kUrl_, kGroup_, kType_,
std::make_unique<base::DictionaryValue>(), 0,
tick_clock()->NowTicks(), 0);
cache()->AddReport(kDifferentUrl, kGroup_, kType_,
std::make_unique<base::DictionaryValue>(), 0,
tick_clock()->NowTicks(), 0);
EXPECT_EQ(0u, pending_uploads().size());

// When we fire the delivery timer, we should NOT batch these two reports into
// a single upload, since each upload must only contain reports about a single
// origin.
EXPECT_TRUE(delivery_timer()->IsRunning());
delivery_timer()->Fire();
ASSERT_EQ(1u, pending_uploads().size());
ASSERT_EQ(2u, pending_uploads().size());

pending_uploads()[0]->Complete(ReportingUploader::Outcome::SUCCESS);
pending_uploads()[0]->Complete(ReportingUploader::Outcome::SUCCESS);
EXPECT_EQ(0u, pending_uploads().size());
}

// Test that the agent won't start a second upload to the same endpoint (even
// for a different origin) while one is pending, but will once it is no longer
// Test that the agent won't start a second upload to the same endpoint for a
// particular origin while one is pending, but will once it is no longer
// pending.
TEST_F(ReportingDeliveryAgentTest, SerializeUploadsToEndpoint) {
static const GURL kDifferentUrl("https://origin2/path");
static const url::Origin kDifferentOrigin =
url::Origin::Create(kDifferentUrl);

SetClient(kOrigin_, kEndpoint_, kGroup_);
SetClient(kDifferentOrigin, kEndpoint_, kGroup_);

cache()->AddReport(kUrl_, kGroup_, kType_,
std::make_unique<base::DictionaryValue>(), 0,
Expand All @@ -450,7 +485,7 @@ TEST_F(ReportingDeliveryAgentTest, SerializeUploadsToEndpoint) {
delivery_timer()->Fire();
EXPECT_EQ(1u, pending_uploads().size());

cache()->AddReport(kDifferentUrl, kGroup_, kType_,
cache()->AddReport(kUrl_, kGroup_, kType_,
std::make_unique<base::DictionaryValue>(), 0,
tick_clock()->NowTicks(), 0);

Expand Down
14 changes: 0 additions & 14 deletions net/reporting/reporting_endpoint_manager.cc
View file
Expand Up @@ -51,8 +51,6 @@ class ReportingEndpointManagerImpl : public ReportingEndpointManager {
for (const ReportingClient* client : clients) {
if (client->expires < now)
continue;
if (base::ContainsKey(pending_endpoints_, client->endpoint))
continue;
if (base::ContainsKey(endpoint_backoff_, client->endpoint) &&
endpoint_backoff_[client->endpoint]->ShouldRejectRequest()) {
continue;
Expand Down Expand Up @@ -97,16 +95,6 @@ class ReportingEndpointManagerImpl : public ReportingEndpointManager {
return nullptr;
}

void SetEndpointPending(const GURL& endpoint) override {
DCHECK(!base::ContainsKey(pending_endpoints_, endpoint));
pending_endpoints_.insert(endpoint);
}

void ClearEndpointPending(const GURL& endpoint) override {
DCHECK(base::ContainsKey(pending_endpoints_, endpoint));
pending_endpoints_.erase(endpoint);
}

void InformOfEndpointRequest(const GURL& endpoint, bool succeeded) override {
if (!base::ContainsKey(endpoint_backoff_, endpoint)) {
endpoint_backoff_[endpoint] = std::make_unique<BackoffEntry>(
Expand All @@ -125,8 +113,6 @@ class ReportingEndpointManagerImpl : public ReportingEndpointManager {

RandIntCallback rand_callback_;

std::set<GURL> pending_endpoints_;

// Note: Currently the ReportingBrowsingDataRemover does not clear this data
// because it's not persisted to disk. If it's ever persisted, it will need
// to be cleared as well.
Expand Down
7 changes: 0 additions & 7 deletions net/reporting/reporting_endpoint_manager.h
View file
Expand Up @@ -47,13 +47,6 @@ class NET_EXPORT ReportingEndpointManager {
const url::Origin& origin,
const std::string& group) = 0;

// Adds |endpoint| to the set of pending endpoints, preventing it from being
// chosen for a second parallel delivery attempt.
virtual void SetEndpointPending(const GURL& endpoint) = 0;

// Removes |endpoint| from the set of pending endpoints.
virtual void ClearEndpointPending(const GURL& endpoint) = 0;

// Informs the EndpointManager of a successful or unsuccessful request made to
// |endpoint| so it can manage exponential backoff of failing endpoints.
virtual void InformOfEndpointRequest(const GURL& endpoint,
Expand Down
41 changes: 20 additions & 21 deletions net/reporting/reporting_endpoint_manager_unittest.cc
View file
Expand Up @@ -21,6 +21,19 @@ namespace net {
namespace {

class ReportingEndpointManagerTest : public ReportingTestBase {
public:
ReportingEndpointManagerTest() {
ReportingPolicy policy;
policy.endpoint_backoff_policy.num_errors_to_ignore = 0;
policy.endpoint_backoff_policy.initial_delay_ms = 60000;
policy.endpoint_backoff_policy.multiply_factor = 2.0;
policy.endpoint_backoff_policy.jitter_factor = 0.0;
policy.endpoint_backoff_policy.maximum_backoff_ms = -1;
policy.endpoint_backoff_policy.entry_lifetime_ms = 0;
policy.endpoint_backoff_policy.always_use_initial_delay = false;
UsePolicy(policy);
}

protected:
void SetClient(const GURL& endpoint, int priority, int weight) {
cache()->SetClient(kOrigin_, endpoint, ReportingClient::Subdomains::EXCLUDE,
Expand Down Expand Up @@ -60,23 +73,6 @@ TEST_F(ReportingEndpointManagerTest, ExpiredEndpoint) {
EXPECT_TRUE(client == nullptr);
}

TEST_F(ReportingEndpointManagerTest, PendingEndpoint) {
SetClient(kEndpoint_, ReportingClient::kDefaultPriority,
ReportingClient::kDefaultWeight);

endpoint_manager()->SetEndpointPending(kEndpoint_);

const ReportingClient* client =
endpoint_manager()->FindClientForOriginAndGroup(kOrigin_, kGroup_);
EXPECT_TRUE(client == nullptr);

endpoint_manager()->ClearEndpointPending(kEndpoint_);

client = endpoint_manager()->FindClientForOriginAndGroup(kOrigin_, kGroup_);
ASSERT_TRUE(client != nullptr);
EXPECT_EQ(kEndpoint_, client->endpoint);
}

TEST_F(ReportingEndpointManagerTest, BackedOffEndpoint) {
ASSERT_EQ(2.0, policy().endpoint_backoff_policy.multiply_factor);

Expand Down Expand Up @@ -182,14 +178,17 @@ TEST_F(ReportingEndpointManagerTest, Priority) {
ASSERT_TRUE(client != nullptr);
EXPECT_EQ(kPrimaryEndpoint, client->endpoint);

endpoint_manager()->SetEndpointPending(kPrimaryEndpoint);

// The backoff policy we set up in the constructor means that a single failed
// upload will take the primary endpoint out of contention. This should cause
// us to choose the backend endpoint.
endpoint_manager()->InformOfEndpointRequest(kPrimaryEndpoint, false);
client = endpoint_manager()->FindClientForOriginAndGroup(kOrigin_, kGroup_);
ASSERT_TRUE(client != nullptr);
EXPECT_EQ(kBackupEndpoint, client->endpoint);

endpoint_manager()->ClearEndpointPending(kPrimaryEndpoint);

// Advance the current time far enough to clear out the primary endpoint's
// backoff clock. This should bring the primary endpoint back into play.
tick_clock()->Advance(base::TimeDelta::FromMinutes(2));
client = endpoint_manager()->FindClientForOriginAndGroup(kOrigin_, kGroup_);
ASSERT_TRUE(client != nullptr);
EXPECT_EQ(kPrimaryEndpoint, client->endpoint);
Expand Down

0 comments on commit c98446b

Please sign in to comment.