Stop refcounting URLRequestContext.

Stop refcounting URLRequestContext.

While doing so, fix a few issues with the code like ordering of URLRequestContext to ensure correct destruction order. Also fix const correctness in some places.

BUG=58859
TEST=none
TBR=willchan

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

[willchan no longer on Chromium, 2012-05-04 02:12:09]

I have a few ownership bugs to sort out, but I'm mostly done. It's ready for a
look.

[eroman, 2012-05-04 04:27:02]

LGTM. What has your testing strategy been? Be sure to run through the valgrind
try-bots, since this has a lot of potential for mistakes :)

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

http://codereview.chromium.org/10299002/diff/34080/chrome/browser/io_thread.c...
chrome/browser/io_thread.cc:199: // Create a separate request context for PAC
fetches to avoid reference cycles.
nit: I wander if "reference cycles" is the right wording now.

http://codereview.chromium.org/10299002/diff/34080/chrome/browser/net/chrome_...
File chrome/browser/net/chrome_url_request_context.cc (right):

http://codereview.chromium.org/10299002/diff/34080/chrome/browser/net/chrome_...
chrome/browser/net/chrome_url_request_context.cc:51: virtual
ChromeURLRequestContext* Create() {
can you add OVERRIDE?

http://codereview.chromium.org/10299002/diff/34080/chrome/browser/net/chrome_...
chrome/browser/net/chrome_url_request_context.cc:65: virtual
ChromeURLRequestContext* Create() {
can you add OVERRIDE?

http://codereview.chromium.org/10299002/diff/34080/chrome/browser/net/chrome_...
chrome/browser/net/chrome_url_request_context.cc:83: virtual
ChromeURLRequestContext* Create() {
can you add OVERRIDE?

http://codereview.chromium.org/10299002/diff/34080/chrome/browser/net/chrome_...
chrome/browser/net/chrome_url_request_context.cc:103: virtual
ChromeURLRequestContext* Create() {
can you add OVERRIDE?

http://codereview.chromium.org/10299002/diff/34080/chrome/browser/net/http_pi...
File chrome/browser/net/http_pipelining_compatibility_client.h (right):

http://codereview.chromium.org/10299002/diff/34080/chrome/browser/net/http_pi...
chrome/browser/net/http_pipelining_compatibility_client.h:144:
scoped_ptr<net::URLRequestContext> url_request_context_;
Seems like http_transaction_factory should outlive url_request_context_ (in
other words, order this further down).

http://codereview.chromium.org/10299002/diff/34080/chrome/service/net/service...
File chrome/service/net/service_url_request_context.h (right):

http://codereview.chromium.org/10299002/diff/34080/chrome/service/net/service...
chrome/service/net/service_url_request_context.h:74:
scoped_ptr<net::ProxyConfigService> proxy_config_service_;
I expect the proxy_config_service_ should outlive url_request_context.

http://codereview.chromium.org/10299002/diff/34080/content/shell/shell_url_re...
File content/shell/shell_url_request_context_getter.h (right):

http://codereview.chromium.org/10299002/diff/34080/content/shell/shell_url_re...
content/shell/shell_url_request_context_getter.h:51:
scoped_ptr<net::URLRequestContextStorage> storage_;
Feels like the storage should probably outlive url_request_context_ in
declaration order.

http://codereview.chromium.org/10299002/diff/34080/content/test/mock_resource...
File content/test/mock_resource_context.h (right):

http://codereview.chromium.org/10299002/diff/34080/content/test/mock_resource...
content/test/mock_resource_context.h:11: #include "base/memory/scoped_ptr.h"
Is this header needed?

http://codereview.chromium.org/10299002/diff/34080/net/url_request/url_reques...
File net/url_request/url_request.cc (right):

http://codereview.chromium.org/10299002/diff/34080/net/url_request/url_reques...
net/url_request/url_request.cc:729: void URLRequest::set_context(const
URLRequestContext* context) {
sidecomment: are there any legitimate use cases for changing the context other
than from NULL --> something OR something --> NULL? Seems like a recipe for
problems to do anything else, and certainly should not change after a request
has started. hmm i'll have to think more about this.

http://codereview.chromium.org/10299002/diff/34080/net/url_request/url_reques...
File net/url_request/url_request_http_job.h (left):

http://codereview.chromium.org/10299002/diff/34080/net/url_request/url_reques...
net/url_request/url_request_http_job.h:98: scoped_refptr<const
URLRequestContext> context_;
*fingers crossed*

[cbentzel, 2012-05-04 14:44:39]

I didn't look at the code at all, but the Subject of this CL makes me happy!

[willchan no longer on Chromium, 2012-05-04 21:47:40]

OK, I think I've fixed all test errors. I'm hitting the valgrind/drmemory bots
too. I left myself a TODO to ditch the proxy script fetcher context. I've
addressed all eroman comments.

https://chromiumcodereview.appspot.com/10299002/diff/34080/net/url_request/ur...
File net/url_request/url_request_http_job.h (left):

https://chromiumcodereview.appspot.com/10299002/diff/34080/net/url_request/ur...
net/url_request/url_request_http_job.h:98: scoped_refptr<const
URLRequestContext> context_;
On 2012/05/04 04:27:02, eroman wrote:
> *fingers crossed*

I'm optimistic :)

[willchan no longer on Chromium, 2012-05-04 21:48:07]

Just to be clear, my target land date is *after* the milestone branch.

[willchan no longer on Chromium, 2012-05-08 02:29:23]

+jam: FYI

I'm going to TBR this after the branch point. The night before, I'll give it
another round of tryserver tests to make sure everything's solid and I have no
merge conflicts. I'm targeting Thursday in the morning so I'm around to fix
anything that breaks. Of course, nothing will break, because my code is perfect
:P

[willchan no longer on Chromium, 2012-05-10 17:46:59]

Initialize to NULL

[willchan no longer on Chromium, 2012-05-10 17:47:35]

Initialize to NULL

[willchan no longer on Chromium, 2012-05-10 17:52:51]

Initialize to NULL

[commit-bot: I haz the power, 2012-05-10 17:54:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/willchan@chromium.org/10299002/39095

[commit-bot: I haz the power, 2012-05-10 18:30:22]

Try job failure for 10299002-39095 (retry) on mac_rel for step "compile"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-05-10 18:42:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/willchan@chromium.org/10299002/39095

[commit-bot: I haz the power, 2012-05-10 19:42:05]

Try job failure for 10299002-39095 (retry) on win for step "update" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

Step "update" is always a major failure.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2012-05-10 19:49:46]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/willchan@chromium.org/10299002/39095

[commit-bot: I haz the power, 2012-05-10 20:16:34]

Try job failure for 10299002-39095 (retry) on android for steps "Compile,
build".
It's a second try, previously, steps "Compile, build" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[commit-bot: I haz the power, 2012-05-11 01:31:49]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/willchan@chromium.org/10299002/39095

[commit-bot: I haz the power, 2012-05-11 03:27:22]

Change committed as 136497