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 f2bd3e4558249f5b0651df1e7d3ab4fe537bd882..67cab139dee02eb239b9d01e9d44e309f8bd6d8d 100644
--- a/net/cookies/cookie_monster.cc
+++ b/net/cookies/cookie_monster.cc
@@ -1092,11 +1092,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);

[mmenke, 2017/05/25 20:04:29]

So the INCLUDE_STRICT_AND_LAX here didn't matter?

[Randy Smith (Not in Mondays), 2017/06/09 11:19:32]

I didn't find anything under SetCanonicalCookie that tested that option, no.  I
presume I would have had a compile failure (or a clear change in behavior
somewhere else in the diff) if it had mattered, since the options variable is no
longer present to be tested under SetCanonicalCookie.

-  return SetCanonicalCookie(std::move(cc), url, options);
+  return SetCanonicalCookie(std::move(cc), url.SchemeIsCryptographic(), true);
 }
 
 CookieList CookieMonster::GetAllCookies() {
@@ -1415,7 +1411,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);
       const Time cookie_access_time(cookie_ptr->LastAccessDate());
       if (earliest_access_time_.is_null() ||
           cookie_access_time < earliest_access_time_)
@@ -1629,7 +1625,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());
@@ -1652,7 +1648,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(
@@ -1693,7 +1689,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();
@@ -1721,21 +1717,15 @@ CookieMonster::CookieMap::iterator CookieMonster::InternalInsertCookie(
   // 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);
-  }
+  CookieSource cookie_source_sample =
+      (source_secure
+           ? (cc_ptr->IsSecure()
+                  ? COOKIE_SOURCE_SECURE_COOKIE_CRYPTOGRAPHIC_SCHEME
+                  : COOKIE_SOURCE_NONSECURE_COOKIE_CRYPTOGRAPHIC_SCHEME)
+           : (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);
 
@@ -1764,20 +1754,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 "
@@ -1799,7 +1790,7 @@ bool CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
           (cc->ExpiryDate() - creation_time).InMinutes());
     }
 
-    InternalInsertCookie(key, std::move(cc), source_url, true);
+    InternalInsertCookie(key, std::move(cc), secure_source, true);
   } else {
     VLOG(kVlogSetCookies) << "SetCookie() not storing already expired cookie.";
   }
@@ -1822,8 +1813,8 @@ bool CookieMonster::SetCanonicalCookies(const CookieList& list) {
 
   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)) {
+    if (!SetCanonicalCookie(base::MakeUnique<CanonicalCookie>(cookie), false,
+                            true)) {
       return false;
     }
   }
@@ -2255,7 +2246,7 @@ void CookieMonster::InitializeHistograms() {
       "Cookie.Type", 1, (1 << COOKIE_TYPE_LAST_ENTRY) - 1,
       1 << COOKIE_TYPE_LAST_ENTRY, base::Histogram::kUmaTargetedHistogramFlag);
   histogram_cookie_source_scheme_ = base::LinearHistogram::FactoryGet(
-      "Cookie.CookieSourceScheme", 1, COOKIE_SOURCE_LAST_ENTRY - 1,
+      "Cookie.CookieSourceScheme2", 1, COOKIE_SOURCE_LAST_ENTRY - 1,

[mmenke, 2017/05/25 20:04:29]

Need to update histograms.xml

[Randy Smith (Not in Mondays), 2017/06/09 11:19:32]

Huh.  Coudla sworn I did.  I wonder where I dropped that change on the floor.

ETA: Well, that escalated quickly.  In writing the comment describing more
precisely what the difference in the histograms was, I realized that I was
making a change that was probably *not* reasonable--I was now included
restoration of cookies from the backing database on startup in the histogram
when I hadn't been before.  So I took a step back and refactored the code s.t.
the only places that actually put entries into this histogram were places where
the URL existed previously (i.e. the places where histogram entries originally
occurred).  So I don't need to change the histogram name anymore, as its
behavior should now be identical.

To give a bit more detail:
* The histogram was being entered in InternalInsertCookie (on all calls to that
function).
* That function was being called from StoreLoadedCookies (initialization path)
with a null URL (no histogram entry), and from SetCanonicalCookie()
* SetCanonicalCookie() was being called from several places that check if their
URL has a cookieable scheme (so presumably not a null URL) and
SetCanonicalCookie*s*() with a null URL.
* SetCanonicalCookies() was being called only from iOS.

I took advantage of droger@'s permission to not worry much about the semantics
of the SetCanonicalCookies() call, reimplemented it without calling
SetCanonicalCookie(), and pulled the histogram from InternalInsertCookie() into
SetCanonicalCookie().  I believe this keeps the histogram behavior identical to
before this refactor.

       COOKIE_SOURCE_LAST_ENTRY, base::Histogram::kUmaTargetedHistogramFlag);
   histogram_cookie_delete_equivalent_ = base::LinearHistogram::FactoryGet(
       "Cookie.CookieDeleteEquivalent", 1,