From 80d6524d33061e0e4f7b06dd87ee94de3e05c7a8 Mon Sep 17 00:00:00 2001 From: "eroman@chromium.org" Date: Tue, 18 Aug 2009 03:58:09 +0000 Subject: 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 --- chrome/browser/net/chrome_url_request_context.cc | 17 ++++++++++------- chrome/browser/net/resolve_proxy_msg_helper.h | 5 +++-- chrome/browser/net/resolve_proxy_msg_helper_unittest.cc | 15 +++++++++------ 3 files changed, 22 insertions(+), 15 deletions(-) (limited to 'chrome') 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 #include +#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 proxy_service_; net::CompletionCallbackImpl 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 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 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 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 service( + new net::ProxyService(new MockProxyConfigService, resolver)); MyDelegate delegate; scoped_ptr helper( - new ResolveProxyMsgHelper(&delegate, &service)); + new ResolveProxyMsgHelper(&delegate, service)); GURL url1("http://www.google1.com/"); GURL url2("http://www.google2.com/"); -- cgit v1.1