Fixes targeting the unique creation times invariant in the CookieMonster:

Fixes targeting the unique creation times invariant in the CookieMonster:
	* Make sure we don't import cookies with identical creation times.
	* DCHECK that duplicate cookie list insertion succeeds (it
	  silently didn't when there were not unique creation times.)
	* Confirm that we eliminate all the duplicats we find on cookie
	  import.
	* Make Methods allowing setting of creation time private.
	* Create performance test for import (involved refactoring
	  backing store mock.)

This change does increase the performance cost on import, and hence adds
to startup time.  However, the increase for importing 15000 cookies was only 
5 ms, so I think that's acceptable to prevent crashes.


rdsmith-macbookpro:~/tmp $ perfparse base_perf_import.txt new_perf_import.txt 
                                     base_perf_import.txt   new_perf_import.txt
CookieMonsterTest.TestImport 
  Cookie_monster_import_from_store   26.36 +/- 0.88         31.38 +/- 1.1

BUG=43188
TEST=net_unittest --gtest_filter=CookieMonsterTest.* (including two new tests.), Linux & Windows

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=53957

[Randy Smith (Not in Mondays), 2010-07-26 18:10:53]

Eric, Andy: Could you take a look at this?  I think I've finally found root
(enough) cause for 43188; it will be nice to put that crash to bed.

[eroman, 2010-07-26 18:53:55]

LGTM.

do we have a separate issue for how the sqlite DB can get wedged like this?

http://codereview.chromium.org/3070001/diff/2001/3001
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/3070001/diff/2001/3001#newcode197
net/base/cookie_monster.cc:197: if (creation_times.find(cookie_creation_time) ==
creation_times.end()) {
I reckon it would be faster to cut out the find(), and instead always call
insert() and test the ret_value.second.

http://codereview.chromium.org/3070001/diff/2001/3001#newcode201
net/base/cookie_monster.cc:201: LOG(ERROR) << StringPrintf("Found cookies with
duplicate creation "
optional: I wander if we should update the histogram
"Net.NumDuplicateCookiesInDb" or track with another histogram.

http://codereview.chromium.org/3070001/diff/2001/3001#newcode312
net/base/cookie_monster.cc:312: DCHECK(insert_success);        // Fails with
duplicate creation time
instead of a comment, consider adding it as a log comment.

DCHECK(insert_success) << "blah blah blah";

http://codereview.chromium.org/3070001/diff/2001/3002
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/3070001/diff/2001/3002#newcode158
net/base/cookie_monster.h:158: // Testing support
nit: please end comments with period.

http://codereview.chromium.org/3070001/diff/2001/3002#newcode219
net/base/cookie_monster.h:219: // Helper function that sets cookies with more
control
nit: please end in period.

http://codereview.chromium.org/3070001/diff/2001/3003
File net/base/cookie_monster_perftest.cc (right):

http://codereview.chromium.org/3070001/diff/2001/3003#newcode11
net/base/cookie_monster_perftest.cc:11: #include
"net/base/cookie_monster_store_test.h"
please sort it with the stuff above.

In fact, you could isolate "cookie_monster.h" at the top, and then have
everything else underneath sorted lexicographically.

http://codereview.chromium.org/3070001/diff/2001/3003#newcode173
net/base/cookie_monster_perftest.cc:173: for (int domain_num=0; domain_num <
300; domain_num++) {
nit; spaces around binary operators like '='.

http://codereview.chromium.org/3070001/diff/2001/3003#newcode176
net/base/cookie_monster_perftest.cc:176: for (int cookie_num=0; cookie_num < 50;
cookie_num++) {
ditto.

http://codereview.chromium.org/3070001/diff/2001/3004
File net/base/cookie_monster_store_test.h (right):

http://codereview.chromium.org/3070001/diff/2001/3004#newcode1
net/base/cookie_monster_store_test.h:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
2010

http://codereview.chromium.org/3070001/diff/2001/3004#newcode6
net/base/cookie_monster_store_test.h:6: // (current cookie_mosnter_unittest.cc
and cookie_monster_perftest.cc)
mostner --> monster

http://codereview.chromium.org/3070001/diff/2001/3004#newcode35
net/base/cookie_monster_store_test.h:35: class MockPersistentCookieStore
May want to note that this implementation is not threadsafe.
(I forget, is PersistentCookieStore supposed to be threadsafe?)

http://codereview.chromium.org/3070001/diff/2001/3005
File net/base/cookie_monster_unittest.cc (right):

http://codereview.chromium.org/3070001/diff/2001/3005#newcode19
net/base/cookie_monster_unittest.cc:19: #include
"net/base/cookie_monster_store_test.h" // For CookieStore Mock
please merge into sort list above.

http://codereview.chromium.org/3070001/diff/2001/3005#newcode1445
net/base/cookie_monster_unittest.cc:1445: Time earlier(now +
TimeDelta::FromDays(-1));
nit: FromDays(-1) reads funny to me, I suggest writing this as:

now - TimeDelta::FromDays(1)

http://codereview.chromium.org/3070001/diff/2001/3005#newcode1447
net/base/cookie_monster_unittest.cc:1447: // to simulate a database with 4
duplicates.
nit: seems like something is missing in this phrase.

http://codereview.chromium.org/3070001/diff/2001/3005#newcode1498
net/base/cookie_monster_unittest.cc:1498: EXPECT_TRUE(name1 == "X" || name2 ==
"X");
nit: you may also want to check that name1 != name2.

http://codereview.chromium.org/3070001/diff/2001/3005#newcode1762
net/base/cookie_monster_unittest.cc:1762: ".google.com", "/", Time(), false,
false);
nit: i would suggest using a non-null Time() value, in case 0 time is special
cased.

[ahendrickson, 2010-07-27 19:15:37]

LGTM.

http://codereview.chromium.org/3070001/diff/17001/18001
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/3070001/diff/17001/18001#newcode1061
net/base/cookie_monster.cc:1061: AutoLock autolock(lock_);
I'm curious about why you moved this section (and in
SetCookieWithCreationTime()) out from SetCookieWithCreationTimeAndOptions().

[Randy Smith (Not in Mondays), 2010-07-27 20:11:47]

Thanks!  (Publishing just to respond to comments; will be committing now.)

http://codereview.chromium.org/3070001/diff/2001/3001
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/3070001/diff/2001/3001#newcode197
net/base/cookie_monster.cc:197: if (creation_times.find(cookie_creation_time) ==
creation_times.end()) {
On 2010/07/26 18:53:55, eroman wrote:
> I reckon it would be faster to cut out the find(), and instead always call
> insert() and test the ret_value.second.

Good point; done.

http://codereview.chromium.org/3070001/diff/2001/3001#newcode201
net/base/cookie_monster.cc:201: LOG(ERROR) << StringPrintf("Found cookies with
duplicate creation "
On 2010/07/26 18:53:55, eroman wrote:
> optional: I wander if we should update the histogram
> "Net.NumDuplicateCookiesInDb" or track with another histogram.

Hmmm.  I'm inclined to defer this.  If we find duplicate creation_times it means
a corrupt database or a bug in our code *and* a SQLite database bug (since
there's a unique constraint on the table that should prevent us inserting
duplicate creation times--I need to try this and find out if it really works.) 
We do want to track database corruption, but I think we care more about
instances of database corruption than we do about how many glitches we get out
of a corrupt database.  And as you point out, we need to think about taking this
stuff a step further somehow.  So we can think about statistics in that context
:-}.

http://codereview.chromium.org/3070001/diff/2001/3001#newcode312
net/base/cookie_monster.cc:312: DCHECK(insert_success);        // Fails with
duplicate creation time
On 2010/07/26 18:53:55, eroman wrote:
> instead of a comment, consider adding it as a log comment.
> 
> DCHECK(insert_success) << "blah blah blah";

Done.

http://codereview.chromium.org/3070001/diff/2001/3002
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/3070001/diff/2001/3002#newcode158
net/base/cookie_monster.h:158: // Testing support
On 2010/07/26 18:53:55, eroman wrote:
> nit: please end comments with period.

Done.  Also done: Moved "Ending comments with period" to the top of my "Usual
coding mistakes" list :-J.

http://codereview.chromium.org/3070001/diff/2001/3002#newcode219
net/base/cookie_monster.h:219: // Helper function that sets cookies with more
control
On 2010/07/26 18:53:55, eroman wrote:
> nit: please end in period.

Done.

http://codereview.chromium.org/3070001/diff/2001/3003
File net/base/cookie_monster_perftest.cc (right):

http://codereview.chromium.org/3070001/diff/2001/3003#newcode11
net/base/cookie_monster_perftest.cc:11: #include
"net/base/cookie_monster_store_test.h"
On 2010/07/26 18:53:55, eroman wrote:
> please sort it with the stuff above.
> 
> In fact, you could isolate "cookie_monster.h" at the top, and then have
> everything else underneath sorted lexicographically.

Done.

http://codereview.chromium.org/3070001/diff/2001/3003#newcode173
net/base/cookie_monster_perftest.cc:173: for (int domain_num=0; domain_num <
300; domain_num++) {
On 2010/07/26 18:53:55, eroman wrote:
> nit; spaces around binary operators like '='.

Done.

http://codereview.chromium.org/3070001/diff/2001/3003#newcode176
net/base/cookie_monster_perftest.cc:176: for (int cookie_num=0; cookie_num < 50;
cookie_num++) {
On 2010/07/26 18:53:55, eroman wrote:
> ditto.

Done.

http://codereview.chromium.org/3070001/diff/2001/3004
File net/base/cookie_monster_store_test.h (right):

http://codereview.chromium.org/3070001/diff/2001/3004#newcode1
net/base/cookie_monster_store_test.h:1: // Copyright (c) 2006-2008 The Chromium
Authors. All rights reserved.
On 2010/07/26 18:53:55, eroman wrote:
> 2010

Done.

http://codereview.chromium.org/3070001/diff/2001/3004#newcode6
net/base/cookie_monster_store_test.h:6: // (current cookie_mosnter_unittest.cc
and cookie_monster_perftest.cc)
On 2010/07/26 18:53:55, eroman wrote:
> mostner --> monster

Done.

http://codereview.chromium.org/3070001/diff/2001/3004#newcode35
net/base/cookie_monster_store_test.h:35: class MockPersistentCookieStore
On 2010/07/26 18:53:55, eroman wrote:
> May want to note that this implementation is not threadsafe.
> (I forget, is PersistentCookieStore supposed to be threadsafe?)

That's a fascinating question.  It isn't specified in cookie_monster.h in any
comment, but CookieMonster::PersistentCookieStore inherits from
RefCountedThreadSafe<>, but the implementation of SQLitePersistentCookieStore
doesn't do any locking, but (I think) nothing but the cookie monster accesses it
so in practice it's protected by the cookie monster lock.  This ties into a
another I have, in that I feel like the interface contracts for the
CookieMonster API aren't well specified in several places.  

But I think I'll settle on answering your question by saying: It looks as thread
safe to me as SQLitePersistentCookieStore is, so I don't think there's a need to
comment it further.  At least until the basic question is clarified.

http://codereview.chromium.org/3070001/diff/2001/3005
File net/base/cookie_monster_unittest.cc (right):

http://codereview.chromium.org/3070001/diff/2001/3005#newcode19
net/base/cookie_monster_unittest.cc:19: #include
"net/base/cookie_monster_store_test.h" // For CookieStore Mock
On 2010/07/26 18:53:55, eroman wrote:
> please merge into sort list above.

Done.

http://codereview.chromium.org/3070001/diff/2001/3005#newcode1445
net/base/cookie_monster_unittest.cc:1445: Time earlier(now +
TimeDelta::FromDays(-1));
On 2010/07/26 18:53:55, eroman wrote:
> nit: FromDays(-1) reads funny to me, I suggest writing this as:
> 
> now - TimeDelta::FromDays(1)

Done.

http://codereview.chromium.org/3070001/diff/2001/3005#newcode1447
net/base/cookie_monster_unittest.cc:1447: // to simulate a database with 4
duplicates.
On 2010/07/26 18:53:55, eroman wrote:
> nit: seems like something is missing in this phrase.

Yeah, I think I accidentally either cut or pasted the wrong line.  I think the
function's fine without the comment; removing it.

http://codereview.chromium.org/3070001/diff/2001/3005#newcode1498
net/base/cookie_monster_unittest.cc:1498: EXPECT_TRUE(name1 == "X" || name2 ==
"X");
On 2010/07/26 18:53:55, eroman wrote:
> nit: you may also want to check that name1 != name2.

Not a nit; that's an important conceptual test.  Ooops.  Done.

http://codereview.chromium.org/3070001/diff/2001/3005#newcode1762
net/base/cookie_monster_unittest.cc:1762: ".google.com", "/", Time(), false,
false);
On 2010/07/26 18:53:55, eroman wrote:
> nit: i would suggest using a non-null Time() value, in case 0 time is special
> cased.

I'm not clear I follow.  0 is a special case; in this case it means "Does not
expire" (this CL removes the option for external code setting creation time
except for two specific tests.)  I'm perfectly happy with a cookie that doesn't
expire in this case.  Have I missed the point you were making?

http://codereview.chromium.org/3070001/diff/17001/18001
File net/base/cookie_monster.cc (right):

http://codereview.chromium.org/3070001/diff/17001/18001#newcode1061
net/base/cookie_monster.cc:1061: AutoLock autolock(lock_);
On 2010/07/27 19:15:37, ahendrickson wrote:
> I'm curious about why you moved this section (and in
> SetCookieWithCreationTime()) out from SetCookieWithCreationTimeAndOptions().

Historically, all of the functions that took the CookieMonster lock have been
public functions, and I was trying to preserve that.  But the it's not like
there's anything sacred about it; it just makes the locking structure slightly
easier to understand.  And in fact, I did break the invariant, as
SetCookieWithCreationTime() is now a private function, but it's still takes the
lock, as it's called from the test functions.  I figured it was more important
to take away the possibility for CookieMonster consumers to cause a nasty bug.

>

[eroman, 2010-07-28 00:02:23]

lgtm