Use automatic memory management for URLRequestContext's members....

Use automatic memory management for URLRequestContext's members.
Also make ProxyService refcounted so the sharing between profiles is explicit.


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

[eroman, 2008-12-10 09:40:21]

This follows from a review comment you made about the "delete"s being repeated
in several places for URLRequestContext.

[wtc, 2008-12-10 22:11:20]

LGTM.  Thank you!

http://codereview.chromium.org/13701/diff/1/5
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/13701/diff/1/5#newcode900
Line 900: CreateSession(proxy_service.get()));
I think .get() is not needed for a scoped_refptr?

http://codereview.chromium.org/13701/diff/1/8
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/13701/diff/1/8#newcode86
Line 86: class ProxyService : public base::RefCounted<ProxyService> {
Please make sure ProxyService doesn't need to be thread-safe.

I guess the only sharing of ProxyService is between the
original URLRequestContext and the incognito mode
URLRequestContext right?  Isn't that sharing made safe
by ensuring the original context outlives the incognito
context?

http://codereview.chromium.org/13701/diff/1/8#newcode141
Line 141: // Create a ProxyService which fails every request. Convenience
function
You may want to add "causing a fallback on direct connections".

http://codereview.chromium.org/13701/diff/1/12
File net/url_request/url_request_context.h (right):

http://codereview.chromium.org/13701/diff/1/12#newcode22
Line 22: #include "net/proxy/proxy_service.h"
There is a trick that allows you to need only forward
declarations of CookieMonster and ProxyService: move the
definitions of the URLRequestContext constructors and
destructors to the .cc file.

http://codereview.chromium.org/13701/diff/1/12#newcode67
Line 67: // The following members are expected to be initialized by subclasses.
You may want to add a comment noting that the ordering of
proxy_service_ and http_transaction_factory_ is important
because proxy_service_ needs to be destroyed after
http_transaction_factory_.

http://codereview.chromium.org/13701/diff/1/10
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/13701/diff/1/10#newcode45
Line 45: proxy_service_ = new net::ProxyService(new net::ProxyResolverNull);
Should you change this to net::ProxyService::CreateNull()?

[eroman, 2008-12-10 22:20:16]

http://codereview.chromium.org/13701/diff/1/8
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/13701/diff/1/8#newcode86
Line 86: class ProxyService : public base::RefCounted<ProxyService> {
On 2008/12/10 22:11:20, wtc wrote:
> Please make sure ProxyService doesn't need to be thread-safe.
> 
> I guess the only sharing of ProxyService is between the
> original URLRequestContext and the incognito mode
> URLRequestContext right?  Isn't that sharing made safe
> by ensuring the original context outlives the incognito
> context?

Maybe you have an opinion regarding this:

Strictly speaking, ProxyService did not need to be made RefCounted.
I could have simply made proxy_service_ be a scoped_ptr like I did for the other
members members, and this changelist would be much shorter.

What displeases me about that approach, is it makes the OTR context awkward --
we would be putting a pointer that we don't own into a scoped_ptr. This means
the OTR context's destructor would need to call release() to avoid the base
destructor from actually free-ing it. This all just feels too hacky, so I opted
for a more explicit sharing model via refcounts.

[wtc, 2008-12-10 22:25:52]

http://codereview.chromium.org/13701/diff/1/8
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/13701/diff/1/8#newcode86
Line 86: class ProxyService : public base::RefCounted<ProxyService> {
Thanks for the explanation.  I was just thinking out loud
as I read your CL.  Now I see that because of the sharing
of proxy_service_ between URLRequestContexts, only two
solutions work:
- The original code, manually managing proxy_service_
  without using scoped_xxx, or
- Using scoped_refptr

Since we don't like manual management of proxy_service_ in
*all* the URLRequestContext subclasses, I agree
scoped_refptr is the better solution.

[eroman, 2008-12-12 20:40:28]

http://codereview.chromium.org/13701/diff/1/5
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/13701/diff/1/5#newcode900
Line 900: CreateSession(proxy_service.get()));
On 2008/12/10 22:11:20, wtc wrote:
> I think .get() is not needed for a scoped_refptr?

Done. Removed the .get().

http://codereview.chromium.org/13701/diff/1/8
File net/proxy/proxy_service.h (right):

http://codereview.chromium.org/13701/diff/1/8#newcode141
Line 141: // Create a ProxyService which fails every request. Convenience
function
On 2008/12/10 22:11:20, wtc wrote:
> You may want to add "causing a fallback on direct connections".

Done.

http://codereview.chromium.org/13701/diff/1/12
File net/url_request/url_request_context.h (right):

http://codereview.chromium.org/13701/diff/1/12#newcode22
Line 22: #include "net/proxy/proxy_service.h"
On 2008/12/10 22:11:20, wtc wrote:
> There is a trick that allows you to need only forward
> declarations of CookieMonster and ProxyService: move the
> definitions of the URLRequestContext constructors and
> destructors to the .cc file.

Clever trick. Alas there is no url_request_context.cc file -- do you think it is
worth adding one to avoid the header inclusion?

http://codereview.chromium.org/13701/diff/1/12#newcode67
Line 67: // The following members are expected to be initialized by subclasses.
On 2008/12/10 22:11:20, wtc wrote:
> You may want to add a comment noting that the ordering of
> proxy_service_ and http_transaction_factory_ is important
> because proxy_service_ needs to be destroyed after
> http_transaction_factory_.
> 

I have addressed this by making HttpNetworkFactory hold a refcount.

A nice consequence of this, is it lets me clean up the unit-test uglification I
had introduced earlier. i.e. in place of:

scoped_refptr<net::ProxyService> proxy_service(
    net::ProxyService::CreateNull());
net::HttpNetworkLayer factory(proxy_service.get());

I can simply do:

net::HttpNetworkLayer factory(net::ProxyService::CreateNull());

[eroman, 2008-12-12 21:30:17]

Added "test_shell_request_context.cc" to the CL in patchset 3.

[eroman, 2008-12-12 21:31:37]

Correction: meant to say "plugin_tests.cc" not "test_shell_request_context.cc"

[wtc, 2008-12-12 22:03:58]

http://codereview.chromium.org/13701/diff/1/5
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/13701/diff/1/5#newcode900
Line 900: CreateSession(proxy_service.get()));
On 2008/12/12 20:40:28, eroman wrote:
>
> Done. Removed the .get().

By the way, there are other instances of .get() that
can also be removed.  Some are in other files.  This
is just a minor nit though.

http://codereview.chromium.org/13701/diff/1/12
File net/url_request/url_request_context.h (right):

http://codereview.chromium.org/13701/diff/1/12#newcode22
Line 22: #include "net/proxy/proxy_service.h"
On 2008/12/12 20:40:28, eroman wrote:
>
> Clever trick. Alas there is no url_request_context.cc file -- do you think it
is
> worth adding one to avoid the header inclusion?

Hmm... I didn't know there is no url_request_context.cc
file.  This is a good question.  I consulted the style
guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Header_File_De...

It seems that the Style Guide recommends that we add
url_request_context.cc to avoid the header inclusion.

http://codereview.chromium.org/13701/diff/1/12#newcode67
Line 67: // The following members are expected to be initialized by subclasses.
On 2008/12/12 20:40:28, eroman wrote:
> 
> I have addressed this by making HttpNetworkFactory hold a refcount.
> 
> A nice consequence of this, is it lets me clean up the unit-test uglification
I
> had introduced earlier. i.e. in place of:
> 
> scoped_refptr<net::ProxyService> proxy_service(
>     net::ProxyService::CreateNull());
> net::HttpNetworkLayer factory(proxy_service.get());
> 
> I can simply do:
> 
> net::HttpNetworkLayer factory(net::ProxyService::CreateNull());

In general I don't like a refcounted object having too
many references.  Too many reference counts make it
difficult to determine the lifetime of the object.

This is just a preference, and I'm not sure if my opinion
is right.

[wtc, 2008-12-12 22:13:05]

LGTM on Patch Set 3.

http://codereview.chromium.org/13701/diff/427/431
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/13701/diff/427/431#newcode206
Line 206: net::HttpNetworkSession* CreateSession() {
Doesn't the Style Guide recommend against this?

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...

http://codereview.chromium.org/13701/diff/427/431#newcode885
Line 885: scoped_refptr<net::HttpNetworkSession>
session(CreateSession(proxy_service));
Why don't you make a similar change here?  (Just pass
new net::ProxyResolverFixed(proxy_info) to CreateSession.)

[eroman, 2008-12-12 23:02:24]

http://codereview.chromium.org/13701/diff/1/5
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/13701/diff/1/5#newcode900
Line 900: CreateSession(proxy_service.get()));
On 2008/12/12 22:03:58, wtc wrote:
> On 2008/12/12 20:40:28, eroman wrote:
> >
> > Done. Removed the .get().
> 
> By the way, there are other instances of .get() that
> can also be removed.  Some are in other files.  This
> is just a minor nit though.
> 
> 

Done, removed the other ".get()"s

http://codereview.chromium.org/13701/diff/1/12
File net/url_request/url_request_context.h (right):

http://codereview.chromium.org/13701/diff/1/12#newcode22
Line 22: #include "net/proxy/proxy_service.h"
On 2008/12/12 22:03:58, wtc wrote:
> On 2008/12/12 20:40:28, eroman wrote:
> >
> > Clever trick. Alas there is no url_request_context.cc file -- do you think
it
> is
> > worth adding one to avoid the header inclusion?
> 
> Hmm... I didn't know there is no url_request_context.cc
> file.  This is a good question.  I consulted the style
> guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Header_File_De...
> 
> It seems that the Style Guide recommends that we add
> url_request_context.cc to avoid the header inclusion.
> 

Done.

Did the build file dance (scons, vcproj, xcode) to add url_request_context.cc,
and restore the forward declarations.

http://codereview.chromium.org/13701/diff/1/10
File net/url_request/url_request_unittest.cc (right):

http://codereview.chromium.org/13701/diff/1/10#newcode45
Line 45: proxy_service_ = new net::ProxyService(new net::ProxyResolverNull);
On 2008/12/10 22:11:20, wtc wrote:
> Should you change this to net::ProxyService::CreateNull()?

Done.

http://codereview.chromium.org/13701/diff/427/431
File net/http/http_network_transaction_unittest.cc (right):

http://codereview.chromium.org/13701/diff/427/431#newcode206
Line 206: net::HttpNetworkSession* CreateSession() {
On 2008/12/12 22:13:06, wtc wrote:
> Doesn't the Style Guide recommend against this?
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Overl...
> 

Done.

Renamed the other one to "CreateSessionWithProxy".

http://codereview.chromium.org/13701/diff/427/431#newcode885
Line 885: scoped_refptr<net::HttpNetworkSession>
session(CreateSession(proxy_service));
On 2008/12/12 22:13:06, wtc wrote:
> Why don't you make a similar change here?  (Just pass
> new net::ProxyResolverFixed(proxy_info) to CreateSession.)

Done.

Changed this and the other instance to instead use:
CreateSessionWithProxy("myproxy:70")

[eroman, 2008-12-15 02:06:18]

Note: I had to roll-back this change, as it caused crashes in theUI-tests.

The problem was that the CookieStore(URLRequestContext::cookie_store_) may
depend on a PersistentCookieStore* that is owned by the superclass. 

Hence by changing the destruction order of cookie_store_ to instead happen in
the base class, it wound up accessing a dependency which had already been freed
(ProfileImpl::RequestContext::cookie_db_).

Given this usage, I no longer think this refactor is worthwhile and will put it
on hold.

[wtc, 2008-12-15 17:55:47]

On 2008/12/15 02:06:18, eroman wrote:
> Note: I had to roll-back this change, as it caused crashes in theUI-tests.
> 
> The problem was that the CookieStore(URLRequestContext::cookie_store_) may
> depend on a PersistentCookieStore* that is owned by the superclass. 
> 
> Hence by changing the destruction order of cookie_store_ to instead happen in
> the base class, it wound up accessing a dependency which had already been
freed
> (ProfileImpl::RequestContext::cookie_db_).
> 
> Given this usage, I no longer think this refactor is worthwhile and will put
it
> on hold.

Sorry to hear that.  Perhaps those three members are intended
to be created and destroyed by the subclasses.  We should add
a comment to summarize your findings so that nobody will try
this again.