Refactor CanonicalCookieFromSystemCookie to return a unique_ptr.

In http://crrev.com/c/2504089 I changed this function to use the new
CanonicalCookie factory method FromStorage() which returns a nullptr if
the resulting CanonicalCookie does not pass IsCanonical().

Since it turns out not every system cookie is guaranteed to be
canonical, which means dereferencing the pointer returned by FromStorage
sometimes resulted in a crash.

In order to fix this, this CL changes CanonicalCookieFromSystemCookie to
return the unique_ptr returned by FromStorage. It also updates the
callsites of CanonicalCookieFromSystemCookie to ignore the cookie in the
case that FromStorage returns a nullptr.

Bug: 1146386
Change-Id: Ic00a1e14af065b5b8b68e36d89ec043d5385f5b2
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2527729
Commit-Queue: Dylan Cutler <dylancutler@google.com>
Reviewed-by: Mark Cogan <marq@chromium.org>
Reviewed-by: Lily Chen <chlily@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826932}

ios/chrome/browser/signin/gaia_auth_fetcher_ios_ns_url_session_bridge.mm

@@ -185,14 +185,18 @@ void GaiaAuthFetcherIOSNSURLSessionBridge::SetCanonicalCookiesFromResponse(
   network::mojom::CookieManager* cookie_manager =
       browser_state_->GetCookieManager();
   for (NSHTTPCookie* cookie : cookies) {
+    std::unique_ptr<net::CanonicalCookie> canonical_cookie =
+        net::CanonicalCookieFromSystemCookie(cookie, base::Time::Now());
+    if (!canonical_cookie)
+      continue;
     net::CookieOptions options;
     options.set_include_httponly();
     // Permit it to set a SameSite cookie if it wants to.
     options.set_same_site_cookie_context(
         net::CookieOptions::SameSiteCookieContext::MakeInclusive());
-    cookie_manager->SetCanonicalCookie(
-        net::CanonicalCookieFromSystemCookie(cookie, base::Time::Now()),
-        net::GURLWithNSURL(response.URL), options, base::DoNothing());
+    cookie_manager->SetCanonicalCookie(*std::move(canonical_cookie),
+                                       net::GURLWithNSURL(response.URL),
+                                       options, base::DoNothing());
   }
 }
 

ios/chrome/browser/signin/gaia_auth_fetcher_ios_ns_url_session_bridge_unittests.mm

@@ -303,9 +303,12 @@ bool GaiaAuthFetcherIOSNSURLSessionBridgeTest::SetCookiesInCookieManager(
   network::mojom::CookieManager* cookie_manager =
       browser_state_->GetCookieManager();
   for (NSHTTPCookie* cookie in cookies) {
-    net::CanonicalCookie canonical_cookie =
+    std::unique_ptr<net::CanonicalCookie> canonical_cookie =
         net::CanonicalCookieFromSystemCookie(cookie, base::Time::Now());
-    if (!AddAllCookiesInCookieManager(cookie_manager, canonical_cookie))
+    if (!canonical_cookie)
+      continue;
+    if (!AddAllCookiesInCookieManager(cookie_manager,
+                                      *std::move(canonical_cookie)))
       return false;
   }
   return true;

ios/net/cookies/cookie_store_ios.mm

@@ -448,8 +448,11 @@ void CookieStoreIOS::WriteToCookieMonster(NSArray* system_cookies) {
   NSUInteger cookie_count = [system_cookies count];
   cookie_list.reserve(cookie_count);
   for (NSHTTPCookie* cookie in system_cookies) {
-    cookie_list.push_back(CanonicalCookieFromSystemCookie(
-        cookie, system_store_->GetCookieCreationTime(cookie)));
+    if (std::unique_ptr<net::CanonicalCookie> canonical_cookie =
+            CanonicalCookieFromSystemCookie(
+                cookie, system_store_->GetCookieCreationTime(cookie))) {
+      cookie_list.push_back(*std::move(canonical_cookie));
+    }
   }
   cookie_monster_->SetAllCookiesAsync(cookie_list, SetCookiesCallback());
 
@@ -491,9 +494,11 @@ void CookieStoreIOS::DeleteCookiesMatchingPredicateAsync(
         for (NSHTTPCookie* cookie in cookies) {
           base::Time creation_time =
               weak_system_store->GetCookieCreationTime(cookie);
-          CanonicalCookie cc =
+          std::unique_ptr<net::CanonicalCookie> canonical_cookie =
               CanonicalCookieFromSystemCookie(cookie, creation_time);
-          if (shared_predicate.Run(cc)) {
+          if (!canonical_cookie)
+            continue;
+          if (shared_predicate.Run(*std::move(canonical_cookie))) {
             weak_system_store->DeleteCookieAsync(
                 cookie, SystemCookieStore::SystemCookieCallback());
             to_delete_count++;
@@ -577,9 +582,11 @@ void CookieStoreIOS::UpdateCacheForCookies(const GURL& gurl,
   std::vector<net::CanonicalCookie> out_added_cookies;
   for (NSHTTPCookie* nscookie in nscookies) {
     if (base::SysNSStringToUTF8(nscookie.name) == cookie_name) {
-      net::CanonicalCookie canonical_cookie = CanonicalCookieFromSystemCookie(
-          nscookie, system_store_->GetCookieCreationTime(nscookie));
-      cookies.push_back(canonical_cookie);
+      if (std::unique_ptr<net::CanonicalCookie> canonical_cookie =
+              CanonicalCookieFromSystemCookie(
+                  nscookie, system_store_->GetCookieCreationTime(nscookie))) {
+        cookies.push_back(*std::move(canonical_cookie));
+      }
     }
   }
 
@@ -673,7 +680,10 @@ CookieStoreIOS::CanonicalCookieListFromSystemCookies(NSArray* cookies) {
   cookie_list.reserve([cookies count]);
   for (NSHTTPCookie* cookie in cookies) {
     base::Time created = system_store_->GetCookieCreationTime(cookie);
-    cookie_list.push_back(CanonicalCookieFromSystemCookie(cookie, created));
+    if (std::unique_ptr<net::CanonicalCookie> canonical_cookie =
+            CanonicalCookieFromSystemCookie(cookie, created)) {
+      cookie_list.push_back(*std::move(canonical_cookie));
+    }
   }
   return cookie_list;
 }
@@ -685,8 +695,11 @@ CookieStoreIOS::CanonicalCookieWithAccessResultListFromSystemCookies(
   cookie_list.reserve([cookies count]);
   for (NSHTTPCookie* cookie in cookies) {
     base::Time created = system_store_->GetCookieCreationTime(cookie);
-    cookie_list.push_back({CanonicalCookieFromSystemCookie(cookie, created),
-                           net::CookieAccessResult()});
+    if (std::unique_ptr<net::CanonicalCookie> canonical_cookie =
+            CanonicalCookieFromSystemCookie(cookie, created)) {
+      cookie_list.push_back(
+          {*std::move(canonical_cookie), net::CookieAccessResult()});
+    }
   }
   return cookie_list;
 }

ios/net/cookies/system_cookie_util.h

@@ -19,7 +19,7 @@ class Time;
 namespace net {
 
 // Converts NSHTTPCookie to net::CanonicalCookie.
-net::CanonicalCookie CanonicalCookieFromSystemCookie(
+std::unique_ptr<net::CanonicalCookie> CanonicalCookieFromSystemCookie(
     NSHTTPCookie* cookie,
     const base::Time& ceation_time);
 

ios/net/cookies/system_cookie_util.mm

@@ -72,7 +72,7 @@ void ReportUMACookieLoss(CookieLossType loss, CookieEvent event) {
 }  // namespace
 
 // Converts NSHTTPCookie to net::CanonicalCookie.
-net::CanonicalCookie CanonicalCookieFromSystemCookie(
+std::unique_ptr<net::CanonicalCookie> CanonicalCookieFromSystemCookie(
     NSHTTPCookie* cookie,
     const base::Time& ceation_time) {
   net::CookieSameSite same_site = net::CookieSameSite::NO_RESTRICTION;
@@ -89,7 +89,7 @@ net::CanonicalCookie CanonicalCookieFromSystemCookie(
       same_site = net::CookieSameSite::NO_RESTRICTION;
   }
 
-  return *std::move(net::CanonicalCookie::FromStorage(
+  return net::CanonicalCookie::FromStorage(
       base::SysNSStringToUTF8([cookie name]),
       base::SysNSStringToUTF8([cookie value]),
       base::SysNSStringToUTF8([cookie domain]),
@@ -99,7 +99,7 @@ net::CanonicalCookie CanonicalCookieFromSystemCookie(
       // When iOS begins to support 'Priority' and 'SameParty' attributes, pass
       // them through here.
       net::COOKIE_PRIORITY_DEFAULT, false /* SameParty */,
-      net::CookieSourceScheme::kUnset));
+      net::CookieSourceScheme::kUnset);
 }
 
 void ReportGetCookiesForURLResult(SystemCookieStoreType store_type,

ios/net/cookies/system_cookie_util_unittest.mm

@@ -106,25 +106,25 @@ TEST_F(CookieUtil, CanonicalCookieFromSystemCookie) {
       [[NSHTTPCookie alloc] initWithProperties:properties];
 
   ASSERT_TRUE(system_cookie);
-  net::CanonicalCookie chrome_cookie =
+  std::unique_ptr<net::CanonicalCookie> chrome_cookie =
       CanonicalCookieFromSystemCookie(system_cookie, creation_time);
-  EXPECT_EQ("a", chrome_cookie.Name());
-  EXPECT_EQ("b", chrome_cookie.Value());
-  EXPECT_EQ("foo", chrome_cookie.Domain());
-  EXPECT_EQ("/", chrome_cookie.Path());
-  EXPECT_EQ(creation_time, chrome_cookie.CreationDate());
-  EXPECT_TRUE(chrome_cookie.LastAccessDate().is_null());
-  EXPECT_TRUE(chrome_cookie.IsPersistent());
+  EXPECT_EQ("a", chrome_cookie->Name());
+  EXPECT_EQ("b", chrome_cookie->Value());
+  EXPECT_EQ("foo", chrome_cookie->Domain());
+  EXPECT_EQ("/", chrome_cookie->Path());
+  EXPECT_EQ(creation_time, chrome_cookie->CreationDate());
+  EXPECT_TRUE(chrome_cookie->LastAccessDate().is_null());
+  EXPECT_TRUE(chrome_cookie->IsPersistent());
   // Allow 1 second difference as iOS rounds expiry time to the nearest second.
   EXPECT_LE(expire_date - base::TimeDelta::FromSeconds(1),
-            chrome_cookie.ExpiryDate());
+            chrome_cookie->ExpiryDate());
   EXPECT_GE(expire_date + base::TimeDelta::FromSeconds(1),
-            chrome_cookie.ExpiryDate());
-  EXPECT_FALSE(chrome_cookie.IsSecure());
-  EXPECT_TRUE(chrome_cookie.IsHttpOnly());
-  EXPECT_EQ(net::COOKIE_PRIORITY_DEFAULT, chrome_cookie.Priority());
+            chrome_cookie->ExpiryDate());
+  EXPECT_FALSE(chrome_cookie->IsSecure());
+  EXPECT_TRUE(chrome_cookie->IsHttpOnly());
+  EXPECT_EQ(net::COOKIE_PRIORITY_DEFAULT, chrome_cookie->Priority());
   if (@available(iOS 13, *)) {
-    EXPECT_EQ(net::CookieSameSite::STRICT_MODE, chrome_cookie.SameSite());
+    EXPECT_EQ(net::CookieSameSite::STRICT_MODE, chrome_cookie->SameSite());
   }
 
   // Test session and secure cookie.
@@ -137,8 +137,8 @@ TEST_F(CookieUtil, CanonicalCookieFromSystemCookie) {
   }];
   ASSERT_TRUE(system_cookie);
   chrome_cookie = CanonicalCookieFromSystemCookie(system_cookie, creation_time);
-  EXPECT_FALSE(chrome_cookie.IsPersistent());
-  EXPECT_TRUE(chrome_cookie.IsSecure());
+  EXPECT_FALSE(chrome_cookie->IsPersistent());
+  EXPECT_TRUE(chrome_cookie->IsSecure());
 }
 
 // Tests that histogram is reported correctly based on the input.

ios/web/download/download_session_cookie_storage.mm

@@ -52,10 +52,11 @@
     cookieAccessSemantics = net::CookieAccessSemantics::UNKNOWN;
   }
   for (NSHTTPCookie* cookie in self.cookies) {
-    net::CanonicalCookie canonical_cookie =
+    std::unique_ptr<net::CanonicalCookie> canonical_cookie =
         net::CanonicalCookieFromSystemCookie(cookie, base::Time());
-    if (canonical_cookie
-            .IncludeForRequestURL(gURL, options, cookieAccessSemantics)
+    if (canonical_cookie &&
+        canonical_cookie
+            ->IncludeForRequestURL(gURL, options, cookieAccessSemantics)
             .status.IsInclude())
       [result addObject:cookie];
   }

ios/web/net/cookies/wk_http_system_cookie_store.mm

@@ -41,8 +41,10 @@ bool ShouldIncludeForRequestUrl(NSHTTPCookie* cookie, const GURL& url) {
   // of rewriting the checks here, the function converts the NSHTTPCookie to
   // canonical cookie and provide it with dummy CookieOption, so when iOS starts
   // to support cookieOptions this function can be modified to support that.
-  net::CanonicalCookie canonical_cookie =
+  std::unique_ptr<net::CanonicalCookie> canonical_cookie =
       net::CanonicalCookieFromSystemCookie(cookie, base::Time());
+  if (!canonical_cookie)
+    return false;
   // Cookies handled by this method are app specific cookies, so it's safe to
   // use strict same site context.
   net::CookieOptions options = net::CookieOptions::MakeAllInclusive();
@@ -55,7 +57,7 @@ bool ShouldIncludeForRequestUrl(NSHTTPCookie* cookie, const GURL& url) {
     cookie_access_semantics = net::CookieAccessSemantics::UNKNOWN;
   }
   return canonical_cookie
-      .IncludeForRequestURL(url, options, cookie_access_semantics)
+      ->IncludeForRequestURL(url, options, cookie_access_semantics)
       .status.IsInclude();
 }