Fix CookieMonster garbage collection when no insecure cookies.

Fix CookieMonster garbage collection when no insecure cookies.

The code wanted to always keep at least one insecure cookie
around, so it could get the earliest access time of that cookie to
update earliest_access_time_, but when there weren't any such cookies,
this broke rather badly.

That logic for calculating the earliest_access_time_ was also wrong,
since it was set either only based on secure cookies, or only based
on insecure cookies.

This CL fixes both those issues.

BUG=730000
Review-Url: https://codereview.chromium.org/2924933002
Cr-Commit-Position: refs/heads/master@{#477689}
Committed: https://chromium.googlesource.com/chromium/src/+/f4721d99f41f62c860b20f1cd53911bdd069d338

[mmenke, 2017-06-06 21:23:17]

The CQ bit was checked by mmenke@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-06 21:23:49]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[mmenke, 2017-06-06 21:55:43]

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

[mmenke, 2017-06-06 21:55:44]

[mkwst]:  PTAL.  The bug is kinda horrifying, in that we're reading off into
random memory, but I don't think it's exploitable, and it would require a used
not having a single non-secure cookie set in the last 30 days (Or at least
having them all expired, or GCed at a per-host level, since they were set), so
probably not worth merging, though it could also completely break Chrome for
people, if they get into a situation where cookie GC always crashes.

[rdsmith]:  FYI.

[commit-bot: I haz the power, 2017-06-06 22:55:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-06 22:55:08]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-06-06 23:19:48]

https://codereview.chromium.org/2924933002/diff/1/net/cookies/cookie_monster_...
File net/cookies/cookie_monster_unittest.cc (right):

https://codereview.chromium.org/2924933002/diff/1/net/cookies/cookie_monster_...
net/cookies/cookie_monster_unittest.cc:2218: SetCookie(cm.get(),
GURL("http://newdomain.com"), "b=2");
If this were a secure cookie, this test would have run into the issue, but
figured it was best to make a new test.

[Mike West, 2017-06-07 06:55:47]

Oh, that's subtle. Thank you for tracking the bug down and submitting this
patch. The code change LGTM, but one of the comments is confusing, as noted
below. Would you mind poking at that before landing?

https://codereview.chromium.org/2924933002/diff/1/net/cookies/cookie_monster.cc
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/2924933002/diff/1/net/cookies/cookie_monster....
net/cookies/cookie_monster.cc:2059: // make it to the secure cookies.
This comment, and especially the second sentence, is a little hard to follow.
Would you mind taking another pass?

[mmenke, 2017-06-07 15:32:54]

https://codereview.chromium.org/2924933002/diff/1/net/cookies/cookie_monster.cc
File net/cookies/cookie_monster.cc (right):

https://codereview.chromium.org/2924933002/diff/1/net/cookies/cookie_monster....
net/cookies/cookie_monster.cc:2059: // make it to the secure cookies.
On 2017/06/07 06:55:47, Mike West wrote:
> This comment, and especially the second sentence, is a little hard to follow.
> Would you mind taking another pass?

I've tried to improve it, but it's a fairly awkward thing to express (Due to the
code itself being pretty awkward).

[mmenke, 2017-06-07 15:34:01]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2017-06-07 15:34:02]

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/2924933002/#ps40001
(title: "Oops")

[commit-bot: I haz the power, 2017-06-07 15:34:23]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Mike West, 2017-06-07 15:35:30]

On 2017/06/07 at 15:32:54, mmenke wrote:
>
https://codereview.chromium.org/2924933002/diff/1/net/cookies/cookie_monster.cc
> File net/cookies/cookie_monster.cc (right):
> 
>
https://codereview.chromium.org/2924933002/diff/1/net/cookies/cookie_monster....
> net/cookies/cookie_monster.cc:2059: // make it to the secure cookies.
> On 2017/06/07 06:55:47, Mike West wrote:
> > This comment, and especially the second sentence, is a little hard to
follow.
> > Would you mind taking another pass?
> 
> I've tried to improve it, but it's a fairly awkward thing to express (Due to
the code itself being pretty awkward).

I think it's a big improvement. Thanks! Still LGTM.

This might be worth merging back to M60. Would you like me to take care of that
after a cycle through Canary?

[mmenke, 2017-06-07 15:42:57]

On 2017/06/07 15:35:30, Mike West wrote:
> On 2017/06/07 at 15:32:54, mmenke wrote:
> >
>
https://codereview.chromium.org/2924933002/diff/1/net/cookies/cookie_monster.cc
> > File net/cookies/cookie_monster.cc (right):
> > 
> >
>
https://codereview.chromium.org/2924933002/diff/1/net/cookies/cookie_monster....
> > net/cookies/cookie_monster.cc:2059: // make it to the secure cookies.
> > On 2017/06/07 06:55:47, Mike West wrote:
> > > This comment, and especially the second sentence, is a little hard to
> follow.
> > > Would you mind taking another pass?
> > 
> > I've tried to improve it, but it's a fairly awkward thing to express (Due to
> the code itself being pretty awkward).
> 
> I think it's a big improvement. Thanks! Still LGTM.
> 
> This might be worth merging back to M60. Would you like me to take care of
that
> after a cycle through Canary?

I'm a bit nervous about merging without more testing, mostly because of the
|earliest_access_time_| changes, and general lack of testing (We have no test
where we have secure cookies and GC them all, for instance).  I feel we should
add more tests around GC, and maybe some DCHECKs around earliest_access_time_,
but I'm unlikely to get to it.  I also think this bug is obscure enough and
non-exploitable enough that it's probably not worth merging the fix,
particularly given how long this has been broken.

[commit-bot: I haz the power, 2017-06-07 17:14:04]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496849641456790,
"parent_rev": "5469a6fb610e9be954eaca04c0dae0f3eb2ae42b", "commit_rev":
"f4721d99f41f62c860b20f1cd53911bdd069d338"}

[commit-bot: I haz the power, 2017-06-07 17:14:17]

Description was changed from

==========
Fix CookieMonster garbage collection when no insecure cookies.

The code wanted to always keep at least one insecure cookie
around, so it could get the earliest access time of that cookie to
update earliest_access_time_, but when there weren't any such cookies,
this broke rather badly.

That logic for calculating the earliest_access_time_ was also wrong,
since it was set either only based on secure cookies, or only based
on insecure cookies.

This CL fixes both those issues.

BUG=730000
==========

to

==========
Fix CookieMonster garbage collection when no insecure cookies.

The code wanted to always keep at least one insecure cookie
around, so it could get the earliest access time of that cookie to
update earliest_access_time_, but when there weren't any such cookies,
this broke rather badly.

That logic for calculating the earliest_access_time_ was also wrong,
since it was set either only based on secure cookies, or only based
on insecure cookies.

This CL fixes both those issues.

BUG=730000

Review-Url: https://codereview.chromium.org/2924933002
Cr-Commit-Position: refs/heads/master@{#477689}
Committed:
https://chromium.googlesource.com/chromium/src/+/f4721d99f41f62c860b20f1cd539...
==========

[commit-bot: I haz the power, 2017-06-07 17:14:19]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/f4721d99f41f62c860b20f1cd539...