Cancel when InitProxyResolver cancels, so we don't use callback later.

Also fix the invariant of data, i.e. null client_callback_ once we
enter state DONE.

BUG=84496
TEST=net_unittests --gtest_filter=Dhcp*:InitProxy*

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

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

3 files changed, 76 insertions, 1 deletions

diff --git a/net/proxy/dhcp_proxy_script_fetcher_win.cc b/net/proxy/dhcp_proxy_script_fetcher_win.cc
index 94bf279..dbe4df2 100644
--- a/net/proxy/dhcp_proxy_script_fetcher_win.cc
+++ b/net/proxy/dhcp_proxy_script_fetcher_win.cc
@@ -99,6 +99,7 @@ void DhcpProxyScriptFetcherWin::CancelImpl() {
   DCHECK(CalledOnValidThread());
 
   if (state_ != STATE_DONE) {
+    client_callback_ = NULL;
     wait_timer_.Stop();
     state_ = STATE_DONE;
 
@@ -206,9 +207,11 @@ void DhcpProxyScriptFetcherWin::TransitionToDone() {
     }
   }
 
+  CompletionCallback* callback = client_callback_;
   CancelImpl();
   DCHECK_EQ(state_, STATE_DONE);
   DCHECK(fetchers_.empty());
+  DCHECK(!client_callback_);  // Invariant of data.
 
   UMA_HISTOGRAM_TIMES("Net.DhcpWpadCompletionTime",
                       base::TimeTicks::Now() - fetch_start_time_);
@@ -218,7 +221,8 @@ void DhcpProxyScriptFetcherWin::TransitionToDone() {
         "Net.DhcpWpadFetchError", std::abs(result), GetAllErrorCodesForUma());
   }
 
-  client_callback_->Run(result);
+  // We may be deleted re-entrantly within this outcall.
+  callback->Run(result);
 }
 
 int DhcpProxyScriptFetcherWin::num_pending_fetchers() const {
diff --git a/net/proxy/init_proxy_resolver.cc b/net/proxy/init_proxy_resolver.cc
index 52c5421..d2983b6 100644
--- a/net/proxy/init_proxy_resolver.cc
+++ b/net/proxy/init_proxy_resolver.cc
@@ -371,6 +371,10 @@ void InitProxyResolver::Cancel() {
       break;
   }
 
+  // This is safe to call in any state.
+  if (dhcp_proxy_script_fetcher_)
+    dhcp_proxy_script_fetcher_->Cancel();
+
   DidCompleteInit();
 }
 
diff --git a/net/proxy/init_proxy_resolver_unittest.cc b/net/proxy/init_proxy_resolver_unittest.cc
index 628abee..221b556 100644
--- a/net/proxy/init_proxy_resolver_unittest.cc
+++ b/net/proxy/init_proxy_resolver_unittest.cc
@@ -4,7 +4,9 @@
 
 #include <vector>
 
+#include "base/message_loop.h"
 #include "base/string_util.h"
+#include "base/time.h"
 #include "base/utf_string_conversions.h"
 #include "net/base/net_errors.h"
 #include "net/base/net_log.h"
@@ -576,6 +578,8 @@ class SynchronousSuccessDhcpFetcher : public DhcpProxyScriptFetcher {
  private:
   GURL gurl_;
   string16 expected_text_;
+
+  DISALLOW_COPY_AND_ASSIGN(SynchronousSuccessDhcpFetcher);
 };
 
 // All of the tests above that use InitProxyResolver have tested
@@ -633,5 +637,68 @@ TEST(InitProxyResolverTest, AutodetectDhcpFailParse) {
   EXPECT_FALSE(effective_config.has_pac_url());
 }
 
+class AsyncFailDhcpFetcher
+    : public DhcpProxyScriptFetcher,
+      public base::RefCountedThreadSafe<AsyncFailDhcpFetcher> {
+ public:
+  AsyncFailDhcpFetcher() : callback_(NULL) {
+  }
+
+  int Fetch(string16* utf16_text, CompletionCallback* callback) OVERRIDE {
+    callback_ = callback;
+    MessageLoop::current()->PostTask(
+        FROM_HERE,
+        NewRunnableMethod(this, &AsyncFailDhcpFetcher::CallbackWithFailure));
+    return ERR_IO_PENDING;
+  }
+
+  void Cancel() OVERRIDE {
+    callback_ = NULL;
+  }
+
+  const GURL& GetPacURL() const OVERRIDE {
+    return dummy_gurl_;
+  }
+
+  void CallbackWithFailure() {
+    if (callback_)
+      callback_->Run(ERR_PAC_NOT_IN_DHCP);
+  }
+
+ private:
+  GURL dummy_gurl_;
+  CompletionCallback* callback_;
+};
+
+TEST(InitProxyResolverTest, DhcpCancelledByDestructor) {
+  // This regression test would crash before
+  // http://codereview.chromium.org/7044058/
+  // Thus, we don't care much about actual results (hence no EXPECT or ASSERT
+  // macros below), just that it doesn't crash.
+  Rules rules;
+  RuleBasedProxyResolver resolver(&rules, true /*expects_pac_bytes*/);
+  RuleBasedProxyScriptFetcher fetcher(&rules);
+
+  scoped_refptr<AsyncFailDhcpFetcher> dhcp_fetcher(new AsyncFailDhcpFetcher());
+
+  ProxyConfig config;
+  config.set_auto_detect(true);
+  rules.AddFailDownloadRule("http://wpad/wpad.dat");
+
+  TestCompletionCallback callback;
+
+  // Scope so InitProxyResolver gets destroyed early.
+  {
+    InitProxyResolver init(&resolver, &fetcher, dhcp_fetcher.get(), NULL);
+    init.Init(config, base::TimeDelta(), NULL, &callback);
+  }
+
+  // Run the message loop to let the DHCP fetch complete and post the results
+  // back. Before the fix linked to above, this would try to invoke on
+  // the callback object provided by InitProxyResolver after it was
+  // no longer valid.
+  MessageLoop::current()->RunAllPending();
+}
+
 }  // namespace
 }  // namespace net