Moves SQLitePersistentCookieStore to net/extras/sqlite.

content/browser/net/quota_policy_cookie_store.cc

+// Copyright 2015 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 "content/browser/net/quota_policy_cookie_store.h"
+
+#include <list>
+
+#include "base/basictypes.h"
+#include "base/bind.h"
+#include "base/callback.h"
+#include "base/files/file_path.h"
+#include "base/files/file_util.h"
+#include "base/memory/ref_counted.h"
+#include "base/memory/scoped_ptr.h"
+#include "base/profiler/scoped_tracker.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/cookie_store_factory.h"
+#include "net/cookies/canonical_cookie.h"
+#include "net/cookies/cookie_constants.h"
+#include "net/cookies/cookie_util.h"
+#include "net/extras/sqlite/cookie_crypto_delegate.h"
+#include "storage/browser/quota/special_storage_policy.h"
+#include "url/gurl.h"
+
+namespace content {
+
+QuotaPolicyCookieStore::QuotaPolicyCookieStore(
+    const base::FilePath& path,
+    const scoped_refptr<base::SequencedTaskRunner>& client_task_runner,
+    const scoped_refptr<base::SequencedTaskRunner>& background_task_runner,
+    bool restore_old_session_cookies,
+    storage::SpecialStoragePolicy* special_storage_policy,
+    net::CookieCryptoDelegate* crypto_delegate)
+    : special_storage_policy_(special_storage_policy),
+      persistent_store_(
+          new net::SQLitePersistentCookieStore(path,
+                                               client_task_runner,
+                                               background_task_runner,
+                                               restore_old_session_cookies,
+                                               crypto_delegate)) {

[Ryan Sleevi, 2015/05/06 00:44:18]

DESIGN: Does it make more sense to have the caller pass in the
SQLitePersistentCookieStore?

I don't see us use |path|, |client_task_runner|, |background_task_runner|,
|restore_old_session_cookies|, or |crypto_delegate| beyond this invocation.

[rohitrao (ping after 24h), 2015/05/11 19:25:29]

Makes sense, changed the API.

+  DCHECK(client_task_runner);
+  DCHECK(background_task_runner);

[Ryan Sleevi, 2015/05/06 00:44:18]

These aren't needed here, are they? They're invariants for the
SQLitePersistentCookieStore to ensure, since we don't seem to do anything with
them.

[rohitrao (ping after 24h), 2015/05/11 19:25:29]

Obsolete now.

+}
+
+QuotaPolicyCookieStore::~QuotaPolicyCookieStore() {
+  if (!special_storage_policy_.get() ||
+      !special_storage_policy_->HasSessionOnlyOrigins()) {
+    return;
+  }
+
+  std::list<net::SQLitePersistentCookieStore::CookieOrigin>
+      session_only_cookies;
+  for (const auto& cookie : cookies_per_origin_) {
+    if (cookie.second <= 0) {
+      DCHECK_EQ(0, cookie.second);
+      continue;
+    }

[Ryan Sleevi, 2015/05/06 00:44:17]

Wat?

It seems like "<0" is an impossible condition, given this DCHECK. As a result,
making this size_t and just checking == 0 seems eminently reasonable. You could
then get rid of the DCHECK on line 56.

[rohitrao (ping after 24h), 2015/05/11 19:25:29]

Done.  I added a new DCHECK to DeleteCookie() to catch cases where we might try
to decrement below 0.

+    const GURL url(net::cookie_util::CookieOriginToURL(cookie.first.first,
+                                                       cookie.first.second));
+    if (!url.is_valid() || !special_storage_policy_->IsStorageSessionOnly(url))
+      continue;
+
+    session_only_cookies.push_back(cookie.first);
+  }
+
+  persistent_store_->DeleteAllInList(session_only_cookies);
+}
+
+void QuotaPolicyCookieStore::Load(const LoadedCallback& loaded_callback) {
+  persistent_store_->Load(
+      base::Bind(&QuotaPolicyCookieStore::OnLoad, this, loaded_callback));

[Ryan Sleevi, 2015/05/06 00:44:18]

DESIGN: It does strike me that this may be potentially dangerous

assume the following

LoadedCallback something = base::Bind(&whatever);
{
  scoped_refptr<QuotaPolicyCookieStore> cookie_store;
  cookie_store->Load(something);
}

^-- will still fire something, even after |cookie_store| has gone out of scope,
due to this binding taking a reference

The existing SQLitePersistentCookieStore tries to handle this a little (c.f "if
(!db_)" and handling Close()), but this doesn't, and I'm curious what the
implications may or may not be.

[rohitrao (ping after 24h), 2015/05/11 19:25:29]

Isn't the caller making lifetime assumptions that it shouldn't?  If you have a
scoped_refptr, having it go out of scope is meaningless.  If the calling object
is being destroyed and doesn't want the callback to be called, shouldn't it
explicitly need to use a WeakPtr or something?

I might be misunderstanding the problem, but this seems like it's a general case
of reference counting being an awful system.

[Ryan Sleevi, 2015/05/12 00:48:33]

What lifetime assumptions do you see it making?

It's an extremely common pattern in //net to allow an object to be deleted
before-or-during a callback. Indeed, all of our socket APIs rely on this - e.g.
it's valid to delete a StreamSocket while it's invoking its Read() callback, and
it's valid to delete a StreamSocket while you have an outstanding Read() that
hasn't invoked the callback.

The problem of design here is one that this class shares different semantics
than its base class, in that the base class goes out of its way to avoid
invoking callbacks after a Close(), but it seems we don't have that guarantee
here.
It's wholly independent of reference counting, and more a statement about what
callers can/should expect if they release the object.

+}
+
+void QuotaPolicyCookieStore::LoadCookiesForKey(
+    const std::string& key,
+    const LoadedCallback& loaded_callback) {
+  persistent_store_->LoadCookiesForKey(
+      key,
+      base::Bind(&QuotaPolicyCookieStore::OnLoad, this, loaded_callback));
+}
+
+void QuotaPolicyCookieStore::AddCookie(const net::CanonicalCookie& cc) {
+  cookies_per_origin_[
+      net::SQLitePersistentCookieStore::CookieOrigin(cc.Domain(),
+                                                     cc.IsSecure())]++;

[Ryan Sleevi, 2015/05/06 00:44:17]

This is valid, but pretty unreadable.

net::SQLitePersistentCookieStore::CookieOrigin origin(cc.Domain(),
cc.IsSecure());
++cookies_per_origin_[origin];
persistent_store_->AddCookie(cc);

?

[rohitrao (ping after 24h), 2015/05/11 19:25:30]

Done.

+  persistent_store_->AddCookie(cc);
+}
+
+void QuotaPolicyCookieStore::UpdateCookieAccessTime(
+    const net::CanonicalCookie& cc) {
+  persistent_store_->UpdateCookieAccessTime(cc);
+}
+
+void QuotaPolicyCookieStore::DeleteCookie(const net::CanonicalCookie& cc) {
+  cookies_per_origin_[
+      net::SQLitePersistentCookieStore::CookieOrigin(cc.Domain(),
+                                                     cc.IsSecure())]--;

[Ryan Sleevi, 2015/05/06 00:44:18]

ditto here

[rohitrao (ping after 24h), 2015/05/11 19:25:29]

Done.

+  persistent_store_->DeleteCookie(cc);
+}
+
+void QuotaPolicyCookieStore::SetForceKeepSessionState() {
+  special_storage_policy_ = NULL;

[Ryan Sleevi, 2015/05/06 00:44:18]

nullptr

[rohitrao (ping after 24h), 2015/05/11 19:25:30]

Done.

+}
+
+void QuotaPolicyCookieStore::Flush(const base::Closure& callback) {
+  persistent_store_->Flush(callback);
+}
+
+void QuotaPolicyCookieStore::OnLoad(
+    const LoadedCallback& loaded_callback,
+    const std::vector<net::CanonicalCookie*>& cookies) {
+  for (const auto& cookie : cookies) {
+    cookies_per_origin_[
+        net::SQLitePersistentCookieStore::CookieOrigin(cookie->Domain(),
+                                                       cookie->IsSecure())]++;
+  }
+
+  loaded_callback.Run(cookies);
+}
+
+CookieStoreConfig::CookieStoreConfig()

[Ryan Sleevi, 2015/05/06 00:44:18]

It seems everything from here & below isn't defined in the header. Shouldn't
this all be nuked/nukable? What am I missing?

[rohitrao (ping after 24h), 2015/05/11 19:25:30]

This is copied straight from the old
content/net/browser/sqlite_persistent_cookie_store.cc file.  I think no one ever
bothered to split it out into a separate file?  I can do that in a followup CL
and see what content owners think.

[Ryan Sleevi, 2015/05/12 00:48:33]

Yeah, I think splitting it up sounds right, but happy to defer that. Definitely
a smell :)

+  : session_cookie_mode(EPHEMERAL_SESSION_COOKIES),
+    crypto_delegate(NULL) {

[Ryan Sleevi, 2015/05/06 00:44:18]

nullptr

[rohitrao (ping after 24h), 2015/05/11 19:25:30]

Done.

+  // Default to an in-memory cookie store.
+}
+
+CookieStoreConfig::CookieStoreConfig(
+    const base::FilePath& path,
+    SessionCookieMode session_cookie_mode,
+    storage::SpecialStoragePolicy* storage_policy,
+    net::CookieMonsterDelegate* cookie_delegate)
+    : path(path),
+      session_cookie_mode(session_cookie_mode),
+      storage_policy(storage_policy),
+      cookie_delegate(cookie_delegate),
+      crypto_delegate(NULL) {

[Ryan Sleevi, 2015/05/06 00:44:17]

nullptr

[rohitrao (ping after 24h), 2015/05/11 19:25:30]

Done.

+  CHECK(!path.empty() || session_cookie_mode == EPHEMERAL_SESSION_COOKIES);
+}
+
+CookieStoreConfig::~CookieStoreConfig() {
+}
+
+net::CookieStore* CreateCookieStore(const CookieStoreConfig& config) {
+  // TODO(bcwhite): Remove ScopedTracker below once crbug.com/483686 is fixed.
+  tracked_objects::ScopedTracker tracking_profile(
+      FROM_HERE_WITH_EXPLICIT_FUNCTION("483686 content::CreateCookieStore"));
+
+  net::CookieMonster* cookie_monster = NULL;

[Ryan Sleevi, 2015/05/06 00:44:17]

nullptr

[rohitrao (ping after 24h), 2015/05/11 19:25:29]

Done.

+
+  if (config.path.empty()) {
+    // Empty path means in-memory store.
+    cookie_monster = new net::CookieMonster(NULL, config.cookie_delegate.get());
+  } else {
+    scoped_refptr<base::SequencedTaskRunner> client_task_runner =
+        config.client_task_runner;
+    scoped_refptr<base::SequencedTaskRunner> background_task_runner =
+        config.background_task_runner;
+
+    if (!client_task_runner.get()) {
+      client_task_runner =
+          BrowserThread::GetMessageLoopProxyForThread(BrowserThread::IO);
+    }
+
+    if (!background_task_runner.get()) {
+      background_task_runner =
+          BrowserThread::GetBlockingPool()->GetSequencedTaskRunner(
+              BrowserThread::GetBlockingPool()->GetSequenceToken());
+    }
+
+    QuotaPolicyCookieStore* persistent_store =
+        new QuotaPolicyCookieStore(
+            config.path,
+            client_task_runner,
+            background_task_runner,
+            (config.session_cookie_mode ==
+             CookieStoreConfig::RESTORED_SESSION_COOKIES),

[Ryan Sleevi, 2015/05/06 00:44:17]

Is this all formatted with git cl format?

[rohitrao (ping after 24h), 2015/05/11 19:25:30]

Not yet.  I figured we would just take the git cl format changes wholesale,
because they are mandatory for net/, and I didn't want to have random diffs
confusing the review.  There will be a bunch of formatting diffs, because this
code is being newly imported into net/.

(Actually, this specific file won't get git cl formatted at all, because it
still lives in content/.)

+            config.storage_policy.get(),
+            config.crypto_delegate);
+
+    cookie_monster =
+        new net::CookieMonster(persistent_store, config.cookie_delegate.get());
+    if ((config.session_cookie_mode ==
+         CookieStoreConfig::PERSISTANT_SESSION_COOKIES) ||
+        (config.session_cookie_mode ==
+         CookieStoreConfig::RESTORED_SESSION_COOKIES)) {
+      cookie_monster->SetPersistSessionCookies(true);
+    }
+  }
+
+  return cookie_monster;
+}
+
+}  // namespace content