Make CookieStore no longer threadsafe.

Make CookieStore no longer threadsafe.

In practice, it's only used on one thread, and it's one of the few net
classes that tries to be threadsafe.  This also removes a lot of
locking, which Chromium code generally tries to avoid doing.

Other than removing thread-safety, and invoking FlushStore's
callback on the network thread, the API is unchanged.

BUG=46185
Committed: https://crrev.com/be0910d6b03b3433448d25587ac57035d227b0d9
Cr-Commit-Position: refs/heads/master@{#378505}

[mmenke, 2016-02-14 02:26:19]

Patchset #3 (id:10007) has been deleted

[mmenke, 2016-02-29 18:47:53]

Patchset #4 (id:70001) has been deleted

[mmenke, 2016-02-29 20:21:00]

Description was changed from

==========
Make CookieStore no longer threadsafe.

In practice, it's only used on one thread, and it's one of the few net
classes that tries to be threadsafe.  This also removes a lot of
locking, which Chromium code generally tries to avoid doing.

Other than removing thread-safety, the API is unchanged.

BUG=46185
==========

to

==========
Make CookieStore no longer threadsafe.

In practice, it's only used on one thread, and it's one of the few net
classes that tries to be threadsafe.  This also removes a lot of
locking, which Chromium code generally tries to avoid doing.

Other than removing thread-safety, and invoking FlushStore's
callback on the network thread, the API is unchanged.

BUG=46185
==========

[mmenke, 2016-02-29 20:22:04]

mmenke@chromium.org changed reviewers:
+ mkwst@chromium.org

[mmenke, 2016-02-29 20:22:05]

None of the FlushStore() callers seem to care about when it's called.  And since
calling it before any other changes were made to the store was only done on a
best effort basis before, no one should have been depending on when it's called.

[Randy Smith (Not in Mondays), 2016-02-29 21:10:30]

rdsmith@chromium.org changed reviewers:
+ rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2016-02-29 21:10:30]

[Drive by, not planning to do a full review, please don't hold on my stamp
without letting me know you're doing that :-}.]

Is there some reason you're not using NonThreadSafe or some other set of
assertions to guarantee that this class really is only used on one thread?  I
did a codesearch for instantiations, and it looks to me as if it's used in
several places other than the main use from URLRequestContext
(system_policy_request_context, quota_cookie_store).  And I know that it at
least used to be used as part of the fine-grained cookie control interface,
though I don't see any instantiations that fit that usage profile.  But some
DCHECK level assertions for no cross-thread accesses make some sense to me?

[mmenke, 2016-02-29 21:15:23]

On 2016/02/29 21:10:30, Randy Smith - Not in Fridays wrote:
> [Drive by, not planning to do a full review, please don't hold on my stamp
> without letting me know you're doing that :-}.]
> 
> Is there some reason you're not using NonThreadSafe or some other set of
> assertions to guarantee that this class really is only used on one thread?  I
> did a codesearch for instantiations, and it looks to me as if it's used in
> several places other than the main use from URLRequestContext
> (system_policy_request_context, quota_cookie_store).  And I know that it at
> least used to be used as part of the fine-grained cookie control interface,
> though I don't see any instantiations that fit that usage profile.  But some
> DCHECK level assertions for no cross-thread accesses make some sense to me?

I am using ThreadChecker...I'm just not invoking it explicitly in all the
methods that call DoCookieTask/DoCookieTaskForURL, in favor of doing it in those
two methods.

[Randy Smith (Not in Mondays), 2016-02-29 21:15:33]

rdsmith@chromium.org changed reviewers:
- rdsmith@chromium.org

[Randy Smith (Not in Mondays), 2016-02-29 21:15:34]

Please belay that question; didn't look closely enough.  Carry on :-}.

[mmenke, 2016-02-29 21:22:07]

On 2016/02/29 21:15:23, mmenke wrote:
> On 2016/02/29 21:10:30, Randy Smith - Not in Fridays wrote:
> > [Drive by, not planning to do a full review, please don't hold on my stamp
> > without letting me know you're doing that :-}.]
> > 
> > Is there some reason you're not using NonThreadSafe or some other set of
> > assertions to guarantee that this class really is only used on one thread? 
I
> > did a codesearch for instantiations, and it looks to me as if it's used in
> > several places other than the main use from URLRequestContext
> > (system_policy_request_context, quota_cookie_store).  And I know that it at
> > least used to be used as part of the fine-grained cookie control interface,
> > though I don't see any instantiations that fit that usage profile.  But some
> > DCHECK level assertions for no cross-thread accesses make some sense to me?
> 
> I am using ThreadChecker...I'm just not invoking it explicitly in all the
> methods that call DoCookieTask/DoCookieTaskForURL, in favor of doing it in
those
> two methods.

Looking at those two places, system_policy_request_context creates it on a
"network task runner", which is presumably also where it's used. 
QuotaPolicyCookieStore is a PersistentCookieStore, not a CookieStore, which,
confusingly, are completely different classes.

Note that my CL to remove refcounting from CookieStores works on top of this, so
I can confirm CookieStores are at least not being passed between threads, and
all owners of CookieStores seem to only be single-thread classes (Except the one
on Android Webivew, but that one makes sure all calls to the store are on a
single thread).  I waited until I had my refcounting CL working before sending
this one out for review as precisely a double-check that everything is happning
on a single thread (And that's how I caught Android WebView).

There's no guarantee that someone doesn't grab a raw pointer and use it on
another thread, but I'm fairly certain that no CookieStore owner is used on
multiple threads.  You can look at them all in my other CL.

[Mike West, 2016-03-01 13:59:28]

LGTM. I really like seeing a million unit tests become obsolete, and all those
AutoLock removals! :)

https://codereview.chromium.org/1698693002/diff/90001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.cc (left):

https://codereview.chromium.org/1698693002/diff/90001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:1415: base::AutoLock autolock(lock_);
Appropos of nothing, I really like seeing all these AutoLock lines being
removed. :)

https://codereview.chromium.org/1698693002/diff/90001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/1698693002/diff/90001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:544: base::Closure RunDeleteTaskAndBindCallback();
I'd suggest doing so, but in a separate CL. No need to refactor everything at
once.

https://codereview.chromium.org/1698693002/diff/90001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:1346: // TODO(mmenke): Can this be turned into a
DCHECK?
I think so. There shouldn't be any case in which we call `OnKeyLoaded` without
having asked for a key to be loaded at some point.

https://codereview.chromium.org/1698693002/diff/90001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:1350: // Run all tasks for the key. Note that
running a task can result it multiple
Nit: s/it multiple/in multiple/

https://codereview.chromium.org/1698693002/diff/90001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:1354: // necessary, but just seems like a good
idea.
Nit: I'd drop the comment. I'd agree that it's a good idea, and doesn't seem
like it needs justification. :)

https://codereview.chromium.org/1698693002/diff/90001/net/extras/sqlite/sqlit...
File net/extras/sqlite/sqlite_persistent_cookie_store.cc (right):

https://codereview.chromium.org/1698693002/diff/90001/net/extras/sqlite/sqlit...
net/extras/sqlite/sqlite_persistent_cookie_store.cc:516: const base::Closure&
callback) {
Combining these makes good sense, thank you.

[mmenke, 2016-03-01 16:03:45]

Thanks!

https://codereview.chromium.org/1698693002/diff/90001/net/cookies/cookie_mons...
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/1698693002/diff/90001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:1346: // TODO(mmenke): Can this be turned into a
DCHECK?
On 2016/03/01 13:59:28, Mike West wrote:
> I think so. There shouldn't be any case in which we call `OnKeyLoaded` without
> having asked for a key to be loaded at some point.

My CL to re-order things may change that property (This comment pre-dates that
CL by over a month), but still may be able to do better.

https://codereview.chromium.org/1698693002/diff/90001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:1350: // Run all tasks for the key. Note that
running a task can result it multiple
On 2016/03/01 13:59:28, Mike West wrote:
> Nit: s/it multiple/in multiple/

Done.

https://codereview.chromium.org/1698693002/diff/90001/net/cookies/cookie_mons...
net/cookies/cookie_monster.cc:1354: // necessary, but just seems like a good
idea.
On 2016/03/01 13:59:28, Mike West wrote:
> Nit: I'd drop the comment. I'd agree that it's a good idea, and doesn't seem
> like it needs justification. :)

Done.

[mmenke, 2016-03-01 16:03:53]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2016-03-01 16:03:54]

The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org
Link to the patchset: https://codereview.chromium.org/1698693002/#ps110001
(title: "response to comments")

[commit-bot: I haz the power, 2016-03-01 16:04:16]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1698693002/110001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1698693002/110001

[commit-bot: I haz the power, 2016-03-01 19:09:16]

Description was changed from

==========
Make CookieStore no longer threadsafe.

In practice, it's only used on one thread, and it's one of the few net
classes that tries to be threadsafe.  This also removes a lot of
locking, which Chromium code generally tries to avoid doing.

Other than removing thread-safety, and invoking FlushStore's
callback on the network thread, the API is unchanged.

BUG=46185
==========

to

==========
Make CookieStore no longer threadsafe.

In practice, it's only used on one thread, and it's one of the few net
classes that tries to be threadsafe.  This also removes a lot of
locking, which Chromium code generally tries to avoid doing.

Other than removing thread-safety, and invoking FlushStore's
callback on the network thread, the API is unchanged.

BUG=46185
==========

[commit-bot: I haz the power, 2016-03-01 19:09:17]

Committed patchset #5 (id:110001)

[commit-bot: I haz the power, 2016-03-01 19:10:36]

Description was changed from

==========
Make CookieStore no longer threadsafe.

In practice, it's only used on one thread, and it's one of the few net
classes that tries to be threadsafe.  This also removes a lot of
locking, which Chromium code generally tries to avoid doing.

Other than removing thread-safety, and invoking FlushStore's
callback on the network thread, the API is unchanged.

BUG=46185
==========

to

==========
Make CookieStore no longer threadsafe.

In practice, it's only used on one thread, and it's one of the few net
classes that tries to be threadsafe.  This also removes a lot of
locking, which Chromium code generally tries to avoid doing.

Other than removing thread-safety, and invoking FlushStore's
callback on the network thread, the API is unchanged.

BUG=46185

Committed: https://crrev.com/be0910d6b03b3433448d25587ac57035d227b0d9
Cr-Commit-Position: refs/heads/master@{#378505}
==========

[commit-bot: I haz the power, 2016-03-01 19:10:37]

Patchset 5 (id:??) landed as
https://crrev.com/be0910d6b03b3433448d25587ac57035d227b0d9
Cr-Commit-Position: refs/heads/master@{#378505}