Create a URLRequestContext for PAC fetching.

Create a URLRequestContext for PAC fetching.

Originally I was going to create a single "system" URLRequestContext.  I realized that was wrong, I need one for proxy script fetching that uses a direct ProxyService.  This way, we don't have the circular dependencies for URLRequestContext(A)=>ProxyService=>ProxyScriptFetcherImpl=>URLRequestContext(A).  Instead, we have URLRequestContext(A)=>ProxyService=>ProxyScriptFetcherImpl=>URLRequestContext(special one for proxy).

This also exposes some setters in URLRequestContext that were in ChromeURLRequestContext.  I guess this makes URLRequestContext a bit more "dangerous" since it could be mutated during runtime, but really we should probably pass around a const URLRequestContext within the network stack.  I've filed http://crbug.com/67597 to track this.

BUG=67232
TEST=none

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

[eroman, 2010-12-22 01:27:56]

The general approach looks reasonable.

Note that you will also want to update the load flag BYPASS_PROXY... either
remove this, or update the comment (since it is no longer really used in that
fashion.

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:179: void
InitializeProxyRequestContext(IOThread::Globals* globals,
See my naming comment.

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:190: new net::HttpCache(
We don't need to use an HttpCache here, since the script fetching will always
bypass cache anyway.

You should be able to just construct an HttpNetworkLayer, as it also implements
HttpTransactionFactory.

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:202: context->set_cookie_store(new
net::CookieMonster(NULL, NULL));
I wander if this might cause some subtle compat issues? Meh, probably won't be
an issue.

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:342: URLRequestContext* proxy_request_context = new
URLRequestContext;
See my naming suggestion.

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:343: InitializeProxyRequestContext(globals_,
net_log_, proxy_request_context);
How about making this a "Create()" method instead?

As an "Initialize" it is more fragile, since it relies on being passed an
un-initialized context.

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:10: #include <set>
Can this be deleted?

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:57: scoped_refptr<URLRequestContext>
proxy_request_context;
naming: I think this would be better called something like:

proxy_script_fetcher_context

Since "proxy request" makes it sound more like a resolving proxy thingy.

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/net/connecti...
File chrome/browser/net/connection_tester_unittest.cc (right):

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/net/connecti...
chrome/browser/net/connection_tester_unittest.cc:107:
scoped_refptr<URLRequestContext> proxy_request_context_;
See my naming suggestion.

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/net/connecti...
chrome/browser/net/connection_tester_unittest.cc:112:
proxy_request_context_->set_cert_verifier(&cert_verifier_);
In theory you can set a number of these dependencies to garbage (NULL?), since
the unittest will never use HTTPS or auth.

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/net/connecti...
chrome/browser/net/connection_tester_unittest.cc:120: new net::HttpCache(
Don't need to use a caching transaction factory.

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/net/connecti...
chrome/browser/net/connection_tester_unittest.cc:132:
proxy_request_context_->set_cookie_store(new net::CookieMonster(NULL, NULL));
greater than 80c.

[willchan no longer on Chromium, 2010-12-22 02:35:07]

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:179: void
InitializeProxyRequestContext(IOThread::Globals* globals,
On 2010/12/22 01:27:56, eroman wrote:
> See my naming comment.

Done.

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:190: new net::HttpCache(
On 2010/12/22 01:27:56, eroman wrote:
> We don't need to use an HttpCache here, since the script fetching will always
> bypass cache anyway.
> 
> You should be able to just construct an HttpNetworkLayer, as it also
implements
> HttpTransactionFactory.

Done.

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:342: URLRequestContext* proxy_request_context = new
URLRequestContext;
On 2010/12/22 01:27:56, eroman wrote:
> See my naming suggestion.

Done.

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:343: InitializeProxyRequestContext(globals_,
net_log_, proxy_request_context);
On 2010/12/22 01:27:56, eroman wrote:
> How about making this a "Create()" method instead?
> 
> As an "Initialize" it is more fragile, since it relies on being passed an
> un-initialized context.

Done.

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:10: #include <set>
On 2010/12/22 01:27:56, eroman wrote:
> Can this be deleted?

Done.

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:57: scoped_refptr<URLRequestContext>
proxy_request_context;
On 2010/12/22 01:27:56, eroman wrote:
> naming: I think this would be better called something like:
> 
> proxy_script_fetcher_context
> 
> Since "proxy request" makes it sound more like a resolving proxy thingy.

Done.

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/net/connecti...
File chrome/browser/net/connection_tester_unittest.cc (right):

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/net/connecti...
chrome/browser/net/connection_tester_unittest.cc:107:
scoped_refptr<URLRequestContext> proxy_request_context_;
On 2010/12/22 01:27:56, eroman wrote:
> See my naming suggestion.

Done.

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/net/connecti...
chrome/browser/net/connection_tester_unittest.cc:112:
proxy_request_context_->set_cert_verifier(&cert_verifier_);
On 2010/12/22 01:27:56, eroman wrote:
> In theory you can set a number of these dependencies to garbage (NULL?), since
> the unittest will never use HTTPS or auth.

I left these unchanged, just so it's easier for people to see how these get set.
 I was too lazy to clean things up and I feel like it's probably cheap anyway.

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/net/connecti...
chrome/browser/net/connection_tester_unittest.cc:120: new net::HttpCache(
On 2010/12/22 01:27:56, eroman wrote:
> Don't need to use a caching transaction factory.

Done.

http://codereview.chromium.org/5961005/diff/12001/chrome/browser/net/connecti...
chrome/browser/net/connection_tester_unittest.cc:132:
proxy_request_context_->set_cookie_store(new net::CookieMonster(NULL, NULL));
On 2010/12/22 01:27:56, eroman wrote:
> greater than 80c.

Done.

[eroman, 2010-12-23 01:40:59]

lgtm

http://codereview.chromium.org/5961005/diff/19001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/5961005/diff/19001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:40: #include "net/http/http_cache.h"
Is this header still needed?

http://codereview.chromium.org/5961005/diff/19001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:202: new
net::SpdySessionPool(globals->ssl_config_service.get()),
Does HttpNetworkLayer take ownership of the session pool, or will this leak?

http://codereview.chromium.org/5961005/diff/19001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/5961005/diff/19001/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:49: net::ClientSocketFactory* client_socket_factory;
Where is this freed? Or is GetDefaultFactory a singleton?

http://codereview.chromium.org/5961005/diff/19001/chrome/browser/net/connecti...
File chrome/browser/net/connection_tester_unittest.cc (right):

http://codereview.chromium.org/5961005/diff/19001/chrome/browser/net/connecti...
chrome/browser/net/connection_tester_unittest.cc:14: #include
"net/disk_cache/disk_cache.h"
is this header necessary?

http://codereview.chromium.org/5961005/diff/19001/chrome/browser/net/connecti...
chrome/browser/net/connection_tester_unittest.cc:19: #include
"net/proxy/proxy_script_fetcher_impl.h"
Is this header necessary?

[willchan no longer on Chromium, 2010-12-23 23:51:58]

http://codereview.chromium.org/5961005/diff/19001/chrome/browser/io_thread.cc
File chrome/browser/io_thread.cc (right):

http://codereview.chromium.org/5961005/diff/19001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:40: #include "net/http/http_cache.h"
On 2010/12/23 01:40:59, eroman wrote:
> Is this header still needed?

Done.

http://codereview.chromium.org/5961005/diff/19001/chrome/browser/io_thread.cc...
chrome/browser/io_thread.cc:202: new
net::SpdySessionPool(globals->ssl_config_service.get()),
On 2010/12/23 01:40:59, eroman wrote:
> Does HttpNetworkLayer take ownership of the session pool, or will this leak?

Takes ownership.  It's sort of weird.  I'm hoping to fix these semantics later
on.  I want ProfileIOData or IOThread::Globals to own it.

http://codereview.chromium.org/5961005/diff/19001/chrome/browser/io_thread.h
File chrome/browser/io_thread.h (right):

http://codereview.chromium.org/5961005/diff/19001/chrome/browser/io_thread.h#...
chrome/browser/io_thread.h:49: net::ClientSocketFactory* client_socket_factory;
On 2010/12/23 01:40:59, eroman wrote:
> Where is this freed? Or is GetDefaultFactory a singleton?

It surprised me to find out it is a singleton.  I originally wrote this as a
scoped_ptr and it crashed on me :P

http://codereview.chromium.org/5961005/diff/19001/chrome/browser/net/connecti...
File chrome/browser/net/connection_tester_unittest.cc (right):

http://codereview.chromium.org/5961005/diff/19001/chrome/browser/net/connecti...
chrome/browser/net/connection_tester_unittest.cc:14: #include
"net/disk_cache/disk_cache.h"
On 2010/12/23 01:40:59, eroman wrote:
> is this header necessary?

Done.

http://codereview.chromium.org/5961005/diff/19001/chrome/browser/net/connecti...
chrome/browser/net/connection_tester_unittest.cc:19: #include
"net/proxy/proxy_script_fetcher_impl.h"
On 2010/12/23 01:40:59, eroman wrote:
> Is this header necessary?

Done.

[Timur Iskhodzhanov, 2010-12-24 06:49:07]

Looks like this change has introduced a few memory leaks which made almost all
the Valgrind bots red.
We (me and glider) will need to file some suppressionz when at the office.
Please watch the memory waterfall next time after commiting

[willchan no longer on Chromium, 2010-12-24 07:22:39]

Oops, easy fix.  I'll patch it tonight.  Sorry!

On Thu, Dec 23, 2010 at 10:49 PM, <timurrrr@chromium.org> wrote:

> Looks like this change has introduced a few memory leaks which made almost
> all
> the Valgrind bots red.
> We (me and glider) will need to file some suppressionz when at the office.
> Please watch the memory waterfall next time after commiting
>
>
> http://codereview.chromium.org/5961005/
>