Reference count ProxyService....

Reference count ProxyService.

This is necessary since ProxyService is getting shared between chrome's url request contexts (off the record, media), and the current way it is being shared could result in free memory read/writes during shutdown.

This is a step towards fixing http://crbug.com/15289.

BUG=http://crbug.com/15289
TEST=The existing tests should continue to pass following this refactor.


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

[willchan no longer on Chromium, 2009-08-13 19:06:17]

LGTM, mod a few nits.

http://codereview.chromium.org/165430/diff/77/1077
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/165430/diff/77/1077#newcode203
Line 203: // profile...
This is slightly unclear.  ProxyService holds a pointer to the
ProxyScriptFetcher, of which the concrete type is ProxyScriptFetcherImpl which
has a pointer to URLRequestContext.  I've looked at this, and think that the
right solution is for ProxyScriptFetcherImpl to indeed maintain a scoped_refptr
to the URLRequestContext and have ProxyService have a Shutdown() method that
releases all its references, in order to break the reference cycle.  Something
in browser_main or something should keep a scoped_refptr to ProxyService and
call ProxyService::Shutdown() on shutdown to break the reference cycle.  Just a
thought.  Anyway, this is a TODO for another cl.

http://codereview.chromium.org/165430/diff/77/1076
File chrome/browser/net/resolve_proxy_msg_helper.h (right):

http://codereview.chromium.org/165430/diff/77/1076#newcode87
Line 87: scoped_refptr<net::ProxyService> proxy_service_;
- nits: you should be #include'ing "base/ref_counted.h" for this.  My guess is
you're pulling this in transitively through #include
"net/proxy/proxy_service.h".  I took a brief glance and don't see why that
include is necessary.  Can we switch that to a forward declare too?  Or will
that require a ton of changes (I'm not sure if anyone else is depending on the
transitive include too).

http://codereview.chromium.org/165430/diff/77/1067
File net/http/http_network_layer.cc (right):

http://codereview.chromium.org/165430/diff/77/1067#newcode80
Line 80: host_resolver_ = NULL;
This looks slightly odd to me.  It took me a second to realize you're
transferring ownership to HttpNetworkSession.  This seems necessary because
GetSession() instantiates HttpNetworkSession lazily.  Can we make it eager
instead of lazy?  Does that hurt startup or something?  It'd simply the code and
get rid of this memory transfer.  Otherwise, can you add comments to the header
explaining that the member variables only exist in order to transfer the memory
ownership to the HttpNetworkSession when it gets created?

http://codereview.chromium.org/165430/diff/77/1071
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/165430/diff/77/1071#newcode176
Line 176: // increase the lifespan of ProxyService while requests are
outstanding).
You should document why this is safe (it seems to be because when ProxyService
gets deleted, it will call Cancel() on each of the requests, which will NULL out
service_.

http://codereview.chromium.org/165430/diff/77/1072
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/165430/diff/77/1072#newcode59
Line 59: // do fallback if the chosen one doesn't work.
This doesn't seem to be part of the original change.  Can you update the
changelist description to mention you are updating stale comments too?

[eroman, 2009-08-14 00:56:48]

http://codereview.chromium.org/165430/diff/77/1077
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/165430/diff/77/1077#newcode203
Line 203: // profile...
On 2009/08/13 19:06:17, willchan wrote:
> This is slightly unclear.  ProxyService holds a pointer to the
> ProxyScriptFetcher, of which the concrete type is ProxyScriptFetcherImpl which
> has a pointer to URLRequestContext.  I've looked at this, and think that the
> right solution is for ProxyScriptFetcherImpl to indeed maintain a
scoped_refptr
> to the URLRequestContext and have ProxyService have a Shutdown() method that
> releases all its references, in order to break the reference cycle.  Something
> in browser_main or something should keep a scoped_refptr to ProxyService and
> call ProxyService::Shutdown() on shutdown to break the reference cycle.  Just
a
> thought.  Anyway, this is a TODO for another cl.

Yeah, that is a possibility.

Another idea could be to give it its own private URLRequestContext (doesn't
really need many dependencies anyway, since it bypasses cache).

http://codereview.chromium.org/165430/diff/77/1076
File chrome/browser/net/resolve_proxy_msg_helper.h (right):

http://codereview.chromium.org/165430/diff/77/1076#newcode87
Line 87: scoped_refptr<net::ProxyService> proxy_service_;
On 2009/08/13 19:06:17, willchan wrote:
> - nits: you should be #include'ing "base/ref_counted.h" for this.  My guess is
> you're pulling this in transitively through #include
> "net/proxy/proxy_service.h".  I took a brief glance and don't see why that
> include is necessary.  Can we switch that to a forward declare too?  Or will
> that require a ton of changes (I'm not sure if anyone else is depending on the
> transitive include too).

Added an include for "base/ref_counted.h".

I don't quite understand the second ask -- proxy_service.h needs to include
ref_counted.h since it inherits from RefCounted.

In fact really, it should inherit RefCountedThreadSafe; fixed.

http://codereview.chromium.org/165430/diff/77/1067
File net/http/http_network_layer.cc (right):

http://codereview.chromium.org/165430/diff/77/1067#newcode80
Line 80: host_resolver_ = NULL;
On 2009/08/13 19:06:17, willchan wrote:
> This looks slightly odd to me.  It took me a second to realize you're
> transferring ownership to HttpNetworkSession.  This seems necessary because
> GetSession() instantiates HttpNetworkSession lazily.  Can we make it eager
> instead of lazy?  Does that hurt startup or something?  It'd simply the code
and
> get rid of this memory transfer.  Otherwise, can you add comments to the
header
> explaining that the member variables only exist in order to transfer the
memory
> ownership to the HttpNetworkSession when it gets created?

Added a comment explaining.

As for why it is lazy rather than eager:

Yeah I dunno. One implication of moving it into the ctor, is it will make
construction happen on the UI thread rather than the IO thread. That is actually
something that bugs me about our setup of URLRequestContext's -- a good amount
of stuff is built on the UI thread, that really is non-thread safe and lives on
the IO thread...

That aside, I can't see anything obvious that would slow down startup if it were
moved to the ctor.

Filed http://crbug.com/19323 to investigate the issue further.

(Note also that HttpNetworkLayer used to be a bridge for having multiple HTTP
stacks, so it may not need to be as weird anymore)

http://codereview.chromium.org/165430/diff/77/1071
File net/proxy/proxy_service.cc (right):

http://codereview.chromium.org/165430/diff/77/1071#newcode176
Line 176: // increase the lifespan of ProxyService while requests are
outstanding).
On 2009/08/13 19:06:17, willchan wrote:
> You should document why this is safe (it seems to be because when ProxyService
> gets deleted, it will call Cancel() on each of the requests, which will NULL
out
> service_.

Done.

The full comment now reads:

  // Note that we don't hold a reference to the ProxyService. Outstanding
  // requests are cancelled during ~ProxyService, so this is guaranteed
  // to be valid throughout our lifetime.

http://codereview.chromium.org/165430/diff/77/1072
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/165430/diff/77/1072#newcode59
Line 59: // do fallback if the chosen one doesn't work.
On 2009/08/13 19:06:17, willchan wrote:
> This doesn't seem to be part of the original change.  Can you update the
> changelist description to mention you are updating stale comments too?

I have removed this from the changelist; will do it separately.

[willchan no longer on Chromium, 2009-08-14 03:52:27]

Still LGTM, although I'd like to understand why this needs to be thread safe ref
counted.

http://codereview.chromium.org/165430/diff/77/1077
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/165430/diff/77/1077#newcode203
Line 203: // profile...
On 2009/08/14 00:56:48, eroman wrote:
> On 2009/08/13 19:06:17, willchan wrote:
> > This is slightly unclear.  ProxyService holds a pointer to the
> > ProxyScriptFetcher, of which the concrete type is ProxyScriptFetcherImpl
which
> > has a pointer to URLRequestContext.  I've looked at this, and think that the
> > right solution is for ProxyScriptFetcherImpl to indeed maintain a
> scoped_refptr
> > to the URLRequestContext and have ProxyService have a Shutdown() method that
> > releases all its references, in order to break the reference cycle. 
Something
> > in browser_main or something should keep a scoped_refptr to ProxyService and
> > call ProxyService::Shutdown() on shutdown to break the reference cycle. 
Just
> a
> > thought.  Anyway, this is a TODO for another cl.
> 
> Yeah, that is a possibility.
> 
> Another idea could be to give it its own private URLRequestContext (doesn't
> really need many dependencies anyway, since it bypasses cache).

I don't know enough about the URLRequestContexts to evaluate this, so I'll trust
your judgment here since you seem to know what you're talking about :)

http://codereview.chromium.org/165430/diff/77/1076
File chrome/browser/net/resolve_proxy_msg_helper.h (right):

http://codereview.chromium.org/165430/diff/77/1076#newcode87
Line 87: scoped_refptr<net::ProxyService> proxy_service_;
On 2009/08/14 00:56:48, eroman wrote:
> On 2009/08/13 19:06:17, willchan wrote:
> > - nits: you should be #include'ing "base/ref_counted.h" for this.  My guess
is
> > you're pulling this in transitively through #include
> > "net/proxy/proxy_service.h".  I took a brief glance and don't see why that
> > include is necessary.  Can we switch that to a forward declare too?  Or will
> > that require a ton of changes (I'm not sure if anyone else is depending on
the
> > transitive include too).
> 
> Added an include for "base/ref_counted.h".
> 
> I don't quite understand the second ask -- proxy_service.h needs to include
> ref_counted.h since it inherits from RefCounted.
> 
> In fact really, it should inherit RefCountedThreadSafe; fixed.

Er, but this file (resolve_proxy_msg_helper.h) doesn't need to #include
"net/proxy/proxy_service.h", does it?

Also, why does it need to be thread safe?  What other thread is accessing the
ProxyService?

http://codereview.chromium.org/165430/diff/77/1067
File net/http/http_network_layer.cc (right):

http://codereview.chromium.org/165430/diff/77/1067#newcode80
Line 80: host_resolver_ = NULL;
On 2009/08/14 00:56:48, eroman wrote:
> On 2009/08/13 19:06:17, willchan wrote:
> > This looks slightly odd to me.  It took me a second to realize you're
> > transferring ownership to HttpNetworkSession.  This seems necessary because
> > GetSession() instantiates HttpNetworkSession lazily.  Can we make it eager
> > instead of lazy?  Does that hurt startup or something?  It'd simply the code
> and
> > get rid of this memory transfer.  Otherwise, can you add comments to the
> header
> > explaining that the member variables only exist in order to transfer the
> memory
> > ownership to the HttpNetworkSession when it gets created?
> 
> Added a comment explaining.
> 
> As for why it is lazy rather than eager:
> 
> Yeah I dunno. One implication of moving it into the ctor, is it will make
> construction happen on the UI thread rather than the IO thread. That is
actually
> something that bugs me about our setup of URLRequestContext's -- a good amount
> of stuff is built on the UI thread, that really is non-thread safe and lives
on
> the IO thread...

I don't think it's a big deal, as long as nothing is accessing it until it gets
transferred to the IO thread.  I do agree it'd be cleaner if it spent its whole
lifetime on the IO thread though.

> 
> That aside, I can't see anything obvious that would slow down startup if it
were
> moved to the ctor.
> 
> Filed http://crbug.com/19323 to investigate the issue further.
> 
> (Note also that HttpNetworkLayer used to be a bridge for having multiple HTTP
> stacks, so it may not need to be as weird anymore)

[eroman, 2009-08-14 04:18:39]

http://codereview.chromium.org/165430/diff/77/1076
File chrome/browser/net/resolve_proxy_msg_helper.h (right):

http://codereview.chromium.org/165430/diff/77/1076#newcode87
Line 87: scoped_refptr<net::ProxyService> proxy_service_;
On 2009/08/14 03:52:27, willchan wrote:
> On 2009/08/14 00:56:48, eroman wrote:
> > On 2009/08/13 19:06:17, willchan wrote:
> > > - nits: you should be #include'ing "base/ref_counted.h" for this.  My
guess
> is
> > > you're pulling this in transitively through #include
> > > "net/proxy/proxy_service.h".  I took a brief glance and don't see why that
> > > include is necessary.  Can we switch that to a forward declare too?  Or
will
> > > that require a ton of changes (I'm not sure if anyone else is depending on
> the
> > > transitive include too).
> > 
> > Added an include for "base/ref_counted.h".
> > 
> > I don't quite understand the second ask -- proxy_service.h needs to include
> > ref_counted.h since it inherits from RefCounted.
> > 
> > In fact really, it should inherit RefCountedThreadSafe; fixed.
> 
> Er, but this file (resolve_proxy_msg_helper.h) doesn't need to #include
> "net/proxy/proxy_service.h", does it?

Oh, thats what you meant!
Yeah sure, I will forward declare ProxyService instead of including the header.

> 
> Also, why does it need to be thread safe?  What other thread is accessing the
> ProxyService?

So what concerns me is that URLRequestContexts are created lazily on the UI
thread (by the Profile).

So lets say the original context gets created (UI thread) during startup, then
the IO thread is spawned and starts processing requests. So far things are
peachy.

Now the user Foo opens up an incognito window .. this creates an OTR profile on
the UI thread, which in turn creates a ChromeURLRequestContext, which in turn
does proxy_service_ = original_context->proxy_service()... In fact what it is
doing here is calling AddRef.  Since some dude could be calling AddRef at the
same time on IO thread it needs to be atomic so as not to bleed any refcounts.

[willchan no longer on Chromium, 2009-08-14 04:20:54]

http://codereview.chromium.org/165430/diff/77/1076
File chrome/browser/net/resolve_proxy_msg_helper.h (right):

http://codereview.chromium.org/165430/diff/77/1076#newcode87
Line 87: scoped_refptr<net::ProxyService> proxy_service_;
On 2009/08/14 04:18:39, eroman wrote:
> On 2009/08/14 03:52:27, willchan wrote:
> > On 2009/08/14 00:56:48, eroman wrote:
> > > On 2009/08/13 19:06:17, willchan wrote:
> > > > - nits: you should be #include'ing "base/ref_counted.h" for this.  My
> guess
> > is
> > > > you're pulling this in transitively through #include
> > > > "net/proxy/proxy_service.h".  I took a brief glance and don't see why
that
> > > > include is necessary.  Can we switch that to a forward declare too?  Or
> will
> > > > that require a ton of changes (I'm not sure if anyone else is depending
on
> > the
> > > > transitive include too).
> > > 
> > > Added an include for "base/ref_counted.h".
> > > 
> > > I don't quite understand the second ask -- proxy_service.h needs to
include
> > > ref_counted.h since it inherits from RefCounted.
> > > 
> > > In fact really, it should inherit RefCountedThreadSafe; fixed.
> > 
> > Er, but this file (resolve_proxy_msg_helper.h) doesn't need to #include
> > "net/proxy/proxy_service.h", does it?
> 
> Oh, thats what you meant!
> Yeah sure, I will forward declare ProxyService instead of including the
header.
> 
> > 
> > Also, why does it need to be thread safe?  What other thread is accessing
the
> > ProxyService?
> 
> So what concerns me is that URLRequestContexts are created lazily on the UI
> thread (by the Profile).
> 
> So lets say the original context gets created (UI thread) during startup, then
> the IO thread is spawned and starts processing requests. So far things are
> peachy.
> 
> Now the user Foo opens up an incognito window .. this creates an OTR profile
on
> the UI thread, which in turn creates a ChromeURLRequestContext, which in turn
> does proxy_service_ = original_context->proxy_service()... In fact what it is
> doing here is calling AddRef.  Since some dude could be calling AddRef at the
> same time on IO thread it needs to be atomic so as not to bleed any refcounts.

Ugh!  We really should just create the ChromeURLRequestContext on the IO thread
:(  Ok, makes sense to me then.