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@56342 0039d316-1c4b-4281-b951-d872f2087c98

3 files changed, 111 insertions, 49 deletions

diff --git a/net/base/dnsrr_resolver.cc b/net/base/dnsrr_resolver.cc
index 8c041d9..79bb2da 100644
--- a/net/base/dnsrr_resolver.cc
+++ b/net/base/dnsrr_resolver.cc
@@ -8,6 +8,7 @@
 #include <resolv.h>
 #endif
 
+#include "base/message_loop.h"
 #include "base/string_piece.h"
 #include "base/task.h"
 #include "base/worker_pool.h"
@@ -19,6 +20,63 @@ 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_;
+};
+
+}  // anonymous namespace
+
 #if defined(OS_POSIX)
 
 // A Buffer is used for walking over a DNS packet.
@@ -147,14 +205,16 @@ 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) {
+bool RRResponse::ParseFromResponse(const uint8* p, unsigned len,
+                                   uint16 rrtype_requested) {
   name.clear();
   ttl = 0;
   dnssec = false;
@@ -240,35 +300,40 @@ class ResolveTask : public Task {
  public:
   ResolveTask(const std::string& name, uint16 rrtype,
               uint16 flags, CompletionCallback* callback,
-              DnsRRResolver::Response* response)
+              RRResponse* response, MessageLoop* ml)
       : name_(name),
         rrtype_(rrtype),
         flags_(flags),
-        callback_(callback),
+        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)
-        return Failure();
+        r = false;
     }
 
-    unsigned long saved_options = _res.options;
-    bool r = Do();
+    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();
-    }
+      if (!r && DnsReloadTimerHasExpired()) {
+        res_nclose(&_res);
+        if (res_ninit(&_res) == 0)
+          r = Do();
+      }
 #endif
-    _res.options = saved_options;
+      _res.options = saved_options;
+    }
     int error = r ? OK : ERR_NAME_NOT_RESOLVED;
-    callback_->Run(error);
+
+    subtask_->set_rv(error);
+    subtask_.release()->Post();
   }
 
   bool Do() {
@@ -302,15 +367,11 @@ class ResolveTask : public Task {
  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_;
+  scoped_ptr<CompletionCallbackTask> subtask_;
+  RRResponse* const response_;
 };
 #else  // OS_POSIX
 // On non-Linux platforms we fail everything for now.
@@ -318,7 +379,7 @@ class ResolveTask : public Task {
  public:
   ResolveTask(const std::string& name, uint16 rrtype,
               uint16 flags, CompletionCallback* callback,
-              DnsRRResolver::Response* response)
+              RRResponse* response)
       : callback_(callback) {
   }
 
@@ -335,7 +396,7 @@ class ResolveTask : public Task {
 // 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 +404,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)];