Fix PepperPortAllocator to resolve STUN addresses using HostResolverPrivate.

remoting/client/plugin/pepper_port_allocator.cc

Index: remoting/client/plugin/pepper_port_allocator.cc
diff --git a/remoting/client/plugin/pepper_port_allocator.cc b/remoting/client/plugin/pepper_port_allocator.cc
index e999f54d6eb9f20fd4a4cb5a4f9372fc557fde32..a55487afa3c7eca73729f1eb6cffe2856bb44945 100644
--- a/remoting/client/plugin/pepper_port_allocator.cc
+++ b/remoting/client/plugin/pepper_port_allocator.cc
@@ -5,18 +5,24 @@
 #include "remoting/client/plugin/pepper_port_allocator.h"
 
 #include "base/string_number_conversions.h"
+#include "net/base/net_util.h"
 #include "ppapi/c/pp_errors.h"
 #include "ppapi/cpp/completion_callback.h"
+#include "ppapi/cpp/private/host_resolver_private.h"
 #include "ppapi/cpp/url_loader.h"
 #include "ppapi/cpp/url_request_info.h"
 #include "ppapi/cpp/url_response_info.h"
 #include "remoting/client/plugin/pepper_network_manager.h"
 #include "remoting/client/plugin/pepper_packet_socket_factory.h"
+#include "remoting/client/plugin/pepper_util.h"
 
 namespace remoting {
 
 namespace {
 
+// URL used to create a relay session.
+const char kCreateRelaySessionURL[] = "/create_session";
+
 // Read buffer we allocate per read when reading response from
 // URLLoader. Normally the response from URL loader is smaller than 1kB.
 const int kReadSize = 1024;
@@ -30,27 +36,40 @@ class PepperPortAllocatorSession
       int component,
       const std::vector<talk_base::SocketAddress>& stun_hosts,
       const std::vector<std::string>& relay_hosts,
-      const std::string& relay,
+      const std::string& relay_token,
       const pp::InstanceHandle& instance);
   virtual ~PepperPortAllocatorSession();
 
   // cricket::HttpPortAllocatorBase overrides.
   virtual void ConfigReady(cricket::PortConfiguration* config) OVERRIDE;
+  virtual void GetPortConfigurations() OVERRIDE;
   virtual void SendSessionRequest(const std::string& host, int port) OVERRIDE;
 
  private:
-  // Callback handlers for pp::URLLoader.
+  // Callback handlers for Pepper APIs.
+  static void HostResolverCallback(void* user_data, int32_t result);
   static void UrlLoaderOpenCallback(void* user_data, int32_t result);
   static void UrlLoaderReadCallback(void* user_data, int32_t result);
 
+  void ResolveStunServerAddress();
+  void OnStunAddressResolved(int32_t result);
+  void OnStunResolutionFinished();
+
   void OnUrlOpened(int32_t result);
   void ReadResponseBody();
   void OnResponseBodyRead(int32_t result);
 
   pp::InstanceHandle instance_;
 
-  scoped_ptr<pp::URLLoader> url_loader_;
-  std::vector<char> body_;
+  pp::HostResolverPrivate stun_address_resolver_;
+  talk_base::SocketAddress unresolved_stun_address_;
+  talk_base::SocketAddress stun_address_;
+  bool stun_address_resolved_;

[Wez, 2012/05/15 23:11:29]

Why do you need |unresolved_stun_address_| and |stun_address_resolved_| rather
than just testing |stun_address_.IsResolved()|, and potentially
|stun_address_.IsNil()|?

[Sergey Ulanov, 2012/05/16 00:22:30]

Removed both unresolved_stun_address_ and stun_address_resolved_.

+  int stun_port_;
+
+  scoped_ptr<pp::URLLoader> relay_url_loader_;
+  std::vector<char> relay_response_body_;
+  bool relay_response_received_;
 
   DISALLOW_COPY_AND_ASSIGN(PepperPortAllocatorSession);
 };
@@ -61,11 +80,19 @@ PepperPortAllocatorSession::PepperPortAllocatorSession(
     int component,
     const std::vector<talk_base::SocketAddress>& stun_hosts,
     const std::vector<std::string>& relay_hosts,
-    const std::string& relay,
+    const std::string& relay_token,
     const pp::InstanceHandle& instance)
-    : HttpPortAllocatorSessionBase(
-        allocator, channel_name, component, stun_hosts, relay_hosts, relay, ""),
-      instance_(instance) {
+    : cricket::HttpPortAllocatorSessionBase(
+        allocator, channel_name, component, stun_hosts,
+        relay_hosts, relay_token, ""),
+      instance_(instance),
+      stun_address_resolver_(instance_),
+      stun_address_resolved_(false),
+      stun_port_(0),
+      relay_response_received_(false) {
+  set_flags(cricket::PORTALLOCATOR_DISABLE_TCP);

[Wez, 2012/05/15 23:11:29]

This doesn't look related to this CL's description?

[Sergey Ulanov, 2012/05/16 00:22:30]

Updated the description.

+  if (stun_hosts.size() > 0)
+    unresolved_stun_address_ = stun_hosts[0];
 }
 
 PepperPortAllocatorSession::~PepperPortAllocatorSession() {
@@ -73,6 +100,11 @@ PepperPortAllocatorSession::~PepperPortAllocatorSession() {
 
 void PepperPortAllocatorSession::ConfigReady(
     cricket::PortConfiguration* config) {
+  if (config->stun_address.IsUnresolved()) {
+    // Replace stun address with the resolved IP address.
+    config->stun_address = stun_address_;
+  }
+
   // Filter out non-UDP relay ports, so that we don't try using TCP.
   for (cricket::PortConfiguration::RelayList::iterator relay =
            config->relays.begin(); relay != config->relays.end(); ++relay) {
@@ -88,10 +120,81 @@ void PepperPortAllocatorSession::ConfigReady(
   cricket::BasicPortAllocatorSession::ConfigReady(config);
 }
 
+void PepperPortAllocatorSession::GetPortConfigurations() {
+  // Add an empty configuration synchronously, so a local connection
+  // can be started immediately.
+  ConfigReady(new cricket::PortConfiguration(talk_base::SocketAddress()));
+
+  ResolveStunServerAddress();
+  TryCreateRelaySession();
+}
+
+void PepperPortAllocatorSession::ResolveStunServerAddress() {
+  if (unresolved_stun_address_.IsNil()) {
+    OnStunResolutionFinished();
+    return;
+  }
+
+  if (!unresolved_stun_address_.IsUnresolved()) {
+    stun_address_ = unresolved_stun_address_;
+    OnStunResolutionFinished();
+    return;
+  }
+
+  std::string hostname = unresolved_stun_address_.hostname();
+  uint16 port = unresolved_stun_address_.port();
+
+  PP_HostResolver_Private_Hint hint;
+  hint.flags = 0;
+  hint.family = PP_NETADDRESSFAMILY_IPV4;
+  int result = stun_address_resolver_.Resolve(
+      hostname, port, hint, pp::CompletionCallback(
+          &PepperPortAllocatorSession::HostResolverCallback, this));
+  DCHECK_EQ(result, PP_OK_COMPLETIONPENDING);
+}
+
+void PepperPortAllocatorSession::OnStunAddressResolved(int32_t result) {
+  if (result < 0) {
+    LOG(ERROR) << "Failed to resolve stun address "
+               << unresolved_stun_address_.hostname() << ": " << result;
+    OnStunResolutionFinished();
+    return;
+  }
+
+  if (!stun_address_resolver_.GetSize()) {
+    LOG(WARNING) << "Received 0 addresses for stun server "
+               << unresolved_stun_address_.hostname();
+    OnStunResolutionFinished();
+    return;
+  }
+
+  PP_NetAddress_Private address;
+  if (!stun_address_resolver_.GetNetAddress(0, &address) ||
+      !PpAddressToSocketAddress(address, &stun_address_)) {
+    LOG(ERROR) << "Failed to get address for STUN server "
+               << unresolved_stun_address_.hostname();
+    OnStunResolutionFinished();
+    return;
+  }
+
+  OnStunResolutionFinished();
+}
+
+void PepperPortAllocatorSession::OnStunResolutionFinished() {
+  stun_address_resolved_ = true;
+  if (!stun_address_.IsNil())

[Wez, 2012/05/15 23:11:29]

|!stun_address_.IsUnresolved()|?

[Wez, 2012/05/15 23:11:29]

Use { } consistently with the other if(), below.

[Sergey Ulanov, 2012/05/16 00:22:30]

STUN address may not be set. IsUnresolved returns false for Nil addresses.

[Sergey Ulanov, 2012/05/16 00:22:30]

Done.

[Wez, 2012/05/16 00:51:53]

OK; consider adding a DCHECK() or even CHECK() for !IsUnresolved() on
stun_address_, at this point.

[Sergey Ulanov, 2012/05/16 01:02:16]

Done. Actually we don't need this method anymore, so I simplified this code
further.

+    ConfigReady(new cricket::PortConfiguration(stun_address_));
+
+  if (relay_response_received_) {

[Wez, 2012/05/15 23:11:29]

This never gets set anywhere.  Do you actually need it; would it be sufficient
to test for a non-empty |relay_response_body_|?

[Sergey Ulanov, 2012/05/16 00:22:30]

It should be set in OnResponseBodyRead(). Fixed now.
 
Yes, we do need it. relay_response_body_ may be non-empty while we are in
process of reading server response, but haven't finished reading it yet.

+    ReceiveSessionResponse(std::string(relay_response_body_.begin(),
+                                       relay_response_body_.end()));
+  }
+}
+
 void PepperPortAllocatorSession::SendSessionRequest(
     const std::string& host,
     int port) {
-  url_loader_.reset(new pp::URLLoader(instance_));
+  relay_url_loader_.reset(new pp::URLLoader(instance_));
   pp::URLRequestInfo request_info(instance_);
   std::string url = "https://" + host + ":" + base::IntToString(port) +
       GetSessionRequestUrl() + "&sn=1";
@@ -103,7 +206,7 @@ void PepperPortAllocatorSession::SendSessionRequest(
   headers << "X-StreamType: " << channel_name() << "\n\r";
   request_info.SetHeaders(headers.str());
 
-  int result = url_loader_->Open(request_info, pp::CompletionCallback(
+  int result = relay_url_loader_->Open(request_info, pp::CompletionCallback(
       &PepperPortAllocatorSession::UrlLoaderOpenCallback, this));
   DCHECK_EQ(result, PP_OK_COMPLETIONPENDING);
 }
@@ -119,7 +222,7 @@ void PepperPortAllocatorSession::OnUrlOpened(int32_t result) {
     return;
   }
 
-  pp::URLResponseInfo response = url_loader_->GetResponseInfo();
+  pp::URLResponseInfo response = relay_url_loader_->GetResponseInfo();
   DCHECK(!response.is_null());
   if (response.GetStatusCode() != 200) {
     LOG(WARNING) << "Received HTTP status code " << response.GetStatusCode();
@@ -128,15 +231,15 @@ void PepperPortAllocatorSession::OnUrlOpened(int32_t result) {
     return;
   }
 
-  body_.clear();
+  relay_response_body_.clear();
   ReadResponseBody();
 }
 
 void PepperPortAllocatorSession::ReadResponseBody() {
-  int pos = body_.size();
-  body_.resize(pos + kReadSize);
-  int result = url_loader_->ReadResponseBody(
-      &body_[pos], kReadSize, pp::CompletionCallback(
+  int pos = relay_response_body_.size();
+  relay_response_body_.resize(pos + kReadSize);
+  int result = relay_url_loader_->ReadResponseBody(
+      &relay_response_body_[pos], kReadSize, pp::CompletionCallback(
           &PepperPortAllocatorSession::UrlLoaderReadCallback, this));
   DCHECK_EQ(result, PP_OK_COMPLETIONPENDING);
 }
@@ -155,12 +258,22 @@ void PepperPortAllocatorSession::OnResponseBodyRead(int32_t result) {
 
   // Resize the buffer in case we've read less than was requested.
   CHECK_LE(result, kReadSize);
-  CHECK_GE(static_cast<int>(body_.size()), kReadSize);
-  body_.resize(body_.size() - kReadSize + result);
+  CHECK_GE(static_cast<int>(relay_response_body_.size()), kReadSize);
+  relay_response_body_.resize(relay_response_body_.size() - kReadSize + result);
 
   if (result == 0) {
-    // Finished reading the response.
-    ReceiveSessionResponse(std::string(body_.begin(), body_.end()));
+    // Finished reading the response. Submit it to
+    // HttpPortAllocatorSessionBase only after we've resolved STUN
+    // address. This is necessary because STUN and Relay parameters
+    // are stored together in PortConfiguration and
+    // ReceiveSessionResponse() doesn't save relay session
+    // configuration for the case we resolve STUN address later.
+    //
+    // TODO(sergeyu): Refactor HttpPortAllocatorSessionBase to fix this.
+    if (stun_address_resolved_) {
+      ReceiveSessionResponse(std::string(relay_response_body_.begin(),
+                                         relay_response_body_.end()));
+    }
     return;
   }
 
@@ -168,6 +281,15 @@ void PepperPortAllocatorSession::OnResponseBodyRead(int32_t result) {
 }
 
 // static
+void PepperPortAllocatorSession::HostResolverCallback(
+    void* user_data,
+    int32_t result) {
+  PepperPortAllocatorSession* object =
+      static_cast<PepperPortAllocatorSession*>(user_data);
+  object->OnStunAddressResolved(result);
+}
+
+// static
 void PepperPortAllocatorSession::UrlLoaderOpenCallback(
     void* user_data,
     int32_t result) {
@@ -215,9 +337,9 @@ PepperPortAllocator::~PepperPortAllocator() {
 cricket::PortAllocatorSession* PepperPortAllocator::CreateSession(
     const std::string& channel_name,
     int component) {
-   return new PepperPortAllocatorSession(
-       this, channel_name, component, stun_hosts(),
-       relay_hosts(), relay_token(), instance_);
+  return new PepperPortAllocatorSession(
+      this, channel_name, component, stun_hosts(),
+      relay_hosts(), relay_token(), instance_);
 }
 
 }  // namespace remoting