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.

Review URL: http://codereview.chromium.org/165430

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@23612 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 22 insertions, 15 deletions

diff --git a/chrome/browser/net/chrome_url_request_context.cc b/chrome/browser/net/chrome_url_request_context.cc
index bb22ce9..16299ba 100644
--- a/chrome/browser/net/chrome_url_request_context.cc
+++ b/chrome/browser/net/chrome_url_request_context.cc
@@ -196,8 +196,11 @@ ChromeURLRequestContext* ChromeURLRequestContext::CreateOffTheRecord(
   ChromeURLRequestContext* context = new ChromeURLRequestContext(profile);
 
   // Share the same proxy service and host resolver as the original profile.
-  // This proxy service's lifespan is dependent on the lifespan of the original
-  // profile which we reference (see above).
+  // TODO(eroman): although ProxyService is reference counted, this sharing
+  // still has a subtle dependency on the lifespan of the original profile --
+  // ProxyService holds a (non referencing) pointer to the URLRequestContext
+  // it uses to download PAC scripts, which in this case is the original
+  // profile...
   context->host_resolver_ =
       profile->GetOriginalProfile()->GetRequestContext()->host_resolver();
   context->proxy_service_ =
@@ -513,11 +516,11 @@ ChromeURLRequestContext::~ChromeURLRequestContext() {
 
   // Do not delete the cookie store in the case of the media context, as it is
   // owned by the original context.
+  // TODO(eroman): The lifetime expectation of cookie_store_ is not right.
+  // The assumption here is that the original request context (which owns
+  // cookie_store_) is going to outlive the media context (which uses it).
+  // However based on the destruction order of profiles this is not true.
+  // http://crbug.com/15289.
   if (!is_media_)
     delete cookie_store_;
-
-  // Do not delete the proxy service in the case of OTR or media contexts, as
-  // it is owned by the original URLRequestContext.
-  if (!is_off_the_record_ && !is_media_)
-    delete proxy_service_;
 }
diff --git a/chrome/browser/net/resolve_proxy_msg_helper.h b/chrome/browser/net/resolve_proxy_msg_helper.h
index 92f1aa9..edc74702 100644
--- a/chrome/browser/net/resolve_proxy_msg_helper.h
+++ b/chrome/browser/net/resolve_proxy_msg_helper.h
@@ -8,6 +8,7 @@
 #include <deque>
 #include <string>
 
+#include "base/ref_counted.h"
 #include "googleurl/src/gurl.h"
 #include "ipc/ipc_message.h"
 #include "net/base/completion_callback.h"
@@ -84,7 +85,7 @@ class ResolveProxyMsgHelper {
   };
 
   // Members for the current outstanding proxy request.
-  net::ProxyService* proxy_service_;
+  scoped_refptr<net::ProxyService> proxy_service_;
   net::CompletionCallbackImpl<ResolveProxyMsgHelper> callback_;
   net::ProxyInfo proxy_info_;
 
@@ -96,7 +97,7 @@ class ResolveProxyMsgHelper {
 
   // Specified by unit-tests, to use this proxy service in place of the
   // global one.
-  net::ProxyService* proxy_service_override_;
+  scoped_refptr<net::ProxyService> proxy_service_override_;
 };
 
 #endif  // CHROME_BROWSER_NET_RESOLVE_PROXY_MSG_HELPER_
diff --git a/chrome/browser/net/resolve_proxy_msg_helper_unittest.cc b/chrome/browser/net/resolve_proxy_msg_helper_unittest.cc
index b9974ca..9e79623 100644
--- a/chrome/browser/net/resolve_proxy_msg_helper_unittest.cc
+++ b/chrome/browser/net/resolve_proxy_msg_helper_unittest.cc
@@ -54,10 +54,11 @@ class MyDelegate : public ResolveProxyMsgHelper::Delegate {
 // Issue three sequential requests -- each should succeed.
 TEST(ResolveProxyMsgHelperTest, Sequential) {
   net::MockAsyncProxyResolver* resolver = new net::MockAsyncProxyResolver;
-  net::ProxyService service(new MockProxyConfigService, resolver);
+  scoped_refptr<net::ProxyService> service(
+      new net::ProxyService(new MockProxyConfigService, resolver));
 
   MyDelegate delegate;
-  ResolveProxyMsgHelper helper(&delegate, &service);
+  ResolveProxyMsgHelper helper(&delegate, service);
 
   GURL url1("http://www.google1.com/");
   GURL url2("http://www.google2.com/");
@@ -116,10 +117,11 @@ TEST(ResolveProxyMsgHelperTest, Sequential) {
 // Issue a request while one is already in progress -- should be queued.
 TEST(ResolveProxyMsgHelperTest, QueueRequests) {
   net::MockAsyncProxyResolver* resolver = new net::MockAsyncProxyResolver;
-  net::ProxyService service(new MockProxyConfigService, resolver);
+  scoped_refptr<net::ProxyService> service(
+      new net::ProxyService(new MockProxyConfigService, resolver));
 
   MyDelegate delegate;
-  ResolveProxyMsgHelper helper(&delegate, &service);
+  ResolveProxyMsgHelper helper(&delegate, service);
 
   GURL url1("http://www.google1.com/");
   GURL url2("http://www.google2.com/");
@@ -182,11 +184,12 @@ TEST(ResolveProxyMsgHelperTest, QueueRequests) {
 // Delete the helper while a request is in progress, and others are pending.
 TEST(ResolveProxyMsgHelperTest, CancelPendingRequests) {
   net::MockAsyncProxyResolver* resolver = new net::MockAsyncProxyResolver;
-  net::ProxyService service(new MockProxyConfigService, resolver);
+  scoped_refptr<net::ProxyService> service(
+      new net::ProxyService(new MockProxyConfigService, resolver));
 
   MyDelegate delegate;
   scoped_ptr<ResolveProxyMsgHelper> helper(
-      new ResolveProxyMsgHelper(&delegate, &service));
+      new ResolveProxyMsgHelper(&delegate, service));
 
   GURL url1("http://www.google1.com/");
   GURL url2("http://www.google2.com/");