DnsRRResolver: hoist inner class and callback on MessageLoop.

This change moves the Response class to the top level to save other
files from having to #include the header in order to get a pointer to
a Response object.

This change also modifies the semantics of resolution: the callback
will now be made of the origin MessageLoop, rather than a random
thread.

TEST=net_unittests
BUG=none

http://codereview.chromium.org/3110015/show

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

3 files changed, 191 insertions, 125 deletions

diff --git a/net/base/dnsrr_resolver.cc b/net/base/dnsrr_resolver.cc
index 8c041d9..dbbb0af 100644
--- a/net/base/dnsrr_resolver.cc
+++ b/net/base/dnsrr_resolver.cc
@@ -8,6 +8,8 @@
 #include <resolv.h>
 #endif
 
+#include "base/message_loop.h"
+#include "base/scoped_ptr.h"
 #include "base/string_piece.h"
 #include "base/task.h"
 #include "base/worker_pool.h"
@@ -19,7 +21,159 @@ namespace net {
 
 static const uint16 kClassIN = 1;
 
+namespace {
+
+class CompletionCallbackTask : public Task,
+                               public MessageLoop::DestructionObserver {
+ public:
+  explicit CompletionCallbackTask(CompletionCallback* callback,
+                                  MessageLoop* ml)
+      : callback_(callback),
+        rv_(OK),
+        posted_(false),
+        message_loop_(ml) {
+    ml->AddDestructionObserver(this);
+  }
+
+  void set_rv(int rv) {
+    rv_ = rv;
+  }
+
+  void Post() {
+    AutoLock locked(lock_);
+    DCHECK(!posted_);
+
+    if (!message_loop_) {
+      // MessageLoop got deleted, nothing to do.
+      delete this;
+      return;
+    }
+    posted_ = true;
+    message_loop_->PostTask(FROM_HERE, this);
+  }
+
+  // Task interface
+  void Run() {
+    message_loop_->RemoveDestructionObserver(this);
+    callback_->Run(rv_);
+    // We will be deleted by the message loop.
+  }
+
+  // DestructionObserver interface
+  virtual void WillDestroyCurrentMessageLoop() {
+    AutoLock locked(lock_);
+    message_loop_ = NULL;
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CompletionCallbackTask);
+
+  CompletionCallback* callback_;
+  int rv_;
+  bool posted_;
+
+  Lock lock_;  // covers |message_loop_|
+  MessageLoop* message_loop_;
+};
+
 #if defined(OS_POSIX)
+class ResolveTask : public Task {
+ public:
+  ResolveTask(const std::string& name, uint16 rrtype,
+              uint16 flags, CompletionCallback* callback,
+              RRResponse* response, MessageLoop* ml)
+      : name_(name),
+        rrtype_(rrtype),
+        flags_(flags),
+        subtask_(new CompletionCallbackTask(callback, ml)),
+        response_(response) {
+  }
+
+  virtual void Run() {
+    // Runs on a worker thread.
+
+    bool r = true;
+    if ((_res.options & RES_INIT) == 0) {
+      if (res_ninit(&_res) != 0)
+        r = false;
+    }
+
+    if (r) {
+      unsigned long saved_options = _res.options;
+      r = Do();
+
+#if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_OPENBSD)
+      if (!r && DnsReloadTimerHasExpired()) {
+        res_nclose(&_res);
+        if (res_ninit(&_res) == 0)
+          r = Do();
+      }
+#endif
+      _res.options = saved_options;
+    }
+    int error = r ? OK : ERR_NAME_NOT_RESOLVED;
+
+    subtask_->set_rv(error);
+    subtask_.release()->Post();
+  }
+
+  bool Do() {
+    // For DNSSEC, a 4K buffer is suggested
+    static const unsigned kMaxDNSPayload = 4096;
+
+#ifndef RES_USE_DNSSEC
+    // Some versions of libresolv don't have support for the DO bit. In this
+    // case, we proceed without it.
+    static const int RES_USE_DNSSEC = 0;
+#endif
+
+#ifndef RES_USE_EDNS0
+    // Some versions of glibc are so old that they don't support EDNS0 either.
+    // http://code.google.com/p/chromium/issues/detail?id=51676
+    static const int RES_USE_EDNS0 = 0;
+#endif
+
+    // We set the options explicitly. Note that this removes several default
+    // options: RES_DEFNAMES and RES_DNSRCH (see res_init(3)).
+    _res.options = RES_INIT | RES_RECURSE | RES_USE_EDNS0 | RES_USE_DNSSEC;
+    uint8 answer[kMaxDNSPayload];
+    int len = res_search(name_.c_str(), kClassIN, rrtype_, answer,
+                         sizeof(answer));
+    if (len == -1)
+      return false;
+
+    return response_->ParseFromResponse(answer, len, rrtype_);
+  }
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(ResolveTask);
+
+  const std::string name_;
+  const uint16 rrtype_;
+  const uint16 flags_;
+  scoped_ptr<CompletionCallbackTask> subtask_;
+  RRResponse* const response_;
+};
+#else  // OS_POSIX
+// On non-Linux platforms we fail everything for now.
+class ResolveTask : public Task {
+ public:
+  ResolveTask(const std::string& name, uint16 rrtype,
+              uint16 flags, CompletionCallback* callback,
+              RRResponse* response, MessageLoop* ml)
+      : subtask_(new CompletionCallbackTask(callback, ml)) {
+  }
+
+  virtual void Run() {
+    subtask_->set_rv(ERR_NAME_NOT_RESOLVED);
+    subtask_->Post();
+  }
+
+ private:
+  CompletionCallbackTask* const subtask_;
+  DISALLOW_COPY_AND_ASSIGN(ResolveTask);
+};
+#endif
 
 // A Buffer is used for walking over a DNS packet.
 class Buffer {
@@ -147,14 +301,19 @@ class Buffer {
   }
 
  private:
+  DISALLOW_COPY_AND_ASSIGN(Buffer);
+
   const uint8* p_;
   const uint8* const packet_;
   unsigned len_;
   const unsigned packet_len_;
 };
 
-bool DnsRRResolver::Response::ParseFromResponse(const uint8* p, unsigned len,
-                                                uint16 rrtype_requested) {
+}  // anonymous namespace
+
+bool RRResponse::ParseFromResponse(const uint8* p, unsigned len,
+                                   uint16 rrtype_requested) {
+#if defined(OS_POSIX)
   name.clear();
   ttl = 0;
   dnssec = false;
@@ -232,110 +391,16 @@ bool DnsRRResolver::Response::ParseFromResponse(const uint8* p, unsigned len,
       signatures.push_back(std::string(rrdata.data(), rrdata.size()));
     }
   }
+#endif  // defined(OS_POSIX)
 
   return true;
 }
 
-class ResolveTask : public Task {
- public:
-  ResolveTask(const std::string& name, uint16 rrtype,
-              uint16 flags, CompletionCallback* callback,
-              DnsRRResolver::Response* response)
-      : name_(name),
-        rrtype_(rrtype),
-        flags_(flags),
-        callback_(callback),
-        response_(response) {
-  }
-
-  virtual void Run() {
-    // Runs on a worker thread.
-
-    if ((_res.options & RES_INIT) == 0) {
-      if (res_ninit(&_res) != 0)
-        return Failure();
-    }
-
-    unsigned long saved_options = _res.options;
-    bool r = Do();
-
-#if defined(OS_POSIX) && !defined(OS_MACOSX) && !defined(OS_OPENBSD)
-    if (!r && DnsReloadTimerHasExpired()) {
-      res_nclose(&_res);
-      if (res_ninit(&_res) == 0)
-        r = Do();
-    }
-#endif
-    _res.options = saved_options;
-    int error = r ? OK : ERR_NAME_NOT_RESOLVED;
-    callback_->Run(error);
-  }
-
-  bool Do() {
-    // For DNSSEC, a 4K buffer is suggested
-    static const unsigned kMaxDNSPayload = 4096;
-
-#ifndef RES_USE_DNSSEC
-    // Some versions of libresolv don't have support for the DO bit. In this
-    // case, we proceed without it.
-    static const int RES_USE_DNSSEC = 0;
-#endif
-
-#ifndef RES_USE_EDNS0
-    // Some versions of glibc are so old that they don't support EDNS0 either.
-    // http://code.google.com/p/chromium/issues/detail?id=51676
-    static const int RES_USE_EDNS0 = 0;
-#endif
-
-    // We set the options explicitly. Note that this removes several default
-    // options: RES_DEFNAMES and RES_DNSRCH (see res_init(3)).
-    _res.options = RES_INIT | RES_RECURSE | RES_USE_EDNS0 | RES_USE_DNSSEC;
-    uint8 answer[kMaxDNSPayload];
-    int len = res_search(name_.c_str(), kClassIN, rrtype_, answer,
-                         sizeof(answer));
-    if (len == -1)
-      return false;
-
-    return response_->ParseFromResponse(answer, len, rrtype_);
-  }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(ResolveTask);
-
-  void Failure() {
-    callback_->Run(ERR_NAME_NOT_RESOLVED);
-  }
-
-  const std::string name_;
-  const uint16 rrtype_;
-  const uint16 flags_;
-  CompletionCallback* const callback_;
-  DnsRRResolver::Response* const response_;
-};
-#else  // OS_POSIX
-// On non-Linux platforms we fail everything for now.
-class ResolveTask : public Task {
- public:
-  ResolveTask(const std::string& name, uint16 rrtype,
-              uint16 flags, CompletionCallback* callback,
-              DnsRRResolver::Response* response)
-      : callback_(callback) {
-  }
-
-  virtual void Run() {
-    callback_->Run(ERR_NAME_NOT_RESOLVED);
-  }
-
- private:
-  CompletionCallback* const callback_;
-  DISALLOW_COPY_AND_ASSIGN(ResolveTask);
-};
-#endif
 
 // static
 bool DnsRRResolver::Resolve(const std::string& name, uint16 rrtype,
                             uint16 flags, CompletionCallback* callback,
-                            Response* response) {
+                            RRResponse* response) {
   if (!callback || !response || name.empty())
     return false;
 
@@ -343,7 +408,8 @@ bool DnsRRResolver::Resolve(const std::string& name, uint16 rrtype,
   if (rrtype == kDNS_ANY)
     return false;
 
-  ResolveTask* task = new ResolveTask(name, rrtype, flags, callback, response);
+  ResolveTask* task = new ResolveTask(name, rrtype, flags, callback, response,
+                                      MessageLoop::current());
 
   return WorkerPool::PostTask(FROM_HERE, task, true /* task is slow */);
 }
diff --git a/net/base/dnsrr_resolver.h b/net/base/dnsrr_resolver.h
index 6dfea2f..28bceaa 100644
--- a/net/base/dnsrr_resolver.h
+++ b/net/base/dnsrr_resolver.h
@@ -15,6 +15,24 @@
 
 namespace net {
 
+// RRResponse contains the result of a successful request for a resource record.
+struct RRResponse {
+  // name contains the canonical name of the resulting domain. If the queried
+  // name was a CNAME then this can differ.
+  std::string name;
+  // ttl contains the TTL of the resource records.
+  uint32 ttl;
+  // dnssec is true if the response was DNSSEC validated.
+  bool dnssec;
+  std::vector<std::string> rrdatas;
+  // sigs contains the RRSIG records returned.
+  std::vector<std::string> signatures;
+
+  // For testing only
+  bool ParseFromResponse(const uint8* data, unsigned len,
+                         uint16 rrtype_requested);
+};
+
 // DnsRRResolver resolves arbitary DNS resource record types. It should not be
 // confused with HostResolver and should not be used to resolve A/AAAA records.
 //
@@ -25,37 +43,19 @@ namespace net {
 // the name is a fully qualified DNS domain.
 class DnsRRResolver {
  public:
-  // Response contains the details of a successful request.
-  struct Response {
-    // name contains the canonical name of the resulting domain. If the queried
-    // name was a CNAME then this can differ.
-    std::string name;
-    // ttl contains the TTL of the resource records.
-    uint32 ttl;
-    // dnssec is true if the response was DNSSEC validated.
-    bool dnssec;
-    std::vector<std::string> rrdatas;
-    // sigs contains the RRSIG records returned.
-    std::vector<std::string> signatures;
-
-    // For testing only
-    bool ParseFromResponse(const uint8* data, unsigned len,
-                           uint16 rrtype_requested);
-  };
-
   enum {
     // Try harder to get a DNSSEC signed response. This doesn't mean that the
-    // Response will always have the dnssec bit set.
+    // RRResponse will always have the dnssec bit set.
     FLAG_WANT_DNSSEC = 1,
   };
 
   // Resolve starts the resolution process. When complete, |callback| is called
   // with a result. If the result is |OK| then |response| is filled with the
-  // result of the resolution. Note the |callback| is called from a random
-  // worker thread.
+  // result of the resolution. Note the |callback| is called on the current
+  // MessageLoop.
   static bool Resolve(const std::string& name, uint16 rrtype,
                       uint16 flags, CompletionCallback* callback,
-                      Response* response);
+                      RRResponse* response);
 
  private:
   DISALLOW_COPY_AND_ASSIGN(DnsRRResolver);
diff --git a/net/base/dnsrr_resolver_unittest.cc b/net/base/dnsrr_resolver_unittest.cc
index 6e36126..23a0b40 100644
--- a/net/base/dnsrr_resolver_unittest.cc
+++ b/net/base/dnsrr_resolver_unittest.cc
@@ -52,7 +52,7 @@ class Rendezvous : public CallbackRunner<Tuple1<int> > {
 // This test is disabled because it depends on the external network to pass.
 // However, it may be useful when chaging the code.
 TEST_F(DnsRRResolverTest, DISABLED_NetworkResolve) {
-  DnsRRResolver::Response response;
+  RRResponse response;
   Rendezvous callback;
   ASSERT_TRUE(DnsRRResolver::Resolve(
         "agl._pka.imperialviolet.org", kDNS_TXT, 0, &callback, &response));
@@ -111,7 +111,7 @@ static const uint8 kExamplePacket[] = {
 };
 
 TEST_F(DnsRRResolverTest, ParseExample) {
-  DnsRRResolver::Response response;
+  RRResponse response;
   ASSERT_TRUE(response.ParseFromResponse(kExamplePacket,
               sizeof(kExamplePacket), kDNS_TXT));
   ASSERT_EQ(1u, response.rrdatas.size());
@@ -124,7 +124,7 @@ TEST_F(DnsRRResolverTest, ParseExample) {
 }
 
 TEST_F(DnsRRResolverTest, FuzzTruncation) {
-  DnsRRResolver::Response response;
+  RRResponse response;
 
   for (unsigned len = sizeof(kExamplePacket); len <= sizeof(kExamplePacket);
        len--) {
@@ -133,7 +133,7 @@ TEST_F(DnsRRResolverTest, FuzzTruncation) {
 }
 
 TEST_F(DnsRRResolverTest, FuzzCorruption) {
-  DnsRRResolver::Response response;
+  RRResponse response;
   uint8 copy[sizeof(kExamplePacket)];