Fix a memory error in ProxyResolverV8Tracing.

net/proxy/proxy_resolver_v8_tracing.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "net/proxy/proxy_resolver_v8_tracing.h"
 
 #include "base/bind.h"
 #include "base/message_loop_proxy.h"
 #include "base/stringprintf.h"
 #include "base/synchronization/cancellation_flag.h"
@@ skipping 641 matching lines @@
 }
 
 void ProxyResolverV8Tracing::Job::DoDnsOperation(
     const std::string& host, ResolveDnsOperation op, bool* out_cache_hit) {
   CheckIsOnOriginThread();
   DCHECK(!pending_dns_);
 
   if (cancelled_.IsSet())
     return;
 
+  HostResolver::RequestHandle dns_request = NULL;
   int result = host_resolver()->Resolve(
       MakeDnsRequestInfo(host, op),
       &pending_dns_addresses_,
       base::Bind(&Job::OnDnsOperationComplete, this),
-      &pending_dns_,
+      &dns_request,
       bound_net_log_);
 
   bool completed_synchronously = result != ERR_IO_PENDING;
 
+  // Check if the request was cancelled as a side-effect of calling into the
+  // HostResolver. This isn't the ordinary execution flow, however it is
+  // exercised by unit-tests.
+  if (cancelled_.IsSet()) {
+    if (!completed_synchronously)
+      host_resolver()->CancelRequest(dns_request);
+    return;
+  }
+
+  if (!completed_synchronously) {
+    DCHECK(dns_request);
+    pending_dns_ = dns_request;

[mmenke, 2013/01/31 21:11:23]

This isn't correct.  If we're doing a blocking call and complete synchronously,
we won't set pending_dns_ here, and then we'll run into the DCHECK in
OnDnsOperationComplete.

[eroman, 2013/01/31 21:29:49]

Actually it is the DCHECK in OnDnsOperationComplete() which is wrong. If the
request completed synchronously, then pending_dns_ should be NULL. Adding the
assignment shouldn't change that catch.

Good catch! This is a left-over from one of my prior refactorings.

For now I have removed the DCHECK. I will follow-up with a test to exercise this
situation, and see if I can't clarify the code in that as well.

[mmenke, 2013/01/31 21:34:32]

You're right.  Was thinking that we could just always set it, but then we'd be
relying on it being non-NULL on synchronous completion.

+  }
+
   if (!blocking_dns_) {
     // Check if the DNS result can be serviced directly from the cache.
     // (The worker thread is blocked waiting for this information).
-    if (completed_synchronously) {
+    if (completed_synchronously)
       SaveDnsToLocalCache(host, op, result, pending_dns_addresses_);
-      pending_dns_ = NULL;
-    }
 
     // Important: Do not read/write |out_cache_hit| after signalling, since
     // the memory may no longer be valid.
     *out_cache_hit = completed_synchronously;
     event_.Signal();
 
     if (completed_synchronously)
       return;
   }
 
@@ skipping 287 matching lines @@
   DCHECK(!set_pac_script_job_);
   CHECK_EQ(0, num_outstanding_callbacks_);
 
   set_pac_script_job_ = new Job(this);
   set_pac_script_job_->StartSetPacScript(script_data, callback);
 
   return ERR_IO_PENDING;
 }
 
 }  // namespace net