Refactor transmission of security of source and http_only mods in cookie_monster.cc.

net/cookies/cookie_monster.cc

Index: net/cookies/cookie_monster.cc
diff --git a/net/cookies/cookie_monster.cc b/net/cookies/cookie_monster.cc
index 94d76d59f5a494477e2b5b688b2918c6f35f321f..ed6c9fd8dacc80e3c1662fa9adc1adae07c190a1 100644
--- a/net/cookies/cookie_monster.cc
+++ b/net/cookies/cookie_monster.cc
@@ -767,7 +767,7 @@ void CookieMonster::SetAllCookiesTask::Run() {
 
   bool result = true;
   if (positive_diff.size() > 0)
-    result = this->cookie_monster()->SetCanonicalCookies(list_);
+    result = this->cookie_monster()->SetAllCookies(list_);
 
   if (!callback_.is_null())
     callback_.Run(result);
@@ -1114,11 +1114,7 @@ bool CookieMonster::SetCookieWithDetails(const GURL& url,
       expiration_time, last_access_time, secure, http_only, same_site,
       priority));
 
-  CookieOptions options;
-  options.set_include_httponly();
-  options.set_same_site_cookie_mode(
-      CookieOptions::SameSiteCookieMode::INCLUDE_STRICT_AND_LAX);
-  return SetCanonicalCookie(std::move(cc), url, options);
+  return SetCanonicalCookie(std::move(cc), url.SchemeIsCryptographic(), true);
 }
 
 CookieList CookieMonster::GetAllCookies() {
@@ -1437,7 +1433,7 @@ void CookieMonster::StoreLoadedCookies(
     if (creation_times_.insert(cookie_creation_time).second) {
       CanonicalCookie* cookie_ptr = cookie.get();
       CookieMap::iterator inserted = InternalInsertCookie(
-          GetKey(cookie_ptr->Domain()), std::move(cookie), GURL(), false);
+          GetKey(cookie_ptr->Domain()), std::move(cookie), false, false);

[mmenke, 2017/06/09 18:48:52]

These used to get histogrammed, didn't they?  Seems like a significant change to
the histogram.

[Randy Smith (Not in Mondays), 2017/06/13 20:53:40]

As I read the code, the null URL resulted in them not getting histogrammed. 
What are you seeing that I'm not?

[mmenke, 2017/06/16 16:19:48]

You're right, I missed that...And it seems like a really weird design decision. 
Too much magic.  :(

[Randy Smith (Not in Mondays), 2017/06/16 20:53:58]

I suspect it wasn't a decision :-{.  But I'm trying to make this a targeted
refactor rather than a cleanup.

       const Time cookie_access_time(cookie_ptr->LastAccessDate());
       if (earliest_access_time_.is_null() ||
           cookie_access_time < earliest_access_time_)
@@ -1651,7 +1647,7 @@ void CookieMonster::FindCookiesForKey(const std::string& key,
 
 bool CookieMonster::DeleteAnyEquivalentCookie(const std::string& key,
                                               const CanonicalCookie& ecc,
-                                              const GURL& source_url,
+                                              bool source_secure,
                                               bool skip_httponly,
                                               bool already_expired) {
   DCHECK(thread_checker_.CalledOnValidThread());
@@ -1674,7 +1670,7 @@ bool CookieMonster::DeleteAnyEquivalentCookie(const std::string& key,
     // attribute.
     //
     // See: https://tools.ietf.org/html/draft-ietf-httpbis-cookie-alone
-    if (cc->IsSecure() && !source_url.SchemeIsCryptographic() &&
+    if (cc->IsSecure() && !source_secure &&
         ecc.IsEquivalentForSecureCookieMatching(*cc)) {
       skipped_secure_cookie = true;
       histogram_cookie_delete_equivalent_->Add(
@@ -1715,7 +1711,7 @@ bool CookieMonster::DeleteAnyEquivalentCookie(const std::string& key,
 CookieMonster::CookieMap::iterator CookieMonster::InternalInsertCookie(
     const std::string& key,
     std::unique_ptr<CanonicalCookie> cc,
-    const GURL& source_url,
+    bool source_secure,
     bool sync_to_store) {
   DCHECK(thread_checker_.CalledOnValidThread());
   CanonicalCookie* cc_ptr = cc.get();
@@ -1738,27 +1734,6 @@ CookieMonster::CookieMap::iterator CookieMonster::InternalInsertCookie(
   type_sample |= cc_ptr->IsSecure() ? 1 << COOKIE_TYPE_SECURE : 0;
   histogram_cookie_type_->Add(type_sample);
 
-  // Histogram the type of scheme used on URLs that set cookies. This
-  // intentionally includes cookies that are set or overwritten by
-  // http:// URLs, but not cookies that are cleared by http:// URLs, to
-  // understand if the former behavior can be deprecated for Secure
-  // cookies.
-  if (!source_url.is_empty()) {
-    CookieSource cookie_source_sample;
-    if (source_url.SchemeIsCryptographic()) {
-      cookie_source_sample =
-          cc_ptr->IsSecure()
-              ? COOKIE_SOURCE_SECURE_COOKIE_CRYPTOGRAPHIC_SCHEME
-              : COOKIE_SOURCE_NONSECURE_COOKIE_CRYPTOGRAPHIC_SCHEME;
-    } else {
-      cookie_source_sample =
-          cc_ptr->IsSecure()
-              ? COOKIE_SOURCE_SECURE_COOKIE_NONCRYPTOGRAPHIC_SCHEME
-              : COOKIE_SOURCE_NONSECURE_COOKIE_NONCRYPTOGRAPHIC_SCHEME;
-    }
-    histogram_cookie_source_scheme_->Add(cookie_source_sample);
-  }
-
   RunCookieChangedCallbacks(*cc_ptr, CookieStore::ChangeCause::INSERTED);
 
   return inserted;
@@ -1786,20 +1761,21 @@ bool CookieMonster::SetCookieWithCreationTimeAndOptions(
     VLOG(kVlogSetCookies) << "WARNING: Failed to allocate CanonicalCookie";
     return false;
   }
-  return SetCanonicalCookie(std::move(cc), url, options);
+  return SetCanonicalCookie(std::move(cc), url.SchemeIsCryptographic(),
+                            !options.exclude_httponly());
 }
 
 bool CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
-                                       const GURL& source_url,
-                                       const CookieOptions& options) {
+                                       bool secure_source,
+                                       bool modify_http_only) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   Time creation_time = cc->CreationDate();
   const std::string key(GetKey(cc->Domain()));
   bool already_expired = cc->IsExpired(creation_time);
 
-  if (DeleteAnyEquivalentCookie(key, *cc, source_url,
-                                options.exclude_httponly(), already_expired)) {
+  if (DeleteAnyEquivalentCookie(key, *cc, secure_source, !modify_http_only,
+                                already_expired)) {
     std::string error;
     error =
         "SetCookie() not clobbering httponly cookie or secure cookie for "
@@ -1821,7 +1797,22 @@ bool CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
           (cc->ExpiryDate() - creation_time).InMinutes());
     }
 
-    InternalInsertCookie(key, std::move(cc), source_url, true);
+    // Histogram the type of scheme used on URLs that set cookies. This
+    // intentionally includes cookies that are set or overwritten by
+    // http:// URLs, but not cookies that are cleared by http:// URLs, to
+    // understand if the former behavior can be deprecated for Secure
+    // cookies.
+    CookieSource cookie_source_sample =
+        (secure_source
+             ? (cc->IsSecure()
+                    ? COOKIE_SOURCE_SECURE_COOKIE_CRYPTOGRAPHIC_SCHEME
+                    : COOKIE_SOURCE_NONSECURE_COOKIE_CRYPTOGRAPHIC_SCHEME)
+             : (cc->IsSecure()
+                    ? COOKIE_SOURCE_SECURE_COOKIE_NONCRYPTOGRAPHIC_SCHEME
+                    : COOKIE_SOURCE_NONSECURE_COOKIE_NONCRYPTOGRAPHIC_SCHEME));
+    histogram_cookie_source_scheme_->Add(cookie_source_sample);
+
+    InternalInsertCookie(key, std::move(cc), secure_source, true);
   } else {
     VLOG(kVlogSetCookies) << "SetCookie() not storing already expired cookie.";
   }
@@ -1836,20 +1827,33 @@ bool CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
   return true;
 }
 
-bool CookieMonster::SetCanonicalCookies(const CookieList& list) {
+bool CookieMonster::SetAllCookies(const CookieList& list) {
   DCHECK(thread_checker_.CalledOnValidThread());
+  for (const auto& cookie : list) {
+    const std::string key(GetKey(cookie.Domain()));
+    Time creation_time = cookie.CreationDate();
+    bool already_expired = cookie.IsExpired(creation_time);
 
-  CookieOptions options;
-  options.set_include_httponly();
+    bool result =
+        DeleteAnyEquivalentCookie(key, cookie, false, false, already_expired);
+    DCHECK(!result);
 
-  for (const auto& cookie : list) {
-    // Use an empty GURL.  This method does not support setting secure cookies.
-    if (!SetCanonicalCookie(base::MakeUnique<CanonicalCookie>(cookie), GURL(),
-                            options)) {
-      return false;
+    if (already_expired)
+      continue;
+
+    if (cookie.IsPersistent()) {
+      histogram_expiration_duration_minutes_->Add(
+          (cookie.ExpiryDate() - creation_time).InMinutes());

[mmenke, 2017/06/09 18:48:52]

It seems to me like this histogram should only be updated when
histogram_cookie_source_scheme_ is.  Having them include different sets of
cookies seems very confusing.

[Randy Smith (Not in Mondays), 2017/06/13 20:53:41]

I am attempting to faithfully replicate the behavior that was present before
this CL.  If you're suggesting that the original behavior was confusing I'm not
going to argue with you, but I am going to request a tell-me-twice before I
attempt to change it :-} (and I'll change it in a separate CL, which will
include revving histograms).

If you're perceiving that I am changing behavior in this CL, my argument that
I'm not is that, before this CL, SetCanonicalCookies() called SetCanonicalCookie
with a null GURL, which unilaterally did this histogram entry in the
!already_expired && IsPersistent() case, and then called InternalInsertCookie,
which didn't do the histogram_cookie_source_source_scheme_ insertion because of
the null GURL.  Do you see a hole in that logic?

     }
+
+    InternalInsertCookie(key, base::MakeUnique<CanonicalCookie>(cookie), false,
+                         true);

[mmenke, 2017/06/09 18:48:52]

So you're also removing these from the histogram?  That's the only reason for
copying duplicate logic to this method?

[Randy Smith (Not in Mondays), 2017/06/13 20:53:40]

Yes, modulo I believe I'm not actually changing behavior.   The places that
passed in null GURLs disabled the histogram_cookie_source_scheme_ insertion in
InternalInsertCookie.  One of those cases I could deal with without the wonky
null GURL signature just by pulling the histogram insertion up into
SetCanonicalCookie().  This is the other one, and duplicating logic struck me as
the simplest way to handle the case (as opposed to a boolean saying "Don't enter
this into the histogram"), especially since this method isn't actually called by
iOS and really should be gotten rid of at some point.

   }
 
+  // TODO(rdsmith): If this function always returns the same value, it
+  // shouldn't have a return value.  But it should also be deleted (see
+  // https://codereview.chromium.org/2882063002/#msg64), which would
+  // solve the return value problem.
   return true;
 }