This CL changes the CookieStore obect to be a refcounted object to get a bett...

This CL changes the CookieStore obect to be a refcounted object to get a better handle on its lifetime as there are cases where this object is handed out to URLRequestContext instances which outlive the URLRequestContext object which created it.

Partial fix for http://code.google.com/p/chromium/issues/detail?id=15289

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

[ananta, 2009-09-04 06:14:19]

On 2009/09/04 05:29:14, ananta wrote:
> 

Please hold off on the review for a bit. Some more changes are needed to get
this to work. I will be uploading those hopefully soon.

-Ananta

[ananta, 2009-09-04 07:48:24]

On 2009/09/04 06:14:19, ananta wrote:
> On 2009/09/04 05:29:14, ananta wrote:
> > 
> 
> Please hold off on the review for a bit. Some more changes are needed to get
> this to work. I will be uploading those hopefully soon.
> 
> -Ananta

Ready for another look.

Thanks
Ananta

[eroman, 2009-09-04 22:38:57]

http://codereview.chromium.org/197023/diff/5013/5024
File chrome/browser/automation/automation_profile_impl.cc (right):

http://codereview.chromium.org/197023/diff/5013/5024#newcode103
Line 103: net::CookieStore* original_cookie_store_;
Shouldn't this be a scoped_refptr?

http://codereview.chromium.org/197023/diff/5013/5022
File chrome/browser/gtk/options/cookies_view_unittest.cc (right):

http://codereview.chromium.org/197023/diff/5013/5022#newcode25
Line 25: virtual ~TestURLRequestContext() {
Can delete this all together

http://codereview.chromium.org/197023/diff/5013/5018
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/197023/diff/5013/5018#newcode550
Line 550: cookie_store_ = NULL;
Why is this necessary? The PersistentCookieStore dependency is now refcounted,
so it shouldn't matter.

http://codereview.chromium.org/197023/diff/5013/5021
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/197023/diff/5013/5021#newcode130
Line 130: scoped_refptr<SQLitePersistentCookieStore> cookie_db_;
Can this variable simply be deleted?

It seems the only reason cookie_db was a member before is to keep the database
alive for cookie_store's lifetime. But now that the dependency is refcounted,
that is not necessary.

http://codereview.chromium.org/197023/diff/5013/5019
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/197023/diff/5013/5019#newcode371
Line 371: : public base::RefCounted<CookieMonster::PersistentCookieStore> {
Does this need to be refcounted threadsafe as well?

http://codereview.chromium.org/197023/diff/5013/5017
File net/base/cookie_store.h (right):

http://codereview.chromium.org/197023/diff/5013/5017#newcode24
Line 24: // be therad safe as its methods can be accessed from IO as well as UI
threads.
Can you correct the pre-existing typeo "therad" safe?

Also, I reckon CookieStore needs to be RefCountedThreadSafe, since it may be
AddRefed from both UI or IO thread.

[ananta, 2009-09-04 23:08:56]

http://codereview.chromium.org/197023/diff/5013/5024
File chrome/browser/automation/automation_profile_impl.cc (right):

http://codereview.chromium.org/197023/diff/5013/5024#newcode103
Line 103: net::CookieStore* original_cookie_store_;
On 2009/09/04 22:38:59, eroman wrote:
> Shouldn't this be a scoped_refptr?

Done.

http://codereview.chromium.org/197023/diff/5013/5022
File chrome/browser/gtk/options/cookies_view_unittest.cc (right):

http://codereview.chromium.org/197023/diff/5013/5022#newcode25
Line 25: virtual ~TestURLRequestContext() {
On 2009/09/04 22:38:59, eroman wrote:
> Can delete this all together

Done.

http://codereview.chromium.org/197023/diff/5013/5018
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/197023/diff/5013/5018#newcode550
Line 550: cookie_store_ = NULL;
On 2009/09/04 22:38:59, eroman wrote:
> Why is this necessary? The PersistentCookieStore dependency is now refcounted,
> so it shouldn't matter.

Done.

http://codereview.chromium.org/197023/diff/5013/5021
File chrome/browser/net/chrome_url_request_context.h (right):

http://codereview.chromium.org/197023/diff/5013/5021#newcode130
Line 130: scoped_refptr<SQLitePersistentCookieStore> cookie_db_;
On 2009/09/04 22:38:59, eroman wrote:
> Can this variable simply be deleted?
> 
> It seems the only reason cookie_db was a member before is to keep the database
> alive for cookie_store's lifetime. But now that the dependency is refcounted,
> that is not necessary.

Done.

http://codereview.chromium.org/197023/diff/5013/5019
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/197023/diff/5013/5019#newcode371
Line 371: : public base::RefCounted<CookieMonster::PersistentCookieStore> {
On 2009/09/04 22:38:59, eroman wrote:
> Does this need to be refcounted threadsafe as well?

Done.

http://codereview.chromium.org/197023/diff/5013/5017
File net/base/cookie_store.h (right):

http://codereview.chromium.org/197023/diff/5013/5017#newcode24
Line 24: // be therad safe as its methods can be accessed from IO as well as UI
threads.
On 2009/09/04 22:38:59, eroman wrote:
> Can you correct the pre-existing typeo "therad" safe?
> 
> Also, I reckon CookieStore needs to be RefCountedThreadSafe, since it may be
> AddRefed from both UI or IO thread.

Done.

[eroman, 2009-09-04 23:13:24]

lgtm!

http://codereview.chromium.org/197023/diff/4010/3021
File net/base/cookie_monster.h (right):

http://codereview.chromium.org/197023/diff/4010/3021#newcode370
Line 370: typedef
base::RefCountedThreadSafe<CookieMonster::PersistentCookieStore>
hehe.