Fix a bug that download doesn't start sometimes.

This change involves:

- Use net::CookieStoreIOS instead of using WKHTTPCookieStore directly.
  -[WKHTTPCookieStore getAllCookies] has a bug that it sometimes doesn't
  call its completion handler immediately
  (https://openradar.appspot.com/39470983). net::CookieStoreIOS
  implements a workaround of the bug.

Bug: 938123
Change-Id: Ica8f3855d3f7355a3903f56e7744abd9f5edb024
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1511094
Commit-Queue: Hiroshi Ichikawa <ichikawa@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Reviewed-by: Mohammad Refaat <mrefaat@chromium.org>
Cr-Commit-Position: refs/heads/master@{#701479}

ios/net/cookies/system_cookie_util.h

@@ -5,6 +5,7 @@
 #ifndef IOS_NET_COOKIES_SYSTEM_COOKIE_UTIL_H_
 #define IOS_NET_COOKIES_SYSTEM_COOKIE_UTIL_H_
 
+#import <Foundation/Foundation.h>
 #include <stddef.h>
 
 #include "net/cookies/canonical_cookie.h"
@@ -26,6 +27,10 @@ net::CanonicalCookie CanonicalCookieFromSystemCookie(
 NSHTTPCookie* SystemCookieFromCanonicalCookie(
     const net::CanonicalCookie& cookie);
 
+// Converts net::CookieList to NSArray<NSHTTPCookie*>.
+NSArray<NSHTTPCookie*>* SystemCookiesFromCanonicalCookieList(
+    const net::CookieList& cookie_list);
+
 enum CookieEvent {
   COOKIES_READ,                     // Cookies have been read from disk.
   COOKIES_APPLICATION_FOREGROUNDED  // The application has been foregrounded.

ios/net/cookies/system_cookie_util.mm

@@ -149,6 +149,15 @@ NSHTTPCookie* SystemCookieFromCanonicalCookie(
   return system_cookie;
 }
 
+NSArray<NSHTTPCookie*>* SystemCookiesFromCanonicalCookieList(
+    const net::CookieList& cookie_list) {
+  NSMutableArray<NSHTTPCookie*>* cookies = [[NSMutableArray alloc] init];
+  for (const net::CanonicalCookie& cookie : cookie_list) {
+    [cookies addObject:net::SystemCookieFromCanonicalCookie(cookie)];
+  }
+  return [cookies copy];
+}
+
 void CheckForCookieLoss(size_t cookie_count, CookieEvent event) {
   NSUserDefaults* defaults = [NSUserDefaults standardUserDefaults];
   BOOL check_cookie_loss = [defaults boolForKey:kCheckCookieLossKey];

ios/net/cookies/system_cookie_util_unittest.mm

@@ -178,4 +178,39 @@ TEST_F(CookieUtil, SystemCookieFromBadCanonicalCookie) {
   EXPECT_TRUE(system_cookie == nil);
 }
 
+TEST_F(CookieUtil, SystemCookiesFromCanonicalCookieList) {
+  base::Time expire_date = base::Time::Now() + base::TimeDelta::FromHours(2);
+  net::CookieList cookie_list = {
+      net::CanonicalCookie("name1", "value1", "domain1", "path1/",
+                           base::Time(),  // creation
+                           expire_date,
+                           base::Time(),  // last_access
+                           false,         // secure
+                           false,         // httponly
+                           net::CookieSameSite::UNSPECIFIED,
+                           net::COOKIE_PRIORITY_DEFAULT),
+      net::CanonicalCookie("name2", "value2", "domain2", "path2/",
+                           base::Time(),  // creation
+                           expire_date,
+                           base::Time(),  // last_access
+                           false,         // secure
+                           false,         // httponly
+                           net::CookieSameSite::UNSPECIFIED,
+                           net::COOKIE_PRIORITY_DEFAULT),
+  };
+
+  NSArray<NSHTTPCookie*>* system_cookies =
+      SystemCookiesFromCanonicalCookieList(cookie_list);
+
+  ASSERT_EQ(2UL, system_cookies.count);
+  EXPECT_NSEQ(@"name1", system_cookies[0].name);
+  EXPECT_NSEQ(@"value1", system_cookies[0].value);
+  EXPECT_NSEQ(@"domain1", system_cookies[0].domain);
+  EXPECT_NSEQ(@"path1/", system_cookies[0].path);
+  EXPECT_NSEQ(@"name2", system_cookies[1].name);
+  EXPECT_NSEQ(@"value2", system_cookies[1].value);
+  EXPECT_NSEQ(@"domain2", system_cookies[1].domain);
+  EXPECT_NSEQ(@"path2/", system_cookies[1].path);
+}
+
 }  // namespace net

ios/web/download/BUILD.gn

@@ -8,6 +8,7 @@ source_set("download") {
   deps = [
     ":download_cookies",
     "//base",
+    "//ios/net",
     "//ios/web/net/cookies",
     "//ios/web/public",
     "//ios/web/public/download",

ios/web/download/download_task_impl.h

@@ -16,6 +16,7 @@
 
 namespace net {
 class URLFetcherResponseWriter;
+class URLRequestContextGetter;
 }
 
 namespace web {
@@ -88,6 +89,12 @@ class DownloadTaskImpl : public DownloadTask {
   // Must be called on UI thread. The callback will be invoked on the UI thread.
   void GetCookies(base::Callback<void(NSArray<NSHTTPCookie*>*)> callback);
 
+  // Asynchronously returns cookies for |context_getter|. Must
+  // be called on IO thread. The callback will be invoked on the UI thread.
+  static void GetCookiesFromContextGetter(
+      scoped_refptr<net::URLRequestContextGetter> context_getter,
+      base::Callback<void(NSArray<NSHTTPCookie*>*)> callback);
+
   // Starts the download with given cookies.
   void StartWithCookies(NSArray<NSHTTPCookie*>* cookies);
 

ios/web/download/download_task_impl.mm

@@ -10,6 +10,7 @@
 #include "base/bind.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/task/post_task.h"
+#import "ios/net/cookies/system_cookie_util.h"
 #import "ios/web/net/cookies/wk_cookie_util.h"
 #include "ios/web/public/browser_state.h"
 #import "ios/web/public/download/download_task_observer.h"
@@ -23,7 +24,10 @@
 #include "net/base/io_buffer.h"
 #import "net/base/mac/url_conversions.h"
 #include "net/base/net_errors.h"
+#include "net/cookies/cookie_store.h"
 #include "net/url_request/url_fetcher_response_writer.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_getter.h"
 #include "url/url_constants.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
@@ -402,14 +406,30 @@ NSURLSession* DownloadTaskImpl::CreateSession(NSString* identifier,
 
 void DownloadTaskImpl::GetCookies(
     base::Callback<void(NSArray<NSHTTPCookie*>*)> callback) {
-  DCHECK_CURRENTLY_ON(web::WebThread::UI);
-  auto store = WKCookieStoreForBrowserState(web_state_->GetBrowserState());
-  DCHECK(store);
-  [store getAllCookies:^(NSArray<NSHTTPCookie*>* cookies) {
-    // getAllCookies: callback is always called on UI thread.
-    DCHECK_CURRENTLY_ON(WebThread::UI);
-    callback.Run(cookies);
-  }];
+  DCHECK_CURRENTLY_ON(WebThread::UI);
+  scoped_refptr<net::URLRequestContextGetter> context_getter(
+      web_state_->GetBrowserState()->GetRequestContext());
+
+  // net::URLRequestContextGetter must be used in the IO thread.
+  base::PostTask(FROM_HERE, {WebThread::IO},
+                 base::BindOnce(&DownloadTaskImpl::GetCookiesFromContextGetter,
+                                context_getter, callback));
+}
+
+void DownloadTaskImpl::GetCookiesFromContextGetter(
+    scoped_refptr<net::URLRequestContextGetter> context_getter,
+    base::Callback<void(NSArray<NSHTTPCookie*>*)> callback) {
+  DCHECK_CURRENTLY_ON(WebThread::IO);
+  context_getter->GetURLRequestContext()->cookie_store()->GetAllCookiesAsync(
+      base::BindOnce(
+          [](base::Callback<void(NSArray<NSHTTPCookie*>*)> callback,
+             const net::CookieList& cookie_list) {
+            NSArray<NSHTTPCookie*>* cookies =
+                SystemCookiesFromCanonicalCookieList(cookie_list);
+            base::PostTask(FROM_HERE, {WebThread::UI},
+                           base::BindOnce(callback, cookies));
+          },
+          callback));
 }
 
 void DownloadTaskImpl::StartWithCookies(NSArray<NSHTTPCookie*>* cookies) {

ios/web/download/download_task_impl_unittest.mm

@@ -18,11 +18,14 @@
 #import "base/test/ios/wait_util.h"
 #import "ios/web/net/cookies/wk_cookie_util.h"
 #import "ios/web/public/download/download_task_observer.h"
+#include "ios/web/public/test/fakes/fake_cookie_store.h"
 #import "ios/web/public/test/fakes/test_web_state.h"
 #include "ios/web/public/test/web_test.h"
 #import "ios/web/test/fakes/crw_fake_nsurl_session_task.h"
 #include "net/base/net_errors.h"
 #include "net/url_request/url_fetcher_response_writer.h"
+#include "net/url_request/url_request_context.h"
+#include "net/url_request/url_request_context_getter.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/gtest_mac.h"
@@ -134,6 +137,9 @@ class DownloadTaskImplTest : public PlatformTest {
         session_delegate_callbacks_queue_(
             dispatch_queue_create(nullptr, DISPATCH_QUEUE_SERIAL)) {
     browser_state_.SetOffTheRecord(true);
+    browser_state_.GetRequestContext()
+        ->GetURLRequestContext()
+        ->set_cookie_store(&cookie_store_);
     web_state_.SetBrowserState(&browser_state_);
     task_->AddObserver(&task_observer_);
   }
@@ -164,20 +170,6 @@ class DownloadTaskImplTest : public PlatformTest {
     return Start(std::make_unique<net::URLFetcherStringWriter>());
   }
 
-  // Sets cookie for the test browser state.
-  bool SetCookie(NSHTTPCookie* cookie) WARN_UNUSED_RESULT
-      API_AVAILABLE(ios(11.0)) {
-    auto store = web::WKCookieStoreForBrowserState(&browser_state_);
-    __block bool cookie_was_set = false;
-    [store setCookie:cookie
-        completionHandler:^{
-          cookie_was_set = true;
-        }];
-    return WaitUntilConditionOrTimeout(kWaitForCookiesTimeout, ^{
-      return cookie_was_set;
-    });
-  }
-
   // Session and session delegate injected into DownloadTaskImpl for testing.
   NSURLSession* session() { return task_delegate_.session(); }
   id<NSURLSessionDataDelegate> session_delegate() {
@@ -222,6 +214,7 @@ class DownloadTaskImplTest : public PlatformTest {
   web::WebTaskEnvironment task_environment_;
   TestBrowserState browser_state_;
   TestWebState web_state_;
+  FakeCookieStore cookie_store_;
   testing::StrictMock<FakeDownloadTaskImplDelegate> task_delegate_;
   std::unique_ptr<DownloadTaskImpl> task_;
   MockDownloadTaskObserver task_observer_;
@@ -544,23 +537,28 @@ TEST_F(DownloadTaskImplTest, FailureInTheMiddle) {
 // Tests that CreateSession is called with the correct cookies from the cookie
 // store.
 TEST_F(DownloadTaskImplTest, Cookie) {
-  // Add a cookie to BrowserState.
-  NSURL* cookie_url = [NSURL URLWithString:@(kUrl)];
-  NSHTTPCookie* cookie = [NSHTTPCookie cookieWithProperties:@{
-    NSHTTPCookieName : @"name",
-    NSHTTPCookieValue : @"value",
-    NSHTTPCookiePath : cookie_url.path,
-    NSHTTPCookieDomain : cookie_url.host,
-    NSHTTPCookieVersion : @1,
-  }];
-  ASSERT_TRUE(SetCookie(cookie));
+  GURL cookie_url(kUrl);
+  base::Time now = base::Time::Now();
+  net::CanonicalCookie expected_cookie(
+      "name", "value", cookie_url.host(), cookie_url.path(),
+      /*creation=*/now,
+      /*expire_date=*/now + base::TimeDelta::FromHours(2),
+      /*last_access=*/now,
+      /*secure=*/false,
+      /*httponly=*/false, net::CookieSameSite::UNSPECIFIED,
+      net::COOKIE_PRIORITY_DEFAULT);
+  cookie_store_.SetAllCookies({expected_cookie});
 
   // Start the download and make sure that all cookie from BrowserState were
   // picked up.
   EXPECT_CALL(task_observer_, OnDownloadUpdated(task_.get()));
   ASSERT_TRUE(Start());
+
   EXPECT_EQ(1U, task_delegate_.cookies().count);
-  EXPECT_NSEQ(cookie, task_delegate_.cookies().firstObject);
+  NSHTTPCookie* actual_cookie = task_delegate_.cookies().firstObject;
+  EXPECT_NSEQ(@"name", actual_cookie.name);
+  EXPECT_NSEQ(@"value", actual_cookie.value);
+
   EXPECT_CALL(task_delegate_, OnTaskDestroyed(task_.get()));
 }
 

ios/web/public/test/fakes/BUILD.gn

@@ -22,6 +22,7 @@ source_set("fakes") {
     "//ios/web/test:test_constants",
     "//ios/web/web_state/ui:crw_web_view_navigation_proxy",
     "//ios/web/webui:webui",
+    "//net",
     "//net:test_support",
     "//testing/gtest",
     "//ui/base",
@@ -38,6 +39,8 @@ source_set("fakes") {
     "crw_fake_web_state_policy_decider.mm",
     "crw_test_web_state_observer.h",
     "crw_test_web_state_observer.mm",
+    "fake_cookie_store.cc",
+    "fake_cookie_store.h",
     "fake_download_controller_delegate.h",
     "fake_download_controller_delegate.mm",
     "fake_download_task.h",

ios/web/public/test/fakes/fake_cookie_store.cc

+// Copyright 2019 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 "ios/web/public/test/fakes/fake_cookie_store.h"
+
+#include "base/task/post_task.h"
+#include "ios/web/public/thread/web_task_traits.h"
+#include "ios/web/public/thread/web_thread.h"
+
+namespace web {
+
+FakeCookieStore::FakeCookieStore() {}
+
+FakeCookieStore::~FakeCookieStore() {}
+
+void FakeCookieStore::SetAllCookies(const net::CookieList& all_cookies) {
+  all_cookies_ = all_cookies;
+}
+
+void FakeCookieStore::GetAllCookiesAsync(GetAllCookiesCallback callback) {
+  DCHECK_CURRENTLY_ON(WebThread::IO);
+  base::PostTask(FROM_HERE, {WebThread::IO},
+                 base::BindOnce(std::move(callback), all_cookies_));
+}
+
+void FakeCookieStore::SetCanonicalCookieAsync(
+    std::unique_ptr<net::CanonicalCookie> cookie,
+    std::string source_scheme,
+    const net::CookieOptions& options,
+    SetCookiesCallback callback) {
+  NOTIMPLEMENTED() << "Implement this if necessary.";
+}
+
+void FakeCookieStore::GetCookieListWithOptionsAsync(
+    const GURL& url,
+    const net::CookieOptions& options,
+    GetCookieListCallback callback) {
+  NOTIMPLEMENTED() << "Implement this if necessary.";
+}
+
+void FakeCookieStore::DeleteCanonicalCookieAsync(
+    const net::CanonicalCookie& cookie,
+    DeleteCallback callback) {
+  NOTIMPLEMENTED() << "Implement this if necessary.";
+}
+
+void FakeCookieStore::DeleteAllCreatedInTimeRangeAsync(
+    const net::CookieDeletionInfo::TimeRange& creation_range,
+    DeleteCallback callback) {
+  NOTIMPLEMENTED() << "Implement this if necessary.";
+}
+
+void FakeCookieStore::DeleteAllMatchingInfoAsync(
+    net::CookieDeletionInfo delete_info,
+    DeleteCallback callback) {
+  NOTIMPLEMENTED() << "Implement this if necessary.";
+}
+
+void FakeCookieStore::DeleteSessionCookiesAsync(DeleteCallback) {
+  NOTIMPLEMENTED() << "Implement this if necessary.";
+}
+
+void FakeCookieStore::FlushStore(base::OnceClosure callback) {
+  NOTIMPLEMENTED() << "Implement this if necessary.";
+}
+
+void FakeCookieStore::SetCookieableSchemes(
+    const std::vector<std::string>& schemes,
+    SetCookieableSchemesCallback callback) {
+  NOTIMPLEMENTED() << "Implement this if necessary.";
+}
+
+net::CookieChangeDispatcher& FakeCookieStore::GetChangeDispatcher() {
+  // This is NOTREACHED not NOTIMPLEMENTED because it would likely cause a weird
+  // SEGV in the next line anyways. Crashing here with a more friendly error
+  // message is preferred.
+  NOTREACHED() << "Not implemented. Implement this if necessary.";
+  // Perform a crazy thing here just to make the compiler happy. It doesn't
+  // matter because it should never reach here.
+  return *reinterpret_cast<net::CookieChangeDispatcher*>(this);
+}
+
+}  // namespace web

ios/web/public/test/fakes/fake_cookie_store.h

+// Copyright 2019 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 IOS_WEB_PUBLIC_TEST_FAKES_FAKE_COOKIE_STORE_H_
+#define IOS_WEB_PUBLIC_TEST_FAKES_FAKE_COOKIE_STORE_H_
+
+#include "net/cookies/cookie_store.h"
+
+namespace web {
+
+// Fake implementation for net::CookieStore interface. Can be used for testing.
+class FakeCookieStore : public net::CookieStore {
+ public:
+  FakeCookieStore();
+  ~FakeCookieStore() override;
+
+  /// Sets cookies returned by GetAllCookiesAsync().
+  void SetAllCookies(const net::CookieList& all_cookies);
+
+  void GetAllCookiesAsync(GetAllCookiesCallback callback) override;
+
+  // Methods below have not been implemented in this fake. Implement them when
+  // necessary.
+
+  void SetCanonicalCookieAsync(std::unique_ptr<net::CanonicalCookie> cookie,
+                               std::string source_scheme,
+                               const net::CookieOptions& options,
+                               SetCookiesCallback callback) override;
+  void GetCookieListWithOptionsAsync(const GURL& url,
+                                     const net::CookieOptions& options,
+                                     GetCookieListCallback callback) override;
+  void DeleteCanonicalCookieAsync(const net::CanonicalCookie& cookie,
+                                  DeleteCallback callback) override;
+  void DeleteAllCreatedInTimeRangeAsync(
+      const net::CookieDeletionInfo::TimeRange& creation_range,
+      DeleteCallback callback) override;
+  void DeleteAllMatchingInfoAsync(net::CookieDeletionInfo delete_info,
+                                  DeleteCallback callback) override;
+  void DeleteSessionCookiesAsync(DeleteCallback) override;
+  void FlushStore(base::OnceClosure callback) override;
+  void SetCookieableSchemes(const std::vector<std::string>& schemes,
+                            SetCookieableSchemesCallback callback) override;
+  net::CookieChangeDispatcher& GetChangeDispatcher() override;
+
+ private:
+  net::CookieList all_cookies_;
+  net::CookieStatusList excluded_list_;
+};
+
+}  // namespace web
+
+#endif  // IOS_WEB_PUBLIC_TEST_FAKES_FAKE_COOKIE_STORE_H_