Revert "[iOS] Use a persistent cookie store for Safe Browsing"

This reverts commit 6503bbcb.

Reason for revert: SafeBrowsingServiceTest.PersistentCookies is failing on iPad-device https://ci.chromium.org/p/chrome/builders/ci/ipad-device/11375 

Original change's description:
> [iOS] Use a persistent cookie store for Safe Browsing
> 
> This makes SafeBrowsingService use a persistent cookie store,
> and clears this cookie store whenever cookies are cleared for
> "all time". This makes the behavior on iOS consistent with
> the behavior of Safe Browsing on other platforms.
> 
> Change-Id: I37fec41b6929386ecf2ce1e29fdbb8e95ac89383
> Bug: 1103219
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2291694
> Commit-Queue: Ali Juma <ajuma@chromium.org>
> Reviewed-by: Eugene But <eugenebut@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#796057}

TBR=ajuma@chromium.org,eugenebut@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 1103219
Change-Id: Ia5dadc043993898ddc339701f2742207f98ef41f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2344446Reviewed-by: Gauthier Ambard <gambard@chromium.org>
Commit-Queue: Gauthier Ambard <gambard@chromium.org>
Cr-Commit-Position: refs/heads/master@{#796323}

ios/chrome/browser/browsing_data/BUILD.gn

@@ -45,7 +45,6 @@ source_set("browsing_data") {
     "//components/open_from_clipboard",
     "//components/password_manager/core/browser",
     "//components/prefs",
-    "//components/safe_browsing/core:features",
     "//components/sessions",
     "//components/signin/ios/browser",
     "//components/translate/core/browser:browser",
@@ -58,7 +57,6 @@ source_set("browsing_data") {
     "//ios/chrome/browser/language",
     "//ios/chrome/browser/passwords",
     "//ios/chrome/browser/reading_list:reading_list_remover",
-    "//ios/chrome/browser/safe_browsing",
     "//ios/chrome/browser/search_engines",
     "//ios/chrome/browser/sessions",
     "//ios/chrome/browser/sessions:serialisation",

ios/chrome/browser/browsing_data/browsing_data_remover_impl.mm

@@ -33,7 +33,6 @@
 #include "components/open_from_clipboard/clipboard_recent_content.h"
 #include "components/password_manager/core/browser/password_store.h"
 #include "components/prefs/pref_service.h"
-#include "components/safe_browsing/core/features.h"
 #include "components/search_engines/template_url_service.h"
 #include "components/sessions/core/tab_restore_service.h"
 #include "components/signin/ios/browser/account_consistency_service.h"
@@ -53,7 +52,6 @@
 #include "ios/chrome/browser/language/url_language_histogram_factory.h"
 #include "ios/chrome/browser/passwords/ios_chrome_password_store_factory.h"
 #include "ios/chrome/browser/reading_list/reading_list_remover_helper.h"
-#import "ios/chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "ios/chrome/browser/search_engines/template_url_service_factory.h"
 #include "ios/chrome/browser/sessions/ios_chrome_tab_restore_service_factory.h"
 #import "ios/chrome/browser/sessions/session_service_ios.h"
@@ -296,24 +294,14 @@ void BrowsingDataRemoverImpl::RemoveImpl(base::Time delete_begin,
 
   if (IsRemoveDataMaskSet(mask, BrowsingDataRemoveMask::REMOVE_COOKIES)) {
     base::RecordAction(base::UserMetricsAction("ClearBrowsingData_Cookies"));
-    net::CookieDeletionInfo::TimeRange deletion_time_range =
-        net::CookieDeletionInfo::TimeRange(delete_begin, delete_end);
     base::PostTask(
         FROM_HERE, task_traits,
         base::BindOnce(
-            &ClearCookies, context_getter_, deletion_time_range,
+            &ClearCookies, context_getter_,
+            net::CookieDeletionInfo::TimeRange(delete_begin, delete_end),
             base::BindOnce(base::IgnoreResult(&base::TaskRunner::PostTask),
                            current_task_runner, FROM_HERE,
                            CreatePendingTaskCompletionClosure())));
-    if (base::FeatureList::IsEnabled(
-            safe_browsing::kSafeBrowsingAvailableOnIOS) &&
-        !browser_state_->IsOffTheRecord()) {
-      GetApplicationContext()->GetSafeBrowsingService()->ClearCookies(
-          deletion_time_range,
-          base::BindOnce(base::IgnoreResult(&base::TaskRunner::PostTask),
-                         current_task_runner, FROM_HERE,
-                         CreatePendingTaskCompletionClosure()));
-    }
   }
 
   // There is no need to clean the remaining types of data for off-the-record

ios/chrome/browser/net/BUILD.gn

@@ -32,6 +32,7 @@ source_set("net") {
     "//components/update_client",
     "//ios/chrome/browser",
     "//ios/chrome/browser/browser_state",
+    "//ios/chrome/browser/browsing_data",
     "//ios/net",
     "//ios/web/common",
     "//net",

ios/chrome/browser/safe_browsing/BUILD.gn

@@ -52,7 +52,6 @@ source_set("safe_browsing") {
     "//ios/chrome/browser/browser_state",
     "//ios/chrome/browser/content_settings",
     "//ios/chrome/browser/history",
-    "//ios/chrome/browser/net",
     "//ios/chrome/browser/prerender",
     "//ios/chrome/browser/signin",
     "//ios/chrome/browser/sync",
@@ -177,8 +176,6 @@ source_set("unit_tests") {
     "//ios/chrome/test:test_support",
     "//ios/web/public",
     "//ios/web/public/test",
-    "//net:test_support",
-    "//net/traffic_annotation:test_support",
     "//services/network/public/cpp",
     "//testing/gmock",
     "//testing/gtest",

ios/chrome/browser/safe_browsing/fake_safe_browsing_service.h

@@ -32,8 +32,6 @@ class FakeSafeBrowsingService : public SafeBrowsingService {
       web::WebState* web_state) override;
   bool CanCheckUrl(const GURL& url) const override;
   scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory() override;
-  void ClearCookies(const net::CookieDeletionInfo::TimeRange& creation_range,
-                    base::OnceClosure callback) override;
 
  protected:
   ~FakeSafeBrowsingService() override;

ios/chrome/browser/safe_browsing/fake_safe_browsing_service.mm

@@ -85,9 +85,3 @@ FakeSafeBrowsingService::GetURLLoaderFactory() {
   return base::MakeRefCounted<network::WeakWrapperSharedURLLoaderFactory>(
       &url_loader_factory_);
 }
-
-void FakeSafeBrowsingService::ClearCookies(
-    const net::CookieDeletionInfo::TimeRange& creation_range,
-    base::OnceClosure callback) {
-  DCHECK_CURRENTLY_ON(web::WebThread::UI);
-}

ios/chrome/browser/safe_browsing/safe_browsing_service.h

@@ -5,9 +5,7 @@
 #ifndef IOS_CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_SERVICE_H_
 #define IOS_CHROME_BROWSER_SAFE_BROWSING_SAFE_BROWSING_SERVICE_H_
 
-#include "base/callback.h"
 #include "base/memory/ref_counted.h"
-#include "net/cookies/cookie_deletion_info.h"
 #include "url/gurl.h"
 
 class PrefService;
@@ -60,12 +58,6 @@ class SafeBrowsingService
   virtual scoped_refptr<network::SharedURLLoaderFactory>
   GetURLLoaderFactory() = 0;
 
-  // Clears cookies if the given deletion time range is for "all time". Calls
-  // the given |callback| once deletion is complete.
-  virtual void ClearCookies(
-      const net::CookieDeletionInfo::TimeRange& creation_range,
-      base::OnceClosure callback) = 0;
-
  protected:
   SafeBrowsingService() = default;
   virtual ~SafeBrowsingService() = default;

ios/chrome/browser/safe_browsing/safe_browsing_service_impl.h

@@ -44,8 +44,6 @@ class SafeBrowsingServiceImpl : public SafeBrowsingService {
       web::WebState* web_state) override;
   bool CanCheckUrl(const GURL& url) const override;
   scoped_refptr<network::SharedURLLoaderFactory> GetURLLoaderFactory() override;
-  void ClearCookies(const net::CookieDeletionInfo::TimeRange& creation_range,
-                    base::OnceClosure callback) override;
 
  private:
   // A helper class for enabling/disabling Safe Browsing and maintaining state
@@ -65,8 +63,7 @@ class SafeBrowsingServiceImpl : public SafeBrowsingService {
     void Initialize(
         scoped_refptr<SafeBrowsingServiceImpl> safe_browsing_service,
         mojo::PendingReceiver<network::mojom::NetworkContext>
-            network_context_receiver,
-        const base::FilePath& safe_browsing_data_path);
+            network_context_receiver);
 
     // Disables Safe Browsing, and destroys the network context and URL loader
     // factory used by the SafeBrowsingDatabaseManager.
@@ -75,9 +72,6 @@ class SafeBrowsingServiceImpl : public SafeBrowsingService {
     // Enables or disables Safe Browsing database updates and lookups.
     void SetSafeBrowsingEnabled(bool enabled);
 
-    // Clears all cookies. Calls the given |callback| when deletion is complete.
-    void ClearAllCookies(base::OnceClosure callback);
-
    private:
     friend base::RefCountedThreadSafe<IOThreadEnabler>;
     ~IOThreadEnabler();
@@ -86,9 +80,8 @@ class SafeBrowsingServiceImpl : public SafeBrowsingService {
     // queries.
     void StartSafeBrowsingDBManager();
 
-    // Constructs a URLRequestContext, using the given path as the location for
-    // the cookie store.
-    void SetUpURLRequestContext(const base::FilePath& safe_browsing_data_path);
+    // Constructs a URLRequestContext.
+    void SetUpURLRequestContext();
 
     // Constructs a SharedURLLoaderFactory.
     void SetUpURLLoaderFactory(

ios/chrome/browser/safe_browsing/safe_browsing_service_impl.mm

@@ -18,14 +18,11 @@
 #include "components/safe_browsing/core/features.h"
 #include "components/safe_browsing/core/realtime/url_lookup_service.h"
 #import "ios/chrome/browser/browser_state/chrome_browser_state.h"
-#import "ios/chrome/browser/net/cookie_util.h"
 #import "ios/chrome/browser/safe_browsing/real_time_url_lookup_service_factory.h"
 #import "ios/chrome/browser/safe_browsing/url_checker_delegate_impl.h"
-#import "ios/net/cookies/system_cookie_store.h"
 #include "ios/web/public/thread/web_task_traits.h"
 #include "ios/web/public/thread/web_thread.h"
 #import "ios/web/public/web_state.h"
-#include "net/cookies/cookie_store.h"
 #include "net/url_request/url_request_context.h"
 #include "net/url_request/url_request_context_builder.h"
 #include "services/network/public/cpp/weak_wrapper_shared_url_loader_factory.h"
@@ -64,8 +61,7 @@ void SafeBrowsingServiceImpl::Initialize(PrefService* prefs,
       FROM_HERE, {web::WebThread::IO},
       base::BindOnce(&IOThreadEnabler::Initialize, io_thread_enabler_,
                      base::WrapRefCounted(this),
-                     network_context_client_.BindNewPipeAndPassReceiver(),
-                     safe_browsing_data_path));
+                     network_context_client_.BindNewPipeAndPassReceiver()));
 
   // Watch for changes to the Safe Browsing opt-out preference.
   pref_change_registrar_ = std::make_unique<PrefChangeRegistrar>();
@@ -115,21 +111,6 @@ SafeBrowsingServiceImpl::GetURLLoaderFactory() {
   return shared_url_loader_factory_;
 }
 
-void SafeBrowsingServiceImpl::ClearCookies(
-    const net::CookieDeletionInfo::TimeRange& creation_range,
-    base::OnceClosure callback) {
-  if (creation_range.start() == base::Time() &&
-      creation_range.end() == base::Time::Max()) {
-    base::PostTask(
-        FROM_HERE,
-        {web::WebThread::IO, base::TaskShutdownBehavior::BLOCK_SHUTDOWN},
-        base::BindOnce(&IOThreadEnabler::ClearAllCookies, io_thread_enabler_,
-                       std::move(callback)));
-  } else {
-    base::PostTask(FROM_HERE, {web::WebThread::IO}, std::move(callback));
-  }
-}
-
 void SafeBrowsingServiceImpl::SetUpURLLoaderFactory(
     mojo::PendingReceiver<network::mojom::URLLoaderFactory> receiver) {
   DCHECK_CURRENTLY_ON(web::WebThread::UI);
@@ -165,9 +146,8 @@ SafeBrowsingServiceImpl::IOThreadEnabler::~IOThreadEnabler() = default;
 void SafeBrowsingServiceImpl::IOThreadEnabler::Initialize(
     scoped_refptr<SafeBrowsingServiceImpl> safe_browsing_service,
     mojo::PendingReceiver<network::mojom::NetworkContext>
-        network_context_receiver,
-    const base::FilePath& safe_browsing_data_path) {
-  SetUpURLRequestContext(safe_browsing_data_path);
+        network_context_receiver) {
+  SetUpURLRequestContext();
   std::vector<std::string> cors_exempt_header_list;
   network_context_ = std::make_unique<network::NetworkContext>(
       /*network_service=*/nullptr, std::move(network_context_receiver),
@@ -198,15 +178,6 @@ void SafeBrowsingServiceImpl::IOThreadEnabler::SetSafeBrowsingEnabled(
     safe_browsing_db_manager_->StopOnIOThread(shutting_down_);
 }
 
-void SafeBrowsingServiceImpl::IOThreadEnabler::ClearAllCookies(
-    base::OnceClosure callback) {
-  DCHECK_CURRENTLY_ON(web::WebThread::IO);
-  net::CookieStore* cookie_store = url_request_context_->cookie_store();
-  cookie_store->DeleteAllAsync(base::BindOnce(
-      [](base::OnceClosure callback, uint32_t) { std::move(callback).Run(); },
-      std::move(callback)));
-}
-
 void SafeBrowsingServiceImpl::IOThreadEnabler::StartSafeBrowsingDBManager() {
   DCHECK_CURRENTLY_ON(web::WebThread::IO);
 
@@ -224,23 +195,12 @@ void SafeBrowsingServiceImpl::IOThreadEnabler::StartSafeBrowsingDBManager() {
                                              config);
 }
 
-void SafeBrowsingServiceImpl::IOThreadEnabler::SetUpURLRequestContext(
-    const base::FilePath& safe_browsing_data_path) {
+void SafeBrowsingServiceImpl::IOThreadEnabler::SetUpURLRequestContext() {
   DCHECK_CURRENTLY_ON(web::WebThread::IO);
 
+  // This uses an in-memory non-persistent cookie store. The Safe Browsing V4
+  // Update API does not depend on cookies.
   net::URLRequestContextBuilder builder;
-  base::FilePath cookie_file_path(safe_browsing_data_path.value() +
-                                  safe_browsing::kCookiesFile);
-  std::unique_ptr<net::CookieStore> cookie_store =
-      cookie_util::CreateCookieStore(
-          cookie_util::CookieStoreConfig(
-              cookie_file_path,
-              cookie_util::CookieStoreConfig::RESTORED_SESSION_COOKIES,
-              cookie_util::CookieStoreConfig::COOKIE_MONSTER,
-              /*crypto_delegate=*/nullptr),
-          /*system_cookie_store=*/nullptr, net::NetLog::Get());
-
-  builder.SetCookieStore(std::move(cookie_store));
   url_request_context_ = builder.Build();
 }
 

ios/chrome/browser/safe_browsing/safe_browsing_service_unittest.mm

@@ -8,7 +8,6 @@
 #include "base/path_service.h"
 #include "base/run_loop.h"
 #include "base/task/post_task.h"
-#include "base/test/bind_test_util.h"
 #include "base/test/scoped_feature_list.h"
 #include "components/prefs/pref_service.h"
 #include "components/safe_browsing/core/browser/safe_browsing_url_checker_impl.h"
@@ -38,11 +37,6 @@
 #include "ios/web/public/test/web_task_environment.h"
 #include "ios/web/public/thread/web_task_traits.h"
 #include "ios/web/public/thread/web_thread.h"
-#include "net/test/embedded_test_server/default_handlers.h"
-#include "net/test/embedded_test_server/embedded_test_server.h"
-#include "net/traffic_annotation/network_traffic_annotation_test_helper.h"
-#include "services/network/public/cpp/shared_url_loader_factory.h"
-#include "services/network/public/cpp/simple_url_loader.h"
 #include "testing/platform_test.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
@@ -201,7 +195,6 @@ class SafeBrowsingServiceTest : public PlatformTest {
   web::WebTaskEnvironment task_environment_;
   scoped_refptr<SafeBrowsingService> safe_browsing_service_;
   std::unique_ptr<TestChromeBrowserState> browser_state_;
-  base::ScopedTempDir temp_dir_;
 
  private:
   void MarkUrlAsMalwareOnIOThread(const GURL& bad_url) {
@@ -225,6 +218,8 @@ class SafeBrowsingServiceTest : public PlatformTest {
     v4_get_hash_factory_->AddToFullHashCache(full_hash_info);
   }
 
+  base::ScopedTempDir temp_dir_;
+
   // Owned by V4Database.
   safe_browsing::TestV4DatabaseFactory* v4_db_factory_;
   // Owned by V4GetHashProtocolManager.
@@ -306,119 +301,3 @@ TEST_F(SafeBrowsingServiceTest, RealTimeSafeAndUnsafePages) {
   EXPECT_FALSE(client.result_pending());
   EXPECT_FALSE(client.url_is_unsafe());
 }
-
-// Verifies that cookies are persisted across instantiations of
-// SafeBrowsingServiceImpl.
-TEST_F(SafeBrowsingServiceTest, PersistentCookies) {
-  net::EmbeddedTestServer server(net::EmbeddedTestServer::TYPE_HTTPS);
-  net::test_server::RegisterDefaultHandlers(&server);
-  ASSERT_TRUE(server.Start());
-  std::string cookie = "test=123";
-  std::unique_ptr<network::ResourceRequest> resource_request =
-      std::make_unique<network::ResourceRequest>();
-
-  // Set a cookie that expires in an hour.
-  resource_request->url = server.GetURL("/set-cookie?" + cookie +
-                                        ";max-age=3600;SameSite=None;Secure");
-  std::unique_ptr<network::SimpleURLLoader> url_loader =
-      network::SimpleURLLoader::Create(std::move(resource_request),
-                                       TRAFFIC_ANNOTATION_FOR_TESTS);
-  base::RunLoop run_loop1;
-  url_loader->DownloadHeadersOnly(
-      safe_browsing_service_->GetURLLoaderFactory().get(),
-      base::BindLambdaForTesting(
-          [&](scoped_refptr<net::HttpResponseHeaders> headers) {
-            run_loop1.Quit();
-          }));
-  run_loop1.Run();
-
-  // Destroy and re-create safe_browsing_service_, and verify that the cookie
-  // is still set.
-  safe_browsing_service_->ShutDown();
-  base::RunLoop().RunUntilIdle();
-  safe_browsing_service_ = base::MakeRefCounted<SafeBrowsingServiceImpl>();
-  safe_browsing_service_->Initialize(browser_state_->GetPrefs(),
-                                     temp_dir_.GetPath());
-  base::RunLoop().RunUntilIdle();
-
-  resource_request = std::make_unique<network::ResourceRequest>();
-  resource_request->url = server.GetURL("/echoheader?Cookie");
-  url_loader = network::SimpleURLLoader::Create(std::move(resource_request),
-                                                TRAFFIC_ANNOTATION_FOR_TESTS);
-  base::RunLoop run_loop2;
-  url_loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
-      safe_browsing_service_->GetURLLoaderFactory().get(),
-      base::BindLambdaForTesting([&](std::unique_ptr<std::string> body) {
-        EXPECT_NE(std::string::npos, body->find(cookie));
-        run_loop2.Quit();
-      }));
-  run_loop2.Run();
-}
-
-// Verifies that cookies are cleared when ClearCookies() is called with a
-// time range of all-time, but not otherwise.
-TEST_F(SafeBrowsingServiceTest, ClearCookies) {
-  net::EmbeddedTestServer server(net::EmbeddedTestServer::TYPE_HTTPS);
-  net::test_server::RegisterDefaultHandlers(&server);
-  ASSERT_TRUE(server.Start());
-  std::string cookie = "test=123";
-  std::unique_ptr<network::ResourceRequest> resource_request =
-      std::make_unique<network::ResourceRequest>();
-
-  // Set a cookie that expires in an hour.
-  resource_request->url = server.GetURL("/set-cookie?" + cookie +
-                                        ";max-age=3600;SameSite=None;Secure");
-  std::unique_ptr<network::SimpleURLLoader> url_loader =
-      network::SimpleURLLoader::Create(std::move(resource_request),
-                                       TRAFFIC_ANNOTATION_FOR_TESTS);
-  base::RunLoop run_loop1;
-  url_loader->DownloadHeadersOnly(
-      safe_browsing_service_->GetURLLoaderFactory().get(),
-      base::BindLambdaForTesting(
-          [&](scoped_refptr<net::HttpResponseHeaders> headers) {
-            run_loop1.Quit();
-          }));
-  run_loop1.Run();
-
-  // Call ClearCookies() with a non-all-time time range, and verify that the
-  // cookie is still set.
-  base::RunLoop run_loop2;
-  safe_browsing_service_->ClearCookies(
-      net::CookieDeletionInfo::TimeRange(base::Time(), base::Time::Now()),
-      base::BindLambdaForTesting([&]() { run_loop2.Quit(); }));
-  run_loop2.Run();
-
-  resource_request = std::make_unique<network::ResourceRequest>();
-  resource_request->url = server.GetURL("/echoheader?Cookie");
-  url_loader = network::SimpleURLLoader::Create(std::move(resource_request),
-                                                TRAFFIC_ANNOTATION_FOR_TESTS);
-  base::RunLoop run_loop3;
-  url_loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
-      safe_browsing_service_->GetURLLoaderFactory().get(),
-      base::BindLambdaForTesting([&](std::unique_ptr<std::string> body) {
-        EXPECT_NE(std::string::npos, body->find(cookie));
-        run_loop3.Quit();
-      }));
-  run_loop3.Run();
-
-  // Call ClearCookies() with a time range of all-time, and verify that the
-  // cookie is no longer set.
-  base::RunLoop run_loop4;
-  safe_browsing_service_->ClearCookies(
-      net::CookieDeletionInfo::TimeRange(base::Time(), base::Time::Max()),
-      base::BindLambdaForTesting([&]() { run_loop4.Quit(); }));
-  run_loop4.Run();
-
-  resource_request = std::make_unique<network::ResourceRequest>();
-  resource_request->url = server.GetURL("/echoheader?Cookie");
-  url_loader = network::SimpleURLLoader::Create(std::move(resource_request),
-                                                TRAFFIC_ANNOTATION_FOR_TESTS);
-  base::RunLoop run_loop5;
-  url_loader->DownloadToStringOfUnboundedSizeUntilCrashAndDie(
-      safe_browsing_service_->GetURLLoaderFactory().get(),
-      base::BindLambdaForTesting([&](std::unique_ptr<std::string> body) {
-        EXPECT_EQ(std::string::npos, body->find(cookie));
-        run_loop5.Quit();
-      }));
-  run_loop5.Run();
-}