Revert "Reporting: Use BACKGROUND_SYNC permission to control uploads"

This reverts commit de03f4c9.

Reason for revert: CL introduced a ReportingPermissionsChecker class which appears to be intended to safely "cancel" UI-thread work posted from the IO thread, when the class instance is destroyed. The class, however, is always destroyed on the IO thread, so this use of WeakPtrs does not have the intended effect.

Original change's description:
> Reporting: Use BACKGROUND_SYNC permission to control uploads
>
> We now use the BACKGROUND_SYNC permission to decide whether or not to
> upload reports for a particular origin.  Note that as currently written,
> we don't use this permission to decide whether to *collect* the reports,
> only whether to *send* them.  Whether or not to collect is controlled
> by the site's cookie settings.
>
> Bug: 704259
> Change-Id: I059019ab85106c26f4d156e9f5d61e2f8b8fa757
> Reviewed-on: https://chromium-review.googlesource.com/937572
> Reviewed-by: Ryan Hamilton <rch@chromium.org>
> Commit-Queue: Douglas Creager <dcreager@google.com>
> Cr-Commit-Position: refs/heads/master@{#540257}

TBR=rch@chromium.org,juliatuttle@chromium.org,dcreager@google.com

Bug: 704259
Change-Id: I9cb1990f4273f74e39de83b99a12e6bfc00e7348
Reviewed-on: https://chromium-review.googlesource.com/1056727
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Wez <wez@chromium.org>
Cr-Commit-Position: refs/heads/master@{#558160}

chrome/browser/BUILD.gn

@@ -808,8 +808,6 @@ jumbo_split_static_library("browser") {
     "net/quota_policy_channel_id_store.h",
     "net/referrer.cc",
     "net/referrer.h",
-    "net/reporting_permissions_checker.cc",
-    "net/reporting_permissions_checker.h",
     "net/safe_search_util.cc",
     "net/safe_search_util.h",
     "net/service_providers_win.cc",

chrome/browser/net/chrome_network_delegate.cc

@@ -521,7 +521,7 @@ bool ChromeNetworkDelegate::OnCancelURLRequestWithPolicyViolatingReferrerHeader(
 bool ChromeNetworkDelegate::OnCanQueueReportingReport(
     const url::Origin& origin) const {
   if (!cookie_settings_)
-    return false;
+    return true;
 
   return cookie_settings_->IsCookieAccessAllowed(origin.GetURL(),
                                                  origin.GetURL());
@@ -530,21 +530,28 @@ bool ChromeNetworkDelegate::OnCanQueueReportingReport(
 void ChromeNetworkDelegate::OnCanSendReportingReports(
     std::set<url::Origin> origins,
     base::OnceCallback<void(std::set<url::Origin>)> result_callback) const {
-  if (!reporting_permissions_checker_) {
-    origins.clear();
+  if (!cookie_settings_) {
     std::move(result_callback).Run(std::move(origins));
     return;
   }
 
-  reporting_permissions_checker_->FilterReportingOrigins(
-      std::move(origins), std::move(result_callback));
+  for (auto it = origins.begin(); it != origins.end();) {
+    const auto& origin = *it;
+    if (!cookie_settings_->IsCookieAccessAllowed(origin.GetURL(),
+                                                 origin.GetURL())) {
+      origins.erase(it++);
+    } else {
+      ++it;
+    }
+  }
+  std::move(result_callback).Run(std::move(origins));
 }
 
 bool ChromeNetworkDelegate::OnCanSetReportingClient(
     const url::Origin& origin,
     const GURL& endpoint) const {
   if (!cookie_settings_)
-    return false;
+    return true;
 
   return cookie_settings_->IsCookieAccessAllowed(endpoint, origin.GetURL());
 }
@@ -553,7 +560,7 @@ bool ChromeNetworkDelegate::OnCanUseReportingClient(
     const url::Origin& origin,
     const GURL& endpoint) const {
   if (!cookie_settings_)
-    return false;
+    return true;
 
   return cookie_settings_->IsCookieAccessAllowed(endpoint, origin.GetURL());
 }

chrome/browser/net/chrome_network_delegate.h

@@ -17,7 +17,6 @@
 #include "base/memory/ref_counted.h"
 #include "base/values.h"
 #include "build/build_config.h"
-#include "chrome/browser/net/reporting_permissions_checker.h"
 #include "chrome/browser/net/safe_search_util.h"
 #include "components/domain_reliability/monitor.h"
 #include "components/prefs/pref_member.h"
@@ -107,16 +106,6 @@ class ChromeNetworkDelegate : public net::NetworkDelegateImpl {
     return domain_reliability_monitor_.get();
   }
 
-  void set_reporting_permissions_checker(
-      std::unique_ptr<ReportingPermissionsChecker>
-          reporting_permissions_checker) {
-    reporting_permissions_checker_ = std::move(reporting_permissions_checker);
-  }
-
-  ReportingPermissionsChecker* reporting_permissions_checker() {
-    return reporting_permissions_checker_.get();
-  }
-
   void set_data_use_aggregator(
       data_usage::DataUseAggregator* data_use_aggregator,
       bool is_data_usage_off_the_record);
@@ -225,7 +214,6 @@ class ChromeNetworkDelegate : public net::NetworkDelegateImpl {
   // Weak, owned by our owner.
   std::unique_ptr<domain_reliability::DomainReliabilityMonitor>
       domain_reliability_monitor_;
-  std::unique_ptr<ReportingPermissionsChecker> reporting_permissions_checker_;
 
   bool experimental_web_platform_features_enabled_;
 

chrome/browser/net/chrome_network_delegate_unittest.cc

@@ -18,9 +18,7 @@
 #include "build/build_config.h"
 #include "chrome/browser/browser_process.h"
 #include "chrome/browser/content_settings/cookie_settings_factory.h"
-#include "chrome/browser/net/reporting_permissions_checker.h"
 #include "chrome/browser/net/safe_search_util.h"
-#include "chrome/browser/profiles/profile_manager.h"
 #include "chrome/common/pref_names.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "chrome/test/base/testing_profile.h"
@@ -36,7 +34,6 @@
 #include "content/public/common/content_switches.h"
 #include "content/public/common/previews_state.h"
 #include "content/public/common/resource_type.h"
-#include "content/public/test/mock_permission_manager.h"
 #include "content/public/test/test_browser_thread_bundle.h"
 #include "content/public/test/test_utils.h"
 #include "extensions/buildflags/buildflags.h"
@@ -158,7 +155,7 @@ class ChromeNetworkDelegateTest : public testing::Test {
     ASSERT_TRUE(profile_manager_->SetUp());
   }
 
-  virtual void Initialize() {
+  void Initialize() {
     network_delegate_.reset(
         new ChromeNetworkDelegate(forwarder(), &enable_referrers_));
     context_->set_client_socket_factory(&socket_factory_);
@@ -650,74 +647,3 @@ TEST(ChromeNetworkDelegateStaticTest, IsAccessAllowed) {
   EXPECT_FALSE(IsAccessAllowed(external_storage_path.AsUTF8Unsafe(), ""));
 #endif
 }
-
-namespace {
-
-class TestingPermissionProfile : public TestingProfile {
- public:
-  TestingPermissionProfile() = default;
-
-  content::MockPermissionManager* mock_permission_manager() {
-    return &mock_permission_manager_;
-  }
-
-  content::PermissionManager* GetPermissionManager() override {
-    return &mock_permission_manager_;
-  }
-
- private:
-  content::MockPermissionManager mock_permission_manager_;
-};
-
-}  // namespace
-
-class ChromeNetworkDelegateReportingTest : public ChromeNetworkDelegateTest {
- public:
-  ChromeNetworkDelegateReportingTest() = default;
-
-  content::MockPermissionManager* mock_permission_manager() {
-    return profile_.mock_permission_manager();
-  }
-
-  void Initialize() override {
-    ChromeNetworkDelegateTest::Initialize();
-    chrome_network_delegate()->set_reporting_permissions_checker(
-        std::make_unique<ReportingPermissionsChecker>(&profile_));
-  }
-
- protected:
-  TestingPermissionProfile profile_;
-  DISALLOW_COPY_AND_ASSIGN(ChromeNetworkDelegateReportingTest);
-};
-
-TEST_F(ChromeNetworkDelegateReportingTest, ChecksReportingPermissions) {
-  using testing::_;
-  using testing::Return;
-
-  Initialize();
-
-  auto origin1 = url::Origin::Create(GURL("https://example.com/"));
-  auto origin2 = url::Origin::Create(GURL("https://foo.example.com/"));
-
-  std::set<url::Origin> origins = {origin1, origin2};
-
-  EXPECT_CALL(*mock_permission_manager(),
-              GetPermissionStatus(_, origin1.GetURL(), _))
-      .WillOnce(Return(blink::mojom::PermissionStatus::GRANTED));
-  EXPECT_CALL(*mock_permission_manager(),
-              GetPermissionStatus(_, origin2.GetURL(), _))
-      .WillOnce(Return(blink::mojom::PermissionStatus::DENIED));
-
-  std::set<url::Origin> allowed_origins;
-  chrome_network_delegate()->CanSendReportingReports(
-      std::move(origins),
-      base::BindOnce(
-          [](std::set<url::Origin>* dest, std::set<url::Origin> result) {
-            *dest = std::move(result);
-          },
-          &allowed_origins));
-  content::RunAllTasksUntilIdle();
-
-  ASSERT_EQ(1u, allowed_origins.size());
-  EXPECT_EQ(origin1, *allowed_origins.begin());
-}

chrome/browser/net/reporting_permissions_checker.cc

-// Copyright (c) 2018 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chrome/browser/net/reporting_permissions_checker.h"
-
-#include "base/bind.h"
-#include "chrome/browser/profiles/profile.h"
-#include "content/public/browser/browser_thread.h"
-#include "content/public/browser/permission_manager.h"
-#include "content/public/browser/permission_type.h"
-#include "url/gurl.h"
-#include "url/origin.h"
-
-ReportingPermissionsChecker::ReportingPermissionsChecker(Profile* profile)
-    : profile_(profile), const_weak_factory_(this) {}
-
-ReportingPermissionsChecker::~ReportingPermissionsChecker() = default;
-
-void ReportingPermissionsChecker::FilterReportingOrigins(
-    std::set<url::Origin> origins,
-    base::OnceCallback<void(std::set<url::Origin>)> result_callback) const {
-  content::BrowserThread::PostTask(
-      content::BrowserThread::UI, FROM_HERE,
-      base::BindOnce(
-          &ReportingPermissionsChecker::FilterReportingOriginsInUIThread,
-          const_weak_factory_.GetWeakPtr(), std::move(origins),
-          std::move(result_callback)));
-}
-
-void ReportingPermissionsChecker::FilterReportingOriginsInUIThread(
-    std::set<url::Origin> origins,
-    base::OnceCallback<void(std::set<url::Origin>)> result_callback) const {
-  content::PermissionManager* permission_manager =
-      profile_->GetPermissionManager();
-
-  if (permission_manager) {
-    for (auto it = origins.begin(); it != origins.end();) {
-      GURL origin = it->GetURL();
-      bool allowed = permission_manager->GetPermissionStatus(
-                         content::PermissionType::BACKGROUND_SYNC, origin,
-                         origin) == blink::mojom::PermissionStatus::GRANTED;
-      if (!allowed) {
-        origins.erase(it++);
-      } else {
-        ++it;
-      }
-    }
-  }
-
-  content::BrowserThread::PostTask(
-      content::BrowserThread::IO, FROM_HERE,
-      base::BindOnce(std::move(result_callback), std::move(origins)));
-}

chrome/browser/net/reporting_permissions_checker.h

-// Copyright (c) 2018 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#ifndef CHROME_BROWSER_NET_REPORTING_PERMISSIONS_CHECKER_H_
-#define CHROME_BROWSER_NET_REPORTING_PERMISSIONS_CHECKER_H_
-
-#include <set>
-
-#include "base/callback.h"
-#include "base/memory/weak_ptr.h"
-
-class Profile;
-
-namespace url {
-class Origin;
-}
-
-// Used by the Reporting API to check whether the user has allowed reports to be
-// uploaded for particular origins, via the BACKGROUND_SYNC permission.
-class ReportingPermissionsChecker {
- public:
-  // Does not take ownership of |profile|, which must outlive this instance.
-  explicit ReportingPermissionsChecker(Profile* profile);
-
-  ~ReportingPermissionsChecker();
-
-  // Checks whether each origin in |origins| has the BACKGROUND_SYNC permission
-  // set, removing any that don't.  Call this from the IO thread.  The filter
-  // will perform the check on the IO thread, and invoke |result_callback| back
-  // on the IO thread with the result.
-  void FilterReportingOrigins(
-      std::set<url::Origin> origins,
-      base::OnceCallback<void(std::set<url::Origin>)> result_callback) const;
-
- private:
-  void FilterReportingOriginsInUIThread(
-      std::set<url::Origin> origins,
-      base::OnceCallback<void(std::set<url::Origin>)> result_callback) const;
-
-  Profile* profile_;
-  mutable base::WeakPtrFactory<const ReportingPermissionsChecker>
-      const_weak_factory_;
-};
-
-#endif  // CHROME_BROWSER_NET_REPORTING_PERMISSIONS_CHECKER_H_

chrome/browser/net/reporting_permissions_checker_unittest.cc

-// Copyright (c) 2018 The Chromium Authors. All rights reserved.
-// Use of this source code is governed by a BSD-style license that can be
-// found in the LICENSE file.
-
-#include "chrome/browser/net/reporting_permissions_checker.h"
-
-#include <set>
-
-#include "base/bind.h"
-#include "chrome/test/base/testing_profile.h"
-#include "content/public/test/mock_permission_manager.h"
-#include "content/public/test/test_browser_thread_bundle.h"
-#include "content/public/test/test_utils.h"
-#include "testing/gtest/include/gtest/gtest.h"
-#include "url/gurl.h"
-#include "url/origin.h"
-
-namespace {
-
-using testing::_;
-using testing::Eq;
-using testing::IsEmpty;
-using testing::Return;
-using testing::UnorderedElementsAre;
-
-class TestingPermissionProfile : public TestingProfile {
- public:
-  TestingPermissionProfile() = default;
-
-  content::MockPermissionManager* mock_permission_manager() {
-    return &mock_permission_manager_;
-  }
-
-  content::PermissionManager* GetPermissionManager() override {
-    return &mock_permission_manager_;
-  }
-
- private:
-  content::MockPermissionManager mock_permission_manager_;
-};
-
-}  // namespace
-
-class ReportingPermissionsCheckerTest : public testing::Test {
- public:
-  ReportingPermissionsCheckerTest()
-      : thread_bundle_(content::TestBrowserThreadBundle::IO_MAINLOOP),
-        reporting_permissions_checker_(&profile_) {}
-
-  content::MockPermissionManager* mock_permission_manager() {
-    return profile_.mock_permission_manager();
-  }
-
- protected:
-  content::TestBrowserThreadBundle thread_bundle_;
-  TestingPermissionProfile profile_;
-  ReportingPermissionsChecker reporting_permissions_checker_;
-  DISALLOW_COPY_AND_ASSIGN(ReportingPermissionsCheckerTest);
-};
-
-TEST_F(ReportingPermissionsCheckerTest, ChecksReportingPermissionsBothGranted) {
-  auto origin1 = url::Origin::Create(GURL("https://example.com/"));
-  auto origin2 = url::Origin::Create(GURL("https://foo.example.com/"));
-
-  std::set<url::Origin> origins = {origin1, origin2};
-
-  EXPECT_CALL(*mock_permission_manager(),
-              GetPermissionStatus(_, origin1.GetURL(), _))
-      .WillOnce(Return(blink::mojom::PermissionStatus::GRANTED));
-  EXPECT_CALL(*mock_permission_manager(),
-              GetPermissionStatus(_, origin2.GetURL(), _))
-      .WillOnce(Return(blink::mojom::PermissionStatus::GRANTED));
-
-  std::set<url::Origin> allowed_origins;
-  reporting_permissions_checker_.FilterReportingOrigins(
-      std::move(origins),
-      base::BindOnce(
-          [](std::set<url::Origin>* dest, std::set<url::Origin> result) {
-            *dest = std::move(result);
-          },
-          &allowed_origins));
-  content::RunAllTasksUntilIdle();
-
-  EXPECT_THAT(allowed_origins, UnorderedElementsAre(Eq(origin1), Eq(origin2)));
-}
-
-TEST_F(ReportingPermissionsCheckerTest, ChecksReportingPermissionsBothDenied) {
-  auto origin1 = url::Origin::Create(GURL("https://example.com/"));
-  auto origin2 = url::Origin::Create(GURL("https://foo.example.com/"));
-
-  std::set<url::Origin> origins = {origin1, origin2};
-
-  EXPECT_CALL(*mock_permission_manager(),
-              GetPermissionStatus(_, origin1.GetURL(), _))
-      .WillOnce(Return(blink::mojom::PermissionStatus::DENIED));
-  EXPECT_CALL(*mock_permission_manager(),
-              GetPermissionStatus(_, origin2.GetURL(), _))
-      .WillOnce(Return(blink::mojom::PermissionStatus::DENIED));
-
-  std::set<url::Origin> allowed_origins;
-  reporting_permissions_checker_.FilterReportingOrigins(
-      std::move(origins),
-      base::BindOnce(
-          [](std::set<url::Origin>* dest, std::set<url::Origin> result) {
-            *dest = std::move(result);
-          },
-          &allowed_origins));
-  content::RunAllTasksUntilIdle();
-
-  EXPECT_THAT(allowed_origins, IsEmpty());
-}
-
-TEST_F(ReportingPermissionsCheckerTest, ChecksReportingPermissionsMixed) {
-  auto origin1 = url::Origin::Create(GURL("https://example.com/"));
-  auto origin2 = url::Origin::Create(GURL("https://foo.example.com/"));
-
-  std::set<url::Origin> origins = {origin1, origin2};
-
-  EXPECT_CALL(*mock_permission_manager(),
-              GetPermissionStatus(_, origin1.GetURL(), _))
-      .WillOnce(Return(blink::mojom::PermissionStatus::GRANTED));
-  EXPECT_CALL(*mock_permission_manager(),
-              GetPermissionStatus(_, origin2.GetURL(), _))
-      .WillOnce(Return(blink::mojom::PermissionStatus::DENIED));
-
-  std::set<url::Origin> allowed_origins;
-  reporting_permissions_checker_.FilterReportingOrigins(
-      std::move(origins),
-      base::BindOnce(
-          [](std::set<url::Origin>* dest, std::set<url::Origin> result) {
-            *dest = std::move(result);
-          },
-          &allowed_origins));
-  content::RunAllTasksUntilIdle();
-
-  EXPECT_THAT(allowed_origins, UnorderedElementsAre(Eq(origin1)));
-}

chrome/browser/profiles/profile_impl_io_data.cc

@@ -161,8 +161,6 @@ void ProfileImplIOData::Handle::Init(
   lazy_params->special_storage_policy = special_storage_policy;
   lazy_params->domain_reliability_monitor =
       std::move(domain_reliability_monitor);
-  lazy_params->reporting_permissions_checker =
-      std::make_unique<ReportingPermissionsChecker>(profile_);
 
   io_data_->lazy_params_.reset(lazy_params);
 
@@ -430,11 +428,6 @@ ProfileImplIOData::ConfigureNetworkDelegate(
         std::move(lazy_params_->domain_reliability_monitor));
   }
 
-  if (lazy_params_->reporting_permissions_checker) {
-    chrome_network_delegate->set_reporting_permissions_checker(
-        std::move(lazy_params_->reporting_permissions_checker));
-  }
-
   return data_reduction_proxy_io_data()->CreateNetworkDelegate(
       io_thread->globals()->data_use_ascriber->CreateNetworkDelegate(
           std::move(chrome_network_delegate),

chrome/browser/profiles/profile_impl_io_data.h

@@ -11,7 +11,6 @@
 #include "base/memory/ref_counted.h"
 #include "chrome/browser/custom_handlers/protocol_handler_registry.h"
 #include "chrome/browser/net/chrome_url_request_context_getter.h"
-#include "chrome/browser/net/reporting_permissions_checker.h"
 #include "chrome/browser/profiles/profile_io_data.h"
 #include "components/prefs/pref_store.h"
 
@@ -133,7 +132,6 @@ class ProfileImplIOData : public ProfileIOData {
     scoped_refptr<storage::SpecialStoragePolicy> special_storage_policy;
     std::unique_ptr<domain_reliability::DomainReliabilityMonitor>
         domain_reliability_monitor;
-    std::unique_ptr<ReportingPermissionsChecker> reporting_permissions_checker;
   };
 
   ProfileImplIOData();

chrome/test/BUILD.gn

@@ -2409,7 +2409,6 @@ test("unit_tests") {
     "../browser/net/predictor_unittest.cc",
     "../browser/net/probe_message_unittest.cc",
     "../browser/net/quota_policy_channel_id_store_unittest.cc",
-    "../browser/net/reporting_permissions_checker_unittest.cc",
     "../browser/net/safe_search_util_unittest.cc",
     "../browser/net/spdyproxy/data_reduction_proxy_chrome_settings_unittest.cc",
     "../browser/net/spdyproxy/data_reduction_proxy_settings_unittest_android.cc",