Add a SetCanonicalCookie method for CookieMonster.

net/cookies/cookie_monster.cc

 // Copyright (c) 2012 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.
 
 // Portions of this code based on Mozilla:
 //   (netwerk/cookie/src/nsCookieService.cpp)
 /* ***** BEGIN LICENSE BLOCK *****
  * Version: MPL 1.1/GPL 2.0/LGPL 2.1
  *
  * The contents of this file are subject to the Mozilla Public License Version
@@ skipping 44 matching lines @@
 #include "base/logging.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/field_trial.h"
 #include "base/metrics/histogram.h"
 #include "base/profiler/scoped_tracker.h"
 #include "base/single_thread_task_runner.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/time.h"

[mmenke, 2017/06/16 15:56:28]

nit:  Not needed, already included in header.

[Randy Smith (Not in Mondays), 2017/06/16 21:38:00]

Done.

 #include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/cookies/canonical_cookie.h"
 #include "net/cookies/cookie_util.h"
 #include "net/cookies/parsed_cookie.h"
 #include "net/ssl/channel_id_service.h"
 #include "url/origin.h"
 
 using base::Time;
 using base::TimeDelta;
 using base::TimeTicks;
@@ skipping 612 matching lines @@
  private:
   CanonicalCookie cookie_;
 
   DISALLOW_COPY_AND_ASSIGN(DeleteCanonicalCookieTask);
 };
 
 int CookieMonster::DeleteCanonicalCookieTask::RunDeleteTask() {
   return this->cookie_monster()->DeleteCanonicalCookie(cookie_);
 }
 
+// Task class for SetCanonicalCookie call.
+class CookieMonster::SetCanonicalCookieTask : public CookieMonsterTask {
+ public:
+  SetCanonicalCookieTask(CookieMonster* cookie_monster,
+                         std::unique_ptr<CanonicalCookie> cookie,
+                         bool secure_source,
+                         bool modify_http_only,
+                         const SetCookiesCallback& callback)
+      : CookieMonsterTask(cookie_monster),
+        cookie_(std::move(cookie)),
+        secure_source_(secure_source),
+        modify_http_only_(modify_http_only),
+        callback_(callback) {}
+
+  // CookieMonsterTask:
+  void Run() override;
+
+ protected:
+  ~SetCanonicalCookieTask() override {}
+
+ private:
+  std::unique_ptr<CanonicalCookie> cookie_;
+  bool secure_source_;
+  bool modify_http_only_;
+  SetCookiesCallback callback_;
+
+  DISALLOW_COPY_AND_ASSIGN(SetCanonicalCookieTask);
+};
+
+void CookieMonster::SetCanonicalCookieTask::Run() {
+  bool result = this->cookie_monster()->SetCanonicalCookie(
+      std::move(cookie_), secure_source_, modify_http_only_);
+  if (!callback_.is_null())
+    callback_.Run(result);
+}
+
 // Task class for SetCookieWithOptions call.
 class CookieMonster::SetCookieWithOptionsTask : public CookieMonsterTask {
  public:
   SetCookieWithOptionsTask(CookieMonster* cookie_monster,
                            const GURL& url,
                            const std::string& cookie_line,
                            const CookieOptions& options,
                            const SetCookiesCallback& callback)
       : CookieMonsterTask(cookie_monster),
         url_(url),
@@ skipping 188 matching lines @@
     store_->SetForceKeepSessionState();
 }
 
 void CookieMonster::SetAllCookiesAsync(const CookieList& list,
                                        const SetCookiesCallback& callback) {
   scoped_refptr<SetAllCookiesTask> task =
       new SetAllCookiesTask(this, list, callback);
   DoCookieTask(task);
 }
 
+void CookieMonster::SetCanonicalCookieAsync(
+    const CanonicalCookie& cookie,
+    bool secure_source,
+    bool modify_http_only,
+    const SetCookiesCallback& callback) {
+  DCHECK(cookie.IsCanonical());
+  scoped_refptr<SetCanonicalCookieTask> task = new SetCanonicalCookieTask(
+      this, base::MakeUnique<CanonicalCookie>(cookie), secure_source,

[mmenke, 2017/06/16 15:56:28]

Should this take a unique_ptr<CanonicalCookie> instead?  Does anyone need to
have their cookie and eat it too (Or at least feed it to the monster, too)?

[Randy Smith (Not in Mondays), 2017/06/16 21:38:00]

I think I'm going to say "Yeah, that makes sense", though it pushes a problem
off until the next CL.  This interface is being created for use by the Mojo
interface, and I don't think I can get mojo to give me a std::unique_ptr<>, so
I'm just going to have to copy it at that level.  But maybe I can find a way
around that when I'm focussing on the mojo CL, and I don't think there's any
gain even if I can't in doing the copy here instead of there.  Done.

+      modify_http_only, callback);
+
+  // TODO(rdsmith): Switch to DoCookieTaskForURL (or the equivalent).
+  // This is tricky because we don't have the scheme in this routine
+  // and DoCookieTaskForURL uses cookie_util::GetEffectiveDomain(scheme, host)
+  // to generate the database key to block behind.
+  DoCookieTask(task);
+}
+
 void CookieMonster::SetCookieWithOptionsAsync(
     const GURL& url,
     const std::string& cookie_line,
     const CookieOptions& options,
     const SetCookiesCallback& callback) {
   scoped_refptr<SetCookieWithOptionsTask> task =
       new SetCookieWithOptionsTask(this, url, cookie_line, options, callback);
 
   DoCookieTaskForURL(task, url);
 }
@@ skipping 146 matching lines @@
                                          base::Time last_access_time,
                                          bool secure,
                                          bool http_only,
                                          CookieSameSite same_site,
                                          CookiePriority priority) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (!HasCookieableScheme(url))
     return false;
 
-  // TODO(mmenke): This class assumes each cookie to have a unique creation
-  // time. Allowing the caller to set the creation time violates that
-  // assumption. Worth fixing? Worth noting that time changes between browser
-  // restarts can cause the same issue.
-  base::Time actual_creation_time = creation_time;
-  if (creation_time.is_null()) {
-    actual_creation_time = CurrentTime();
-    last_time_seen_ = actual_creation_time;
-  }
-
   // Validate consistency of passed arguments.
   if (ParsedCookie::ParseTokenString(name) != name ||
       ParsedCookie::ParseValueString(value) != value ||
       ParsedCookie::ParseValueString(domain) != domain ||
       ParsedCookie::ParseValueString(path) != path) {
     return false;
   }
 
   std::string cookie_domain;
   if (!cookie_util::GetCookieDomainWithString(url, domain, &cookie_domain))
     return false;
 
   std::string cookie_path = CanonicalCookie::CanonPathWithString(url, path);
   if (!path.empty() && cookie_path != path)
     return false;
 
   // Canonicalize path again to make sure it escapes characters as needed.
   url::Component path_component(0, cookie_path.length());
   url::RawCanonOutputT<char> canon_path;
   url::Component canon_path_component;
   url::CanonicalizePath(cookie_path.data(), path_component, &canon_path,
                         &canon_path_component);
   cookie_path = std::string(canon_path.data() + canon_path_component.begin,
                             canon_path_component.len);
 
   std::unique_ptr<CanonicalCookie> cc(base::MakeUnique<CanonicalCookie>(
-      name, value, cookie_domain, cookie_path, actual_creation_time,
-      expiration_time, last_access_time, secure, http_only, same_site,
-      priority));
+      name, value, cookie_domain, cookie_path, creation_time, expiration_time,
+      last_access_time, secure, http_only, same_site, priority));
 
   return SetCanonicalCookie(std::move(cc), url.SchemeIsCryptographic(), true);
 }
 
 CookieList CookieMonster::GetAllCookies() {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   // This function is being called to scrape the cookie list for management UI
   // or similar.  We shouldn't show expired cookies in this list since it will
   // just be confusing to users, and this function is called rarely enough (and
@@ skipping 639 matching lines @@
 }
 
 bool CookieMonster::SetCanonicalCookie(std::unique_ptr<CanonicalCookie> cc,
                                        bool secure_source,
                                        bool modify_http_only) {
   DCHECK(thread_checker_.CalledOnValidThread());
 
   if (cc->IsSecure() && !secure_source)
     return false;
 
-  Time creation_time = cc->CreationDate();
   const std::string key(GetKey(cc->Domain()));
-  bool already_expired = cc->IsExpired(creation_time);
+
+  // TODO(mmenke): This class assumes each cookie to have a unique creation
+  // time. Allowing the caller to set the creation time violates that
+  // assumption. Worth fixing? Worth noting that time changes between browser
+  // restarts can cause the same issue.
+  base::Time creation_date = cc->CreationDate();
+  if (creation_date.is_null()) {
+    creation_date = CurrentTime();
+    cc->SetCreationDate(creation_date);
+    last_time_seen_ = creation_date;
+  }
+  bool already_expired = cc->IsExpired(creation_date);
 
   if (DeleteAnyEquivalentCookie(key, *cc, secure_source, !modify_http_only,
                                 already_expired)) {
     std::string error;
     error =
         "SetCookie() not clobbering httponly cookie or secure cookie for "
         "insecure scheme";
 
     VLOG(kVlogSetCookies) << error;
     return false;
   }
 
   VLOG(kVlogSetCookies) << "SetCookie() key: " << key
                         << " cc: " << cc->DebugString();
 
   // Realize that we might be setting an expired cookie, and the only point
   // was to delete the cookie which we've already done.
   if (!already_expired) {
     // See InitializeHistograms() for details.
     if (cc->IsPersistent()) {
       histogram_expiration_duration_minutes_->Add(
-          (cc->ExpiryDate() - creation_time).InMinutes());
+          (cc->ExpiryDate() - creation_date).InMinutes());
     }
 
     // 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), true);
   } else {
     VLOG(kVlogSetCookies) << "SetCookie() not storing already expired cookie.";
   }
 
   // We assume that hopefully setting a cookie will be less common than
   // querying a cookie.  Since setting a cookie can put us over our limits,
   // make sure that we garbage collect...  We can also make the assumption that
   // if a cookie was set, in the common case it will be used soon after,
   // and we will purge the expired cookies in GetCookies().
-  GarbageCollect(creation_time, key);
+  GarbageCollect(creation_date, key);

[mmenke, 2017/06/16 15:56:28]

So we just accept a creation date from the consumer, and we garbage collect,
just assuming it's the current time?  That seems not great.

[Randy Smith (Not in Mondays), 2017/06/16 21:38:00]

Good point, but I think it's currently existing behavior: SetCookieWithDetails
-> SetCanonicalCookie -> GarbageCollect()?

 
   return true;
 }
 
 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);
@@ skipping 467 matching lines @@
       "Cookie.Count", 1, 4000, 50, base::Histogram::kUmaTargetedHistogramFlag);
 
   // From UMA_HISTOGRAM_ENUMERATION
   histogram_cookie_deletion_cause_ = base::LinearHistogram::FactoryGet(
       "Cookie.DeletionCause", 1, DELETE_COOKIE_LAST_ENTRY - 1,
       DELETE_COOKIE_LAST_ENTRY, base::Histogram::kUmaTargetedHistogramFlag);
   histogram_cookie_type_ = base::LinearHistogram::FactoryGet(
       "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,

[mmenke, 2017/06/16 15:56:28]

Isn't this the histogram you renamed in the XML file?  Is that supposed to be
part of another CL?

[Randy Smith (Not in Mondays), 2017/06/16 21:38:00]

Whoops.  Removed renaming from the xml file.  

       COOKIE_SOURCE_LAST_ENTRY, base::Histogram::kUmaTargetedHistogramFlag);
   histogram_cookie_delete_equivalent_ = base::LinearHistogram::FactoryGet(
       "Cookie.CookieDeleteEquivalent", 1,
       COOKIE_DELETE_EQUIVALENT_LAST_ENTRY - 1,
       COOKIE_DELETE_EQUIVALENT_LAST_ENTRY,
       base::Histogram::kUmaTargetedHistogramFlag);
 
   // From UMA_HISTOGRAM_{CUSTOM_,}TIMES
   histogram_time_blocked_on_load_ = base::Histogram::FactoryTimeGet(
       "Cookie.TimeBlockedOnLoad", base::TimeDelta::FromMilliseconds(1),
@@ skipping 123 matching lines @@
        it != hook_map_.end(); ++it) {
     std::pair<GURL, std::string> key = it->first;
     if (cookie.IncludeForRequestURL(key.first, opts) &&
         cookie.Name() == key.second) {
       it->second->Notify(cookie, cause);
     }
   }
 }
 
 }  // namespace net