CookieStore: Remove reference counting.

CookieStore: Remove reference counting.

This has caused a number of issues in the past, and having
defined lifecycles for objects makes code easier to reason
about and maintain.

BUG=472744
TBR=alexclarke@chromium.org, creis@chromium.org

Committed: https://crrev.com/606c59ca3b9bba3f4a7b1206cb6b1b3c0a8e2c9b
Cr-Commit-Position: refs/heads/master@{#379588}

[mmenke, 2016-02-16 18:09:10]

Description was changed from

==========
URLRequestHttpJob: No longer wait for setting a cookie to complete.

The new code is simpler, and results in fewer roundtrips if the
CookieStore often completes tasks asynchronously.

BUG=587127
==========

to

==========

==========

[mmenke, 2016-02-29 19:47:32]

Patchset #9 (id:160001) has been deleted

[mmenke, 2016-02-29 19:59:22]

Patchset #10 (id:200001) has been deleted

[mmenke, 2016-02-29 20:13:30]

Patchset #8 (id:140001) has been deleted

[mmenke, 2016-02-29 20:13:49]

Patchset #3 (id:40001) has been deleted

[mmenke, 2016-02-29 20:13:54]

Patchset #4 (id:80001) has been deleted

[mmenke, 2016-02-29 20:13:59]

Patchset #4 (id:100001) has been deleted

[mmenke, 2016-02-29 20:30:11]

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

[mmenke, 2016-02-29 20:30:12]

Randy:  Mind reviewing?  I've sent my other cookie store CLs to Mike, but I'm
not really sure about this one to him.  It's more about URLRequestContext
construction and ownership.  Know you aren't an expert in this area, but we
don't really have any expert here, other than me.

Note that this is on top of a CL that makes the CookieStore non-threadsafe.

Focus on net/, content/ and chrome/ files.  I'll have domain experts review the
others (Particularly iOS and WebView).

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

Sure, no problem; this way I can take some vicarious enjoyment in watching it go
away :-}.  

Did you do any archaeology as to why it's been reference counted in the past? 
That would help in evaluating the current day removal of reference counting.

[mmenke, 2016-02-29 21:34:54]

On 2016/02/29 21:11:46, Randy Smith - Not in Fridays wrote:
> Sure, no problem; this way I can take some vicarious enjoyment in watching it
go
> away :-}.  
> 
> Did you do any archaeology as to why it's been reference counted in the past? 
> That would help in evaluating the current day removal of reference counting.

https://bugs.chromium.org/p/chromium/issues/detail?id=15289

Basically, everything else in net/ that talked to it was refcounted, so refcount
all the things!

Out new model is to delete stuff and pray no one that has borrowed bits of the
main URLRequestContext for their own nefarious ends, or still has an active
URLRequest out.

I have also done some auditing of code that accesses cookie store, beyond the
DCHECKs added in the other CL and auditing CookieStore owners in this CL.

[Randy Smith (Not in Mondays), 2016-02-29 22:37:46]

I've done an initial review of non-test code in net/, content/, and chrome/, and
it looks good.  I want to do a review of the test code, and also at least look
at all the call sites, but if you wanted to overlap with pulling in call site
owners now, I think that would be safe--the bones of this CL are good.

https://codereview.chromium.org/1701063002/diff/180001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl_io_data.cc (left):

https://codereview.chromium.org/1701063002/diff/180001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:490: if (!cookie_store.get()) {
I know the code's going away, but: Huh?  Won't this always be true?

https://codereview.chromium.org/1701063002/diff/220001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1701063002/diff/220001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:576:
set_cookie_store(cookie_store_.get());
Not part of this CL, but I think it's dangerously confusing that the base
URLRequestContext has "pointer but no ownership" semantics for the things in it,
and this derived class has ownership semantics.   Dunno whether that's on your
radar screen (or should be), but I'll mention my aversive reaction in case it
has any data in it.

https://codereview.chromium.org/1701063002/diff/220001/chrome/browser/session...
File chrome/browser/sessions/session_data_deleter.cc (right):

https://codereview.chromium.org/1701063002/diff/220001/chrome/browser/session...
chrome/browser/sessions/session_data_deleter.cc:138: // If these callbacks are
invoked, |cookie_store| is gauranteed to still
nit: "guaranteed".

https://codereview.chromium.org/1701063002/diff/220001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.cc (right):

https://codereview.chromium.org/1701063002/diff/220001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:188: return
make_scoped_ptr(cookie_monster.release());
nit: Is there something I'm not understanding about scoped_ptr<> semantics?  I
thought if the scoped pointer was going out of scope at the point of transfer,
no explicit transfer was needed, i.e. "return cookie_monster;" should work here.
 Even if it doesn't, I'd think "return std::move(cookie_monster);" would be more
idiomatic in the new chrome world.  Am I missing something?

https://codereview.chromium.org/1701063002/diff/220001/net/cookies/cookie_mon...
File net/cookies/cookie_monster.h (right):

https://codereview.chromium.org/1701063002/diff/220001/net/cookies/cookie_mon...
net/cookies/cookie_monster.h:682: scoped_refptr<PersistentCookieStore> store_;
Is there a bug somewhere to make this scoped_ptr<> as well?  I think the pointer
here is owning, and that things outside of this class don't generally access the
PersistentCookieStore.

[mmenke, 2016-02-29 23:00:29]

https://codereview.chromium.org/1701063002/diff/180001/chrome/browser/profile...
File chrome/browser/profiles/profile_impl_io_data.cc (left):

https://codereview.chromium.org/1701063002/diff/180001/chrome/browser/profile...
chrome/browser/profiles/profile_impl_io_data.cc:490: if (!cookie_store.get()) {
On 2016/02/29 22:37:45, Randy Smith - Not in Fridays wrote:
> I know the code's going away, but: Huh?  Won't this always be true?

Yes, which is exactly why I removed it.  :)  I suspect it was copied from the
code to create a cookie store below, which has a similar check (Which I also
remove in this CL), which can actually be false.

https://codereview.chromium.org/1701063002/diff/220001/chrome/browser/profile...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/1701063002/diff/220001/chrome/browser/profile...
chrome/browser/profiles/profile_io_data.cc:576:
set_cookie_store(cookie_store_.get());
On 2016/02/29 22:37:45, Randy Smith - Not in Fridays wrote:
> Not part of this CL, but I think it's dangerously confusing that the base
> URLRequestContext has "pointer but no ownership" semantics for the things in
it,
> and this derived class has ownership semantics.   Dunno whether that's on your
> radar screen (or should be), but I'll mention my aversive reaction in case it
> has any data in it.

I'd really like to move to a world where all URLRequestContexts have ownership
semantics.  Sharing guts has gotten us into all sorts of trouble.  Sharing stuff
is actually the exception, rather than the rule.  The main request context
doesn't own its stuff, but all other request contexts (Except those that share
pars of the main one) own everything.  Suppose the system and proxy request
contexts also share components.

In this particular case, there's a bug on file about giving apps their own
separate URLRequestContexts, since sharing allows certain sidechannel attacks
(Don't know the specifics, beyond things like DNS timing attacks).

https://codereview.chromium.org/1701063002/diff/220001/chrome/browser/session...
File chrome/browser/sessions/session_data_deleter.cc (right):

https://codereview.chromium.org/1701063002/diff/220001/chrome/browser/session...
chrome/browser/sessions/session_data_deleter.cc:138: // If these callbacks are
invoked, |cookie_store| is gauranteed to still
On 2016/02/29 22:37:45, Randy Smith - Not in Fridays wrote:
> nit: "guaranteed".

Done.

https://codereview.chromium.org/1701063002/diff/220001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.cc (right):

https://codereview.chromium.org/1701063002/diff/220001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:188: return
make_scoped_ptr(cookie_monster.release());
On 2016/02/29 22:37:45, Randy Smith - Not in Fridays wrote:
> nit: Is there something I'm not understanding about scoped_ptr<> semantics?  I
> thought if the scoped pointer was going out of scope at the point of transfer,
> no explicit transfer was needed, i.e. "return cookie_monster;" should work
here.
>  Even if it doesn't, I'd think "return std::move(cookie_monster);" would be
more
> idiomatic in the new chrome world.  Am I missing something?

cookie_monster is a scoped_ptr<CookieMonster>, while we're returning a
scoped_ptr<CookieStore>.  "return cookie_monster" doesn't handle the conversion.
 std::move does, but I think it generates a clang error.  I'll try it, and see
what the bots think.

https://codereview.chromium.org/1701063002/diff/220001/net/cookies/cookie_mon...
File net/cookies/cookie_monster.h (right):

https://codereview.chromium.org/1701063002/diff/220001/net/cookies/cookie_mon...
net/cookies/cookie_monster.h:682: scoped_refptr<PersistentCookieStore> store_;
On 2016/02/29 22:37:45, Randy Smith - Not in Fridays wrote:
> Is there a bug somewhere to make this scoped_ptr<> as well?  I think the
pointer
> here is owning, and that things outside of this class don't generally access
the
> PersistentCookieStore.

I don't think there's much value in making it non-refcounted - it's basically a
private object that writes stuff on its own thread.  To shut it down, we'd have
to either block on it finishing that, or post a task to the other thread to
delete it when we're done with it.

I wouldn't oppose the effort to do this, and don't think it would be too hard,
just don't think there are too many gains there.

[mmenke, 2016-03-01 01:40:47]

https://codereview.chromium.org/1701063002/diff/220001/content/browser/net/qu...
File content/browser/net/quota_policy_cookie_store.cc (right):

https://codereview.chromium.org/1701063002/diff/220001/content/browser/net/qu...
content/browser/net/quota_policy_cookie_store.cc:188: return
make_scoped_ptr(cookie_monster.release());
On 2016/02/29 23:00:29, mmenke wrote:
> On 2016/02/29 22:37:45, Randy Smith - Not in Fridays wrote:
> > nit: Is there something I'm not understanding about scoped_ptr<> semantics? 
I
> > thought if the scoped pointer was going out of scope at the point of
transfer,
> > no explicit transfer was needed, i.e. "return cookie_monster;" should work
> here.
> >  Even if it doesn't, I'd think "return std::move(cookie_monster);" would be
> more
> > idiomatic in the new chrome world.  Am I missing something?
> 
> cookie_monster is a scoped_ptr<CookieMonster>, while we're returning a
> scoped_ptr<CookieStore>.  "return cookie_monster" doesn't handle the
conversion.
>  std::move does, but I think it generates a clang error.  I'll try it, and see
> what the bots think.

Looks like clang is smart enough to know when std::move is converting between
types and when it's not.  I'll fix all instances of this and upload a new patch
tomorrow.

[Randy Smith (Not in Mondays), 2016-03-01 20:48:43]

Scanned all call sites, and all test code in content/, chrome/ and net/.  Still
looking basically good, though there's a general issue I raise about lifetimes
below for which I'm not sure what the right resolution is.  Abstract summary: In
cases when a class inherits from URLRequestContext or inherits from another
class that includes a URLRequestContext, and the derived class owns the
CookieStore, we're changing lifetimes, in that during destruction the
CookieStore will be destructed with the derived class, as opposed to waiting
until the base class URLRequestContext releases its reference.  In non-test code
I think this is ok (or, as I say, "ok") because we already have the pattern for
other classes besides CookieMonster/Store and presumably we can rely on the
URLRequestContext destructor to not do anything ugly.  But for test classes,
we're sometimes relying on an arbitrary class constructor to not do anything
with the URLRequestContext that would involve touching the already deleted
CookieStore, and that makes me more uncomfortable (for all that I don't think
any of them currently do).  Let me know what you think.

https://codereview.chromium.org/1701063002/diff/280001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/1701063002/diff/280001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:422:
SetMonster(std::move(monster));
You look to be changing the deletion semantics of the cookie monster here; IIUC,
originally it leaked.  I don't offhand see why we'd expect
RemoveSafeBrowsingCookieTester to outlive the URLRequestContext off the
safebrowsing service.  Maybe save the original pointer and put it back in the
destructor?  Or at least set it to null in the destructor, to make it more
obvious if we trip over a lifetime issue?

https://codereview.chromium.org/1701063002/diff/280001/chrome/test/base/testi...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/1701063002/diff/280001/chrome/test/base/testi...
chrome/test/base/testing_profile.cc:167: scoped_ptr<net::CookieStore>
cookie_store_;
This (anti) pattern of a derived class actually owning the object the base class
has a pointer to does raise a potential failure mode during destruction, because
the derived class is destructed before the base class, so the base class will
have a raw pointer to freed memory.  I presume it's ok ("ok" :-}) since we have
this pattern elsewhere for things other than cookies so presumably the
URLRequestContext destructor can't do anything ugly, but I wanted to raise the
flag that this shift is actually going in the wrong direction for safe
destruction ordering.

https://codereview.chromium.org/1701063002/diff/280001/headless/lib/browser/h...
File headless/lib/browser/headless_url_request_context_getter.cc (right):

https://codereview.chromium.org/1701063002/diff/280001/headless/lib/browser/h...
headless/lib/browser/headless_url_request_context_getter.cc:15: #include
"net/cookies/cookie_store.h"
Hmmm.  As best I can tell, this type is only used implicitly, in the argument to
one function and the return value from another.  I'd think that both of those
situations would be better handled by the include files that declare those
functions, so my inclination would be that they wouldn't provoke an IWYU
inclusion.  But I may be misunderstanding how IWYU should be implemented.

https://codereview.chromium.org/1701063002/diff/280001/ios/chrome/browser/bro...
File
ios/chrome/browser/browser_state/off_the_record_chrome_browser_state_io_data.mm
(right):

https://codereview.chromium.org/1701063002/diff/280001/ios/chrome/browser/bro...
ios/chrome/browser/browser_state/off_the_record_chrome_browser_state_io_data.mm:203:
cookie_util::CookieStoreConfig::RESTORED_SESSION_COOKIES,
Not a problem for this CL, but do you have any idea why we're using a
cookie_path_ and a RESTORED_SESSION_COOKIES for an OTR cookie context?

https://codereview.chromium.org/1701063002/diff/280001/net/url_request/url_re...
File net/url_request/url_request_http_job_unittest.cc (right):

https://codereview.chromium.org/1701063002/diff/280001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:548:
context_.set_cookie_store(&cookie_monster);
Same issue mentioned elsewhere about a context raw pointer outliving the object
pointed to (by a little bit).  I'm inclined to suggest that you set the cookie
store to null (or save the pointer and reset it) in this routine, since I think
of the destructor of the URLRequestContext likely being more stable than the
destructor of a random test class.

[mmenke, 2016-03-01 22:44:45]

Thanks for the review, and calling out the ugliness around ownership - I agree
we want to do better there, longer term (Patches welcome!)

https://codereview.chromium.org/1701063002/diff/280001/chrome/browser/browsin...
File chrome/browser/browsing_data/browsing_data_remover_unittest.cc (right):

https://codereview.chromium.org/1701063002/diff/280001/chrome/browser/browsin...
chrome/browser/browsing_data/browsing_data_remover_unittest.cc:422:
SetMonster(std::move(monster));
On 2016/03/01 20:48:43, Randy Smith - Not in Fridays wrote:
> You look to be changing the deletion semantics of the cookie monster here;
IIUC,
> originally it leaked.  I don't offhand see why we'd expect
> RemoveSafeBrowsingCookieTester to outlive the URLRequestContext off the
> safebrowsing service.  Maybe save the original pointer and put it back in the
> destructor?  Or at least set it to null in the destructor, to make it more
> obvious if we trip over a lifetime issue?

Hrm...Actually, I'm not sure we even need to create a cookie store here. 
Safebrowsing already makes its own store.  I'll try removing it, and see what
happens.

https://codereview.chromium.org/1701063002/diff/280001/chrome/test/base/testi...
File chrome/test/base/testing_profile.cc (right):

https://codereview.chromium.org/1701063002/diff/280001/chrome/test/base/testi...
chrome/test/base/testing_profile.cc:167: scoped_ptr<net::CookieStore>
cookie_store_;
On 2016/03/01 20:48:43, Randy Smith - Not in Fridays wrote:
> This (anti) pattern of a derived class actually owning the object the base
class
> has a pointer to does raise a potential failure mode during destruction,
because
> the derived class is destructed before the base class, so the base class will
> have a raw pointer to freed memory.  I presume it's ok ("ok" :-}) since we
have
> this pattern elsewhere for things other than cookies so presumably the
> URLRequestContext destructor can't do anything ugly, but I wanted to raise the
> flag that this shift is actually going in the wrong direction for safe
> destruction ordering.

True...Though this isn't a "real" URLRequestContext, it's just used as a
URLRequestContextForExtensions, which only uses its CookieStore, and doesn't
make requests.  I want to get rid of the context, at some point.

https://codereview.chromium.org/1701063002/diff/280001/headless/lib/browser/h...
File headless/lib/browser/headless_url_request_context_getter.cc (right):

https://codereview.chromium.org/1701063002/diff/280001/headless/lib/browser/h...
headless/lib/browser/headless_url_request_context_getter.cc:15: #include
"net/cookies/cookie_store.h"
On 2016/03/01 20:48:43, Randy Smith - Not in Fridays wrote:
> Hmmm.  As best I can tell, this type is only used implicitly, in the argument
to
> one function and the return value from another.  I'd think that both of those
> situations would be better handled by the include files that declare those
> functions, so my inclination would be that they wouldn't provoke an IWYU
> inclusion.  But I may be misunderstanding how IWYU should be implemented.

Both of those headers forward declare the class and only use it in scoped_ptrs,
but this file needs access to its destructor, since if (briefly) holds on to a
scoped_ptr.  Including those files isn't enough, unfortunately.  Get a build
error otherwise.

https://codereview.chromium.org/1701063002/diff/280001/ios/chrome/browser/bro...
File
ios/chrome/browser/browser_state/off_the_record_chrome_browser_state_io_data.mm
(right):

https://codereview.chromium.org/1701063002/diff/280001/ios/chrome/browser/bro...
ios/chrome/browser/browser_state/off_the_record_chrome_browser_state_io_data.mm:203:
cookie_util::CookieStoreConfig::RESTORED_SESSION_COOKIES,
On 2016/03/01 20:48:43, Randy Smith - Not in Fridays wrote:
> Not a problem for this CL, but do you have any idea why we're using a
> cookie_path_ and a RESTORED_SESSION_COOKIES for an OTR cookie context?

Yes.  This is mobile, where we can be destroyed at any time we're not in the
foreground, transparently to the user.  So as long as we have an incognito
window around, we'll keep around the cookies.  I think this is weird and not
great.  :(

https://codereview.chromium.org/1701063002/diff/280001/net/url_request/url_re...
File net/url_request/url_request_http_job_unittest.cc (right):

https://codereview.chromium.org/1701063002/diff/280001/net/url_request/url_re...
net/url_request/url_request_http_job_unittest.cc:548:
context_.set_cookie_store(&cookie_monster);
On 2016/03/01 20:48:43, Randy Smith - Not in Fridays wrote:
> Same issue mentioned elsewhere about a context raw pointer outliving the
object
> pointed to (by a little bit).  I'm inclined to suggest that you set the cookie
> store to null (or save the pointer and reset it) in this routine, since I
think
> of the destructor of the URLRequestContext likely being more stable than the
> destructor of a random test class.  

What I think is uglier than the lifetime issue here is that we're modifying a
"live" URLRequestContext, after initialization.  I'd really like the
TestURLRequestContext to be able to take ownership of stuff, before we call its
initialization function, so it can wire everything up as needed.

Anyhow, I addressed both issues by just creating a new TestURLRequestContext for
this test.

[mmenke, 2016-03-02 20:34:55]

Patchset #12 (id:340001) has been deleted

[mmenke, 2016-03-02 20:54:19]

mmenke@chromium.org changed reviewers:
+ alexclarke@chromium.org, davidben@chromium.org, droger@chromium.org,
lcwu@chromium.org, mkwst@chromium.org, sky@chromium.org, tnagel@chromium.org,
torne@chromium.org, zea@chromium.org

[mmenke, 2016-03-02 20:54:20]

I'm probably erring on the side of too many reviewers here, but here goes...

[+torne]:  Please review android_webview/.
[+droger]:  Please review ios/
[+zea]:  Please review chrome/browser/sync/
[+mkwst]:  Please review chrome/browser/browsing_data/
[+sky]:  Please review chrome/browser/sessions, chrome/test, and
chrome/browser/safe_browsing (I don't think I need a safe_browsing subowner,
given that it's fairly straightforward and Randy has already reviewed it).

The rest of the changes are probably TBRable, but feel free to review in the
meantime:
[+lcwu]:  chromecast/
[+davidben]:  content/
[+alexclarke]:  headless/
[+tnagel]:  components/policy

[Randy Smith (Not in Mondays), 2016-03-02 21:39:41]

LGTM.

[davidben, 2016-03-02 21:41:15]

content lgtm.

https://codereview.chromium.org/1701063002/diff/360001/content/public/browser...
File content/public/browser/cookie_store_factory.h (left):

https://codereview.chromium.org/1701063002/diff/360001/content/public/browser...
content/public/browser/cookie_store_factory.h:12: #include
"base/memory/ref_counted.h"
I think you still need this include due to CookieStoreConfig fields.

[mmenke, 2016-03-02 21:46:29]

https://codereview.chromium.org/1701063002/diff/360001/content/public/browser...
File content/public/browser/cookie_store_factory.h (left):

https://codereview.chromium.org/1701063002/diff/360001/content/public/browser...
content/public/browser/cookie_store_factory.h:12: #include
"base/memory/ref_counted.h"
On 2016/03/02 21:41:15, davidben wrote:
> I think you still need this include due to CookieStoreConfig fields.

Thanks for catching that!

We need to make an automatic tool for IWYU issues.  The number of developer
hours wasted on that...

[Nicolas Zea, 2016-03-02 22:33:18]

sync LGTM

https://codereview.chromium.org/1701063002/diff/380001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/1701063002/diff/380001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/sync_test.cc:87: #include
"net/url_request/url_request_context.h"
remove the request context and context getter headers too?

[mmenke, 2016-03-02 22:37:10]

https://codereview.chromium.org/1701063002/diff/380001/chrome/browser/sync/te...
File chrome/browser/sync/test/integration/sync_test.cc (right):

https://codereview.chromium.org/1701063002/diff/380001/chrome/browser/sync/te...
chrome/browser/sync/test/integration/sync_test.cc:87: #include
"net/url_request/url_request_context.h"
On 2016/03/02 22:33:18, Nicolas Zea wrote:
> remove the request context and context getter headers too?

Done, thanks!

[lcwu1, 2016-03-02 23:20:45]

chromecast lgtm

[sky, 2016-03-03 00:58:57]

https://codereview.chromium.org/1701063002/diff/400001/chrome/browser/session...
File chrome/browser/sessions/session_data_deleter.cc (right):

https://codereview.chromium.org/1701063002/diff/400001/chrome/browser/session...
chrome/browser/sessions/session_data_deleter.cc:94:
url_request_context_getter));
Is URLRequestContextGetter thread safe?

https://codereview.chromium.org/1701063002/diff/400001/chrome/browser/session...
chrome/browser/sessions/session_data_deleter.cc:117: // Shouldn't happen, but
best not to depend on the URLRequestContext's
I don't understand this comment. Either it can return null, or it can't. You're
implying you don't know.
Also, the description for GetURLRequestContext() says "Must only be called on
the network task runner., yet this is running on the io thread.

[mmenke, 2016-03-03 01:14:40]

https://codereview.chromium.org/1701063002/diff/400001/chrome/browser/session...
File chrome/browser/sessions/session_data_deleter.cc (right):

https://codereview.chromium.org/1701063002/diff/400001/chrome/browser/session...
chrome/browser/sessions/session_data_deleter.cc:94:
url_request_context_getter));
On 2016/03/03 00:58:56, sky wrote:
> Is URLRequestContextGetter thread safe?

It's refcounted.

https://codereview.chromium.org/1701063002/diff/400001/chrome/browser/session...
chrome/browser/sessions/session_data_deleter.cc:117: // Shouldn't happen, but
best not to depend on the URLRequestContext's
On 2016/03/03 00:58:56, sky wrote:
> I don't understand this comment. Either it can return null, or it can't.
You're
> implying you don't know.

It can, but only if ProfileIOData has been destroyed on the IO thread.  The task
to do this is posted to the IO thread by the ProfileIOData's destructor. 
SessionDataDeleter::Run is most likely not called after the Profile has been
destroyed, or in the Profile's destructor.  I don't want to depend on that.

> Also, the description for GetURLRequestContext() says "Must only be called on
> the network task runner., yet this is running on the io thread.

The IO thread is a content/ layer concept.  Net is below content, and an
instance network stack can be set up on any single thread (Well...modulo a
couple globals that should be fixed).

The IO thread is the thread the main network stack runs on.

[sky, 2016-03-03 04:28:09]

Ok, LGTM

[Mike West, 2016-03-03 09:18:05]

net/cookies and chrome/browser/browsing_data LGTM, thanks!

[droger, 2016-03-03 13:22:06]

ios/ LGTM

[mmenke, 2016-03-03 13:34:49]

Description was changed from

==========

==========

to

==========
CookieStore: Remove reference counting.

This has caused a number of issues in the past, and having
defined lifecycles for objects makes code easier to reason
about and maintain.

BUG=472744
==========

[Torne, 2016-03-05 01:02:08]

android_webview LGTM

[Thiemo Nagel, 2016-03-07 14:03:42]

https://codereview.chromium.org/1701063002/diff/400001/components/policy/core...
File components/policy/core/common/cloud/system_policy_request_context.h
(right):

https://codereview.chromium.org/1701063002/diff/400001/components/policy/core...
components/policy/core/common/cloud/system_policy_request_context.h:28: //
Overridden from net::URLRequestContextGetter:
Could you please document that the ownership of the returned pointer stays with
SystemPolicyRequestContext?

https://codereview.chromium.org/1701063002/diff/400001/components/policy/core...
components/policy/core/common/cloud/system_policy_request_context.h:38:
scoped_ptr<net::HttpNetworkLayer> http_transaction_factory_;
Is moving http_transaction_factory_ required for correctness or is this just a
cosmetic change?

https://codereview.chromium.org/1701063002/diff/400001/components/policy/core...
components/policy/core/common/cloud/system_policy_request_context.h:45:
scoped_refptr<net::URLRequestContextGetter> system_context_getter_;
Why was system_context_getter_ moved?

[mmenke, 2016-03-07 14:29:20]

https://codereview.chromium.org/1701063002/diff/400001/components/policy/core...
File components/policy/core/common/cloud/system_policy_request_context.h
(right):

https://codereview.chromium.org/1701063002/diff/400001/components/policy/core...
components/policy/core/common/cloud/system_policy_request_context.h:28: //
Overridden from net::URLRequestContextGetter:
On 2016/03/07 14:03:42, Thiemo Nagel wrote:
> Could you please document that the ownership of the returned pointer stays
with
> SystemPolicyRequestContext?

I've updated the comment in URLRequestContextGetter making explicit that this
does not transfer ownership.  Since it's a property of the parent class's
method, doesn't make sense to have all subclasses repeat the class docs.

https://codereview.chromium.org/1701063002/diff/400001/components/policy/core...
components/policy/core/common/cloud/system_policy_request_context.h:38:
scoped_ptr<net::HttpNetworkLayer> http_transaction_factory_;
On 2016/03/07 14:03:41, Thiemo Nagel wrote:
> Is moving http_transaction_factory_ required for correctness or is this just a
> cosmetic change?

The URLRequestContext depends on the HttpNetworkTransaction factory.  While, in
practice, you can tear down the context first, flipping the order isn't a great
idea.

Doesn't matter if this is above or below the CookieStore, but both should be
torn down after the context.

I didn't notice the StaticHttpUserAgentSettings.  I've moved that above the
context, too.

https://codereview.chromium.org/1701063002/diff/400001/components/policy/core...
components/policy/core/common/cloud/system_policy_request_context.h:45:
scoped_refptr<net::URLRequestContextGetter> system_context_getter_;
On 2016/03/07 14:03:42, Thiemo Nagel wrote:
> Why was system_context_getter_ moved?

Misread this file...Was thinking that this was the getter for context_ above,
which would mean it should be torn down first.  Actually, context_ depends on
this.  I've moved it back to where it was.

[Thiemo Nagel, 2016-03-07 17:01:21]

components/policy LGTM

https://codereview.chromium.org/1701063002/diff/400001/components/policy/core...
File components/policy/core/common/cloud/system_policy_request_context.h
(right):

https://codereview.chromium.org/1701063002/diff/400001/components/policy/core...
components/policy/core/common/cloud/system_policy_request_context.h:28: //
Overridden from net::URLRequestContextGetter:
On 2016/03/07 14:29:20, mmenke wrote:
> On 2016/03/07 14:03:42, Thiemo Nagel wrote:
> > Could you please document that the ownership of the returned pointer stays
> with
> > SystemPolicyRequestContext?
> 
> I've updated the comment in URLRequestContextGetter making explicit that this
> does not transfer ownership.  Since it's a property of the parent class's
> method, doesn't make sense to have all subclasses repeat the class docs.

Awesome, thanks!

[mmenke, 2016-03-07 17:50:59]

Description was changed from

==========
CookieStore: Remove reference counting.

This has caused a number of issues in the past, and having
defined lifecycles for objects makes code easier to reason
about and maintain.

BUG=472744
==========

to

==========
CookieStore: Remove reference counting.

This has caused a number of issues in the past, and having
defined lifecycles for objects makes code easier to reason
about and maintain.

BUG=472744
TBR=alexclarke@chromium.org, creis@chromium.org
==========

[mmenke, 2016-03-07 17:53:27]

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

[mmenke, 2016-03-07 17:53:28]

[+creis]  For content/, now that David's no longer an owner.

TBRing last two owners (alexclarke - just a header change, creis - davidben
signed off on this before he relinquished his content/ ownership)

[mmenke, 2016-03-07 17:53:50]

The CQ bit was checked by mmenke@chromium.org

[mmenke, 2016-03-07 17:53:51]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdsmith@chromium.org, davidben@chromium.org, zea@chromium.org,
droger@chromium.org, sky@chromium.org, torne@chromium.org, mkwst@chromium.org,
lcwu@chromium.org
Link to the patchset: https://codereview.chromium.org/1701063002/#ps440001
(title: "merge")

[commit-bot: I haz the power, 2016-03-07 17:54:14]

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

[mmenke, 2016-03-07 17:54:22]

The CQ bit was unchecked by mmenke@chromium.org

[mmenke, 2016-03-07 17:59:40]

The CQ bit was checked by mmenke@chromium.org

[commit-bot: I haz the power, 2016-03-07 18:00:10]

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

[commit-bot: I haz the power, 2016-03-07 18:21:04]

Description was changed from

==========
CookieStore: Remove reference counting.

This has caused a number of issues in the past, and having
defined lifecycles for objects makes code easier to reason
about and maintain.

BUG=472744
TBR=alexclarke@chromium.org, creis@chromium.org
==========

to

==========
CookieStore: Remove reference counting.

This has caused a number of issues in the past, and having
defined lifecycles for objects makes code easier to reason
about and maintain.

BUG=472744
TBR=alexclarke@chromium.org, creis@chromium.org
==========

[commit-bot: I haz the power, 2016-03-07 18:21:05]

Committed patchset #16 (id:440001)

[commit-bot: I haz the power, 2016-03-07 18:22:32]

Description was changed from

==========
CookieStore: Remove reference counting.

This has caused a number of issues in the past, and having
defined lifecycles for objects makes code easier to reason
about and maintain.

BUG=472744
TBR=alexclarke@chromium.org, creis@chromium.org
==========

to

==========
CookieStore: Remove reference counting.

This has caused a number of issues in the past, and having
defined lifecycles for objects makes code easier to reason
about and maintain.

BUG=472744
TBR=alexclarke@chromium.org, creis@chromium.org

Committed: https://crrev.com/606c59ca3b9bba3f4a7b1206cb6b1b3c0a8e2c9b
Cr-Commit-Position: refs/heads/master@{#379588}
==========

[commit-bot: I haz the power, 2016-03-07 18:22:34]

Patchset 16 (id:??) landed as
https://crrev.com/606c59ca3b9bba3f4a7b1206cb6b1b3c0a8e2c9b
Cr-Commit-Position: refs/heads/master@{#379588}

[Charlie Reis, 2016-03-07 19:17:12]

Yep, content/ LGTM.