NULL out the ProxyScriptFetcher used by ProxyService when the URLRequestContext it was using for downloads is destroyed.

This avoids the possibility of accessing freed memory when sharing ProxyService amongst request context, and the main context is destroyed first.

BUG=25338

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

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

7 files changed, 123 insertions, 10 deletions

diff --git a/net/proxy/init_proxy_resolver.cc b/net/proxy/init_proxy_resolver.cc
index a185278..bd8b444 100644
--- a/net/proxy/init_proxy_resolver.cc
+++ b/net/proxy/init_proxy_resolver.cc
@@ -129,6 +129,11 @@ int InitProxyResolver::DoFetchPacScript() {
 
   LOG(INFO) << "Starting fetch of PAC script " << pac_url;
 
+  if (!proxy_script_fetcher_) {
+    LOG(ERROR) << "Can't download PAC script, because no fetcher specified";
+    return ERR_UNEXPECTED;
+  }
+
   return proxy_script_fetcher_->Fetch(pac_url, &pac_bytes_, &io_callback_);
 }
 
diff --git a/net/proxy/init_proxy_resolver_unittest.cc b/net/proxy/init_proxy_resolver_unittest.cc
index 680a870..4e0b881 100644
--- a/net/proxy/init_proxy_resolver_unittest.cc
+++ b/net/proxy/init_proxy_resolver_unittest.cc
@@ -238,6 +238,20 @@ TEST(InitProxyResolverTest, CustomPacFails2) {
   EXPECT_EQ("", resolver.pac_bytes());
 }
 
+// Fail downloading the custom PAC script, because the fetcher was NULL.
+TEST(InitProxyResolverTest, HasNullProxyScriptFetcher) {
+  Rules rules;
+  RuleBasedProxyResolver resolver(&rules, true /*expects_pac_bytes*/);
+
+  ProxyConfig config;
+  config.pac_url = GURL("http://custom/proxy.pac");
+
+  TestCompletionCallback callback;
+  InitProxyResolver init(&resolver, NULL);
+  EXPECT_EQ(ERR_UNEXPECTED, init.Init(config, &callback, NULL));
+  EXPECT_EQ("", resolver.pac_bytes());
+}
+
 // Succeeds in choosing autodetect (wpad).
 TEST(InitProxyResolverTest, AutodetectSuccess) {
   Rules rules;
diff --git a/net/proxy/proxy_script_fetcher.cc b/net/proxy/proxy_script_fetcher.cc
index 882c406..7dc800e 100644
--- a/net/proxy/proxy_script_fetcher.cc
+++ b/net/proxy/proxy_script_fetcher.cc
@@ -86,6 +86,7 @@ class ProxyScriptFetcherImpl : public ProxyScriptFetcher,
   virtual int Fetch(const GURL& url, std::string* bytes,
                     CompletionCallback* callback);
   virtual void Cancel();
+  virtual URLRequestContext* GetRequestContext();
 
   // URLRequest::Delegate methods:
 
@@ -206,6 +207,10 @@ void ProxyScriptFetcherImpl::Cancel() {
   ResetCurRequestState();
 }
 
+URLRequestContext* ProxyScriptFetcherImpl::GetRequestContext() {
+  return url_request_context_;
+}
+
 void ProxyScriptFetcherImpl::OnAuthRequired(URLRequest* request,
                                             AuthChallengeInfo* auth_info) {
   DCHECK(request == cur_request_.get());
diff --git a/net/proxy/proxy_script_fetcher.h b/net/proxy/proxy_script_fetcher.h
index cbdb911..09ac84e 100644
--- a/net/proxy/proxy_script_fetcher.h
+++ b/net/proxy/proxy_script_fetcher.h
@@ -45,6 +45,10 @@ class ProxyScriptFetcher {
   // Aborts the in-progress fetch (if any).
   virtual void Cancel() = 0;
 
+  // Returns the request context that this fetcher uses to issue downloads,
+  // or NULL.
+  virtual URLRequestContext* GetRequestContext() { return NULL; }
+
   // Create a ProxyScriptFetcher that uses |url_request_context|.
   static ProxyScriptFetcher* Create(URLRequestContext* url_request_context);
 
diff --git a/net/proxy/proxy_service.cc b/net/proxy/proxy_service.cc
index 0177b75..92870fe 100644
--- a/net/proxy/proxy_service.cc
+++ b/net/proxy/proxy_service.cc
@@ -513,7 +513,23 @@ void ProxyService::RemovePendingRequest(PacRequest* req) {
 
 void ProxyService::SetProxyScriptFetcher(
     ProxyScriptFetcher* proxy_script_fetcher) {
-  proxy_script_fetcher_.reset(proxy_script_fetcher);
+  if (init_proxy_resolver_.get()) {
+    // We need to be careful to first cancel |init_proxy_resolver_|, since it
+    // holds a pointer to the old proxy script fetcher we are about to delete.
+
+    DCHECK(IsInitializingProxyResolver());
+    init_proxy_resolver_.reset();
+    proxy_script_fetcher_.reset(proxy_script_fetcher);
+
+    // Restart the initialization, using the new proxy script fetcher.
+    StartInitProxyResolver();
+  } else {
+    proxy_script_fetcher_.reset(proxy_script_fetcher);
+  }
+}
+
+ProxyScriptFetcher* ProxyService::GetProxyScriptFetcher() const {
+  return proxy_script_fetcher_.get();
 }
 
 void ProxyService::ResetConfigService(
@@ -629,17 +645,24 @@ void ProxyService::SetConfig(const ProxyConfig& config) {
     // OnInitProxyResolverComplete().
     SuspendAllPendingRequests();
 
-    init_proxy_resolver_.reset(
-        new InitProxyResolver(resolver_.get(), proxy_script_fetcher_.get()));
+    // Calls OnInitProxyResolverComplete() on completion.
+    StartInitProxyResolver();
+  }
+}
 
-    init_proxy_resolver_log_ = new LoadLog(kMaxNumLoadLogEntries);
+void ProxyService::StartInitProxyResolver() {
+  DCHECK(!init_proxy_resolver_.get());
 
-    int rv = init_proxy_resolver_->Init(
-        config_, &init_proxy_resolver_callback_, init_proxy_resolver_log_);
+  init_proxy_resolver_.reset(
+      new InitProxyResolver(resolver_.get(), proxy_script_fetcher_.get()));
 
-    if (rv != ERR_IO_PENDING)
-      OnInitProxyResolverComplete(rv);
-  }
+  init_proxy_resolver_log_ = new LoadLog(kMaxNumLoadLogEntries);
+
+  int rv = init_proxy_resolver_->Init(
+      config_, &init_proxy_resolver_callback_, init_proxy_resolver_log_);
+
+  if (rv != ERR_IO_PENDING)
+    OnInitProxyResolverComplete(rv);
 }
 
 void ProxyService::UpdateConfigIfOld() {
diff --git a/net/proxy/proxy_service.h b/net/proxy/proxy_service.h
index 194ad7b..4324e40 100644
--- a/net/proxy/proxy_service.h
+++ b/net/proxy/proxy_service.h
@@ -90,6 +90,7 @@ class ProxyService : public base::RefCountedThreadSafe<ProxyService> {
   // is of type ProxyResolverWithoutFetch. ProxyService takes ownership of
   // |proxy_script_fetcher|.
   void SetProxyScriptFetcher(ProxyScriptFetcher* proxy_script_fetcher);
+  ProxyScriptFetcher* GetProxyScriptFetcher() const;
 
   // Tells this ProxyService to start using a new ProxyConfigService to
   // retrieve its ProxyConfig from. The new ProxyConfigService will immediately
@@ -180,6 +181,10 @@ class ProxyService : public base::RefCountedThreadSafe<ProxyService> {
   // Assign |config| as the current configuration.
   void SetConfig(const ProxyConfig& config);
 
+  // Starts downloading and testing the various PAC choices.
+  // Calls OnInitProxyResolverComplete() when completed.
+  void StartInitProxyResolver();
+
   // Tries to update the configuration if it hasn't been checked in a while.
   void UpdateConfigIfOld();
 
diff --git a/net/proxy/proxy_service_unittest.cc b/net/proxy/proxy_service_unittest.cc
index 72faf17..b7c7d00 100644
--- a/net/proxy/proxy_service_unittest.cc
+++ b/net/proxy/proxy_service_unittest.cc
@@ -923,7 +923,7 @@ TEST(ProxyServiceTest, CancelInProgressRequest) {
   EXPECT_EQ("request3:80", info3.proxy_server().ToURI());
 }
 
-// Test the initial PAC download for ProxyResolverWithoutFetch.
+// Test the initial PAC download for resolver that expects bytes.
 TEST(ProxyServiceTest, InitialPACScriptDownload) {
   MockProxyConfigService* config_service =
       new MockProxyConfigService("http://foopy/proxy.pac");
@@ -1002,6 +1002,63 @@ TEST(ProxyServiceTest, InitialPACScriptDownload) {
   EXPECT_EQ("request3:80", info3.proxy_server().ToURI());
 }
 
+// Test changing the ProxyScriptFetcher while PAC download is in progress.
+TEST(ProxyServiceTest, ChangeScriptFetcherWhilePACDownloadInProgress) {
+  MockProxyConfigService* config_service =
+      new MockProxyConfigService("http://foopy/proxy.pac");
+
+  MockAsyncProxyResolverExpectsBytes* resolver =
+      new MockAsyncProxyResolverExpectsBytes;
+
+  scoped_refptr<ProxyService> service(
+      new ProxyService(config_service, resolver));
+
+  MockProxyScriptFetcher* fetcher = new MockProxyScriptFetcher;
+  service->SetProxyScriptFetcher(fetcher);
+
+  // Start 2 requests.
+
+  ProxyInfo info1;
+  TestCompletionCallback callback1;
+  int rv = service->ResolveProxy(
+      GURL("http://request1"), &info1, &callback1, NULL, NULL);
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  // The first request should have triggered download of PAC script.
+  EXPECT_TRUE(fetcher->has_pending_request());
+  EXPECT_EQ(GURL("http://foopy/proxy.pac"), fetcher->pending_request_url());
+
+  ProxyInfo info2;
+  TestCompletionCallback callback2;
+  rv = service->ResolveProxy(
+      GURL("http://request2"), &info2, &callback2, NULL, NULL);
+  EXPECT_EQ(ERR_IO_PENDING, rv);
+
+  // At this point the ProxyService should be waiting for the
+  // ProxyScriptFetcher to invoke its completion callback, notifying it of
+  // PAC script download completion.
+
+  // We now change out the ProxyService's script fetcher. We should restart
+  // the initialization with the new fetcher.
+
+  fetcher = new MockProxyScriptFetcher;
+  service->SetProxyScriptFetcher(fetcher);
+
+  // Nothing has been sent to the resolver yet.
+  EXPECT_TRUE(resolver->pending_requests().empty());
+
+  fetcher->NotifyFetchCompletion(OK, "pac-v1");
+
+  // Now that the PAC script is downloaded, it will have been sent to the proxy
+  // resolver.
+  EXPECT_EQ("pac-v1", resolver->pending_set_pac_script_request()->pac_bytes());
+  resolver->pending_set_pac_script_request()->CompleteNow(OK);
+
+  ASSERT_EQ(2u, resolver->pending_requests().size());
+  EXPECT_EQ(GURL("http://request1"), resolver->pending_requests()[0]->url());
+  EXPECT_EQ(GURL("http://request2"), resolver->pending_requests()[1]->url());
+}
+
 // Test cancellation of a request, while the PAC script is being fetched.
 TEST(ProxyServiceTest, CancelWhilePACFetching) {
   MockProxyConfigService* config_service =