Reporting: Properly support endpoints with includeSubdomains.

net/reporting/reporting_cache.cc

Index: net/reporting/reporting_cache.cc
diff --git a/net/reporting/reporting_cache.cc b/net/reporting/reporting_cache.cc
index 9e43153b4f96556ec8017a29a9d3d2365ef90943..f1da1cf59d3137f411f613cf1b9ecbfb69296162 100644
--- a/net/reporting/reporting_cache.cc
+++ b/net/reporting/reporting_cache.cc
@@ -20,6 +20,18 @@
 
 namespace net {
 
+namespace {
+
+std::string GetSuperdomain(const std::string& domain) {

[shivanisha, 2017/04/10 20:08:41]

A brief comment on this function with an example of input and output domains.

[Julia Tuttle, 2017/04/12 16:11:37]

Done.

+  size_t dot_pos = domain.find('.');
+  if (dot_pos == std::string::npos)
+    return "";
+
+  return domain.substr(dot_pos + 1);
+}
+
+}  // namespace
+
 ReportingCache::ReportingCache(ReportingContext* context) : context_(context) {
   DCHECK(context_);
 }
@@ -88,12 +100,12 @@ void ReportingCache::IncrementReportsAttempts(
 void ReportingCache::RemoveReports(
     const std::vector<const ReportingReport*>& reports) {
   for (const ReportingReport* report : reports) {
-    DCHECK(base::ContainsKey(reports_, report));
     if (base::ContainsKey(pending_reports_, report))
       doomed_reports_.insert(report);
     else {

[shivanisha, 2017/04/10 20:08:41]

For readability, if else has {} then include in the if block as well, and vice
versa. 

[Julia Tuttle, 2017/04/12 16:11:37]

Done.

       DCHECK(!base::ContainsKey(doomed_reports_, report));
-      reports_.erase(report);
+      size_t erased = reports_.erase(report);
+      DCHECK_EQ(1u, erased);

[shivanisha, 2017/04/10 20:08:41]

Why removing the dcheck above? It is still applicable to pending_reports_ as
well, right?

[Julia Tuttle, 2017/04/12 16:11:37]

If a caller passes in a random Report pointer that's not in the cache, it won't
be in pending_reports_, so it will execute the else block, but erased will be 0
because it's not in reports_ either, and then it'll trip the DCHECK_EQ here.

I'm not concerned about internal errors in the cache that could leave reports in
pending_reports_ but not reports_.

     }
   }
 
@@ -133,12 +145,20 @@ void ReportingCache::GetClientsForOriginAndGroup(
   clients_out->clear();
 
   const auto it = clients_.find(origin);
-  if (it == clients_.end())
-    return;
+  if (it != clients_.end()) {
+    for (const auto& endpoint_and_client : it->second) {
+      if (endpoint_and_client.second->group == group)
+        clients_out->push_back(endpoint_and_client.second.get());
+    }
+  }

[shivanisha, 2017/04/10 20:08:41]

Should there be a return in the case when clients_out is populated above? As per
the comment below "If no clients were found..." , it seems like.

[Julia Tuttle, 2017/04/12 16:11:37]

Nope, it'll get to the while loop and clients_out->empty() will be false, so it
won't do anything else.

 
-  for (const auto& endpoint_and_client : it->second) {
-    if (endpoint_and_client.second->group == group)
-      clients_out->push_back(endpoint_and_client.second.get());
+  // If no clients were found, try successive superdomain suffixes until a
+  // client with includeSubdomains is found or there are no more domain
+  // components left.
+  std::string domain = origin.host();

[shivanisha, 2017/04/10 20:08:41]

I am not sure what origin.host returns. Does it return the complete host name
without the port number, if any?

[Julia Tuttle, 2017/04/12 16:11:37]

Yes, it's just the hostname, no port number.

+  while (clients_out->empty() && !domain.empty()) {
+    GetWildcardClientsForDomainAndGroup(domain, group, clients_out);
+    domain = GetSuperdomain(domain);
   }
 }
 
@@ -149,18 +169,25 @@ void ReportingCache::SetClient(const url::Origin& origin,
                                base::TimeTicks expires) {
   DCHECK(endpoint.SchemeIsCryptographic());
 
+  if (base::ContainsKey(clients_, origin) &&

[shivanisha, 2017/04/10 20:08:41]

Comment explaining why we are using the Wildcard related functions while setting
the client.

[Julia Tuttle, 2017/04/12 16:11:37]

Done.

+      base::ContainsKey(clients_[origin], endpoint)) {
+    MaybeRemoveWildcardClient(clients_[origin][endpoint].get());
+  }
+
   clients_[origin][endpoint] = base::MakeUnique<ReportingClient>(
       origin, endpoint, subdomains, group, expires);
 
+  MaybeAddWildcardClient(clients_[origin][endpoint].get());
+
   context_->NotifyCacheUpdated();
 }
 
 void ReportingCache::RemoveClients(
     const std::vector<const ReportingClient*>& clients_to_remove) {
   for (const ReportingClient* client : clients_to_remove) {
-    DCHECK(base::ContainsKey(clients_[client->origin], client->endpoint));
-    DCHECK(clients_[client->origin][client->endpoint].get() == client);
-    clients_[client->origin].erase(client->endpoint);
+    MaybeRemoveWildcardClient(client);
+    size_t erased = clients_[client->origin].erase(client->endpoint);
+    DCHECK_EQ(1u, erased);
   }
 
   context_->NotifyCacheUpdated();
@@ -168,24 +195,65 @@ void ReportingCache::RemoveClients(
 
 void ReportingCache::RemoveClientForOriginAndEndpoint(const url::Origin& origin,
                                                       const GURL& endpoint) {
-  DCHECK(base::ContainsKey(clients_, origin));
-  DCHECK(base::ContainsKey(clients_[origin], endpoint));
-  clients_[origin].erase(endpoint);
+  MaybeRemoveWildcardClient(clients_[origin][endpoint].get());
+  size_t erased = clients_[origin].erase(endpoint);
+  DCHECK_EQ(1u, erased);
 
   context_->NotifyCacheUpdated();
 }
 
 void ReportingCache::RemoveClientsForEndpoint(const GURL& endpoint) {
-  for (auto& it : clients_)
-    it.second.erase(endpoint);
+  for (auto& origin_and_endpoints : clients_) {
+    if (base::ContainsKey(origin_and_endpoints.second, endpoint)) {
+      MaybeRemoveWildcardClient(origin_and_endpoints.second[endpoint].get());
+      origin_and_endpoints.second.erase(endpoint);
+    }
+  }
 
   context_->NotifyCacheUpdated();
 }
 
 void ReportingCache::RemoveAllClients() {
   clients_.clear();
+  wildcard_clients_.clear();
 
   context_->NotifyCacheUpdated();
 }
 
+void ReportingCache::MaybeAddWildcardClient(const ReportingClient* client) {
+  if (client->subdomains != ReportingClient::Subdomains::INCLUDE)
+    return;
+
+  const std::string& domain = client->origin.host();
+  auto inserted = wildcard_clients_[domain].insert(client);
+  DCHECK(inserted.second);
+}
+
+void ReportingCache::MaybeRemoveWildcardClient(const ReportingClient* client) {
+  if (client->subdomains != ReportingClient::Subdomains::INCLUDE)
+    return;
+
+  const std::string& domain = client->origin.host();
+  size_t erased = wildcard_clients_[domain].erase(client);
+  DCHECK_EQ(1u, erased);
+}
+
+void ReportingCache::GetWildcardClientsForDomainAndGroup(
+    const std::string& domain,
+    const std::string& group,
+    std::vector<const ReportingClient*>* clients_out) const {
+  clients_out->clear();
+
+  auto it = wildcard_clients_.find(domain);
+  if (it == wildcard_clients_.end())
+    return;
+
+  for (const ReportingClient* client : it->second) {
+    if (client->group == group &&
+        client->subdomains == ReportingClient::Subdomains::INCLUDE) {

[shivanisha, 2017/04/10 20:08:41]

Should this be a dcheck instead of a condition since only clients with
ReportingClient::Subdomains::INCLUDE should be included in wildcard_clients_?

[Julia Tuttle, 2017/04/12 16:11:37]

Done.

+      clients_out->push_back(client);
+    }
+  }
+}
+
 }  // namespace net