Use empty network filter for deletions without filtering

When deleting with an EmptyBlacklist filter, we shouldn't create a
Network deletion filter to properly use optimized "delete all" code paths.

Bug: 843995
Cq-Include-Trybots: luci.chromium.try:linux_mojo
Change-Id: Ifc84f403dc2f2b50471e7fab273d20ebcba3f4f4
Reviewed-on: https://chromium-review.googlesource.com/1068736Reviewed-by: Mike West <mkwst@chromium.org>
Reviewed-by: Jochen Eisinger <jochen@chromium.org>
Reviewed-by: Martin Šrámek <msramek@chromium.org>
Commit-Queue: Christian Dullweber <dullweber@chromium.org>
Cr-Commit-Position: refs/heads/master@{#577127}

chrome/browser/browsing_data/chrome_browsing_data_remover_delegate_unittest.cc

@@ -1010,8 +1010,9 @@ class MockReportingService : public net::ReportingService {
   }
 
   void RemoveAllBrowsingData(int data_type_mask) override {
-    RemoveBrowsingData(data_type_mask,
-                       base::RepeatingCallback<bool(const GURL&)>());
+    ++remove_all_calls_;
+    last_data_type_mask_ = data_type_mask;
+    last_origin_filter_ = base::RepeatingCallback<bool(const GURL&)>();
   }
 
   int GetUploadDepth(const net::URLRequest& request) override {
@@ -1026,6 +1027,7 @@ class MockReportingService : public net::ReportingService {
   }
 
   int remove_calls() const { return remove_calls_; }
+  int remove_all_calls() const { return remove_all_calls_; }
   int last_data_type_mask() const { return last_data_type_mask_; }
   const base::RepeatingCallback<bool(const GURL&)>& last_origin_filter() const {
     return last_origin_filter_;
@@ -1033,6 +1035,7 @@ class MockReportingService : public net::ReportingService {
 
  private:
   int remove_calls_ = 0;
+  int remove_all_calls_ = 0;
   int last_data_type_mask_ = 0;
   base::RepeatingCallback<bool(const GURL&)> last_origin_filter_;
 
@@ -1059,16 +1062,7 @@ class ClearReportingCacheTester {
     request_context->set_reporting_service(old_service_);
   }
 
-  void GetMockInfo(
-      int* remove_calls_out,
-      int* last_data_type_mask_out,
-      base::RepeatingCallback<bool(const GURL&)>* last_origin_filter_out) {
-    DCHECK_NE(nullptr, service_.get());
-
-    *remove_calls_out = service_->remove_calls();
-    *last_data_type_mask_out = service_->last_data_type_mask();
-    *last_origin_filter_out = service_->last_origin_filter();
-  }
+  const MockReportingService& mock() { return *service_; }
 
  private:
   TestingProfile* profile_;
@@ -1099,17 +1093,19 @@ class MockNetworkErrorLoggingService : public net::NetworkErrorLoggingService {
   }
 
   void RemoveAllBrowsingData() override {
-    ++remove_calls_;
+    ++remove_all_calls_;
     last_origin_filter_ = base::RepeatingCallback<bool(const GURL&)>();
   }
 
   int remove_calls() const { return remove_calls_; }
+  int remove_all_calls() const { return remove_all_calls_; }
   const base::RepeatingCallback<bool(const GURL&)>& last_origin_filter() const {
     return last_origin_filter_;
   }
 
  private:
   int remove_calls_ = 0;
+  int remove_all_calls_ = 0;
   base::RepeatingCallback<bool(const GURL&)> last_origin_filter_;
 
   DISALLOW_COPY_AND_ASSIGN(MockNetworkErrorLoggingService);
@@ -1135,14 +1131,7 @@ class ClearNetworkErrorLoggingTester {
     request_context->set_network_error_logging_service(nullptr);
   }
 
-  void GetMockInfo(
-      int* remove_calls_out,
-      base::RepeatingCallback<bool(const GURL&)>* last_origin_filter_out) {
-    DCHECK_NE(nullptr, service_.get());
-
-    *remove_calls_out = service_->remove_calls();
-    *last_origin_filter_out = service_->last_origin_filter();
-  }
+  const MockNetworkErrorLoggingService& mock() { return *service_; };
 
  private:
   TestingProfile* profile_;
@@ -2780,16 +2769,12 @@ TEST_F(ChromeBrowsingDataRemoverDelegateTest, ReportingCache) {
       base::Time(), base::Time::Max(),
       ChromeBrowsingDataRemoverDelegate::DATA_TYPE_HISTORY, true);
 
-  int remove_count;
-  int data_type_mask;
-  base::RepeatingCallback<bool(const GURL&)> origin_filter;
-  tester.GetMockInfo(&remove_count, &data_type_mask, &origin_filter);
-
-  EXPECT_EQ(1, remove_count);
+  EXPECT_EQ(0, tester.mock().remove_calls());
+  EXPECT_EQ(1, tester.mock().remove_all_calls());
   EXPECT_EQ(net::ReportingBrowsingDataRemover::DATA_TYPE_REPORTS,
-            data_type_mask);
-  EXPECT_TRUE(ProbablySameFilters(BrowsingDataFilterBuilder::BuildNoopFilter(),
-                                  origin_filter));
+            tester.mock().last_data_type_mask());
+  EXPECT_TRUE(ProbablySameFilters(base::RepeatingCallback<bool(const GURL&)>(),
+                                  tester.mock().last_origin_filter()));
 }
 
 // TODO(crbug.com/589586): Disabled, since history is not yet marked as
@@ -2806,16 +2791,12 @@ TEST_F(ChromeBrowsingDataRemoverDelegateTest,
       base::Time(), base::Time::Max(),
       ChromeBrowsingDataRemoverDelegate::DATA_TYPE_HISTORY, builder->Copy());
 
-  int remove_count;
-  int data_type_mask;
-  base::RepeatingCallback<bool(const GURL&)> origin_filter;
-  tester.GetMockInfo(&remove_count, &data_type_mask, &origin_filter);
-
-  EXPECT_EQ(1, remove_count);
+  EXPECT_EQ(1, tester.mock().remove_calls());
+  EXPECT_EQ(0, tester.mock().remove_all_calls());
   EXPECT_EQ(net::ReportingBrowsingDataRemover::DATA_TYPE_REPORTS,
-            data_type_mask);
-  EXPECT_TRUE(
-      ProbablySameFilters(builder->BuildGeneralFilter(), origin_filter));
+            tester.mock().last_data_type_mask());
+  EXPECT_TRUE(ProbablySameFilters(builder->BuildGeneralFilter(),
+                                  tester.mock().last_origin_filter()));
 }
 
 TEST_F(ChromeBrowsingDataRemoverDelegateTest, NetworkErrorLogging_NoDelegate) {
@@ -2837,13 +2818,10 @@ TEST_F(ChromeBrowsingDataRemoverDelegateTest, NetworkErrorLogging_History) {
       base::Time(), base::Time::Max(),
       ChromeBrowsingDataRemoverDelegate::DATA_TYPE_HISTORY, true);
 
-  int remove_count;
-  base::RepeatingCallback<bool(const GURL&)> origin_filter;
-  tester.GetMockInfo(&remove_count, &origin_filter);
-
-  EXPECT_EQ(1, remove_count);
-  EXPECT_TRUE(ProbablySameFilters(BrowsingDataFilterBuilder::BuildNoopFilter(),
-                                  origin_filter));
+  EXPECT_EQ(0, tester.mock().remove_calls());
+  EXPECT_EQ(1, tester.mock().remove_all_calls());
+  EXPECT_TRUE(ProbablySameFilters(base::RepeatingCallback<bool(const GURL&)>(),
+                                  tester.mock().last_origin_filter()));
 }
 #endif  // BUILDFLAG(ENABLE_REPORTING)
 

content/browser/browsing_data/browsing_data_filter_builder_impl.cc

@@ -128,6 +128,8 @@ BrowsingDataFilterBuilderImpl::BuildGeneralFilter() const {
 
 network::mojom::ClearDataFilterPtr
 BrowsingDataFilterBuilderImpl::BuildNetworkServiceFilter() const {
+  if (IsEmptyBlacklist())
+    return nullptr;
   network::mojom::ClearDataFilterPtr filter =
       network::mojom::ClearDataFilter::New();
   filter->type = (mode_ == Mode::WHITELIST)

content/browser/browsing_data/browsing_data_remover_impl.cc

@@ -303,7 +303,7 @@ void BrowsingDataRemoverImpl::RemoveImpl(
 
     network::mojom::ClearDataFilterPtr service_filter =
         filter_builder.BuildNetworkServiceFilter();
-    DCHECK(service_filter->origins.empty())
+    DCHECK(!service_filter || service_filter->origins.empty())
         << "Origin-based deletion is not suitable for channel IDs.";
 
     BrowserContext::GetDefaultStoragePartition(browser_context_)

content/public/browser/browsing_data_filter_builder.h

@@ -69,7 +69,7 @@ class CONTENT_EXPORT BrowsingDataFilterBuilder {
   // Builds a filter that can be used with the network service. This uses a Mojo
   // struct rather than a predicate function (as used by the rest of the filters
   // built by this class) because we need to be able to pass the filter to the
-  // network service via IPC.
+  // network service via IPC. Returns nullptr if |IsEmptyBlacklist()| is true.
   virtual network::mojom::ClearDataFilterPtr BuildNetworkServiceFilter()
       const = 0;
 

services/network/public/mojom/network_context.mojom

@@ -353,6 +353,8 @@ interface NetworkContext {
   // Clears content from the HTTP cache. A specific range of time can be
   // specified with |start_time| and |end_time|. This supports unbounded deletes
   // in either direction by using null Time values for either argument.
+  // If a non-null |filter| is specified, will clear only entries matching the
+  // filter.
   ClearHttpCache(mojo_base.mojom.Time start_time,
                  mojo_base.mojom.Time end_time,
                  ClearDataFilter? filter) => ();