"Reland" of r241209. DialServiceImpl now binds to all ipv4 and interfaces instead of just the first

"Reland" of r241209.

DialServiceImpl now binds to all ipv4 interfaces instead of just the first ipv4 found. Dedup is done using address family + interface index.
It now also checks if there are any other open sockets when notifying observers on error and sends a different error code (DIAL_SERVICE_NO_INTERFACES instead of the usual DIAL_SERVICE_SOCKET_ERROR) if there are no other open sockets. The error codes are treated the same upstream currently. Later we should make sure DIAL_SERVICE_SOCKET_ERROR doesn't clear the device list.

Added unit tests for multiple interfaces and open sockets checking.

TEST=unit tests, did some manual testing at the lab.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=242090

[Alpha Left Google, 2013-12-19 00:28:47]

On 2013/12/19 00:04:29, imcheng1 wrote:

LGTM.

[commit-bot: I haz the power, 2013-12-19 02:21:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/imcheng@chromium.org/117853004/40001

[commit-bot: I haz the power, 2013-12-19 02:21:14]

Failed to apply patch for chrome/browser/extensions/api/dial/dial_service.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file chrome/browser/extensions/api/dial/dial_service.cc
  Hunk #2 FAILED at 134.
  Hunk #3 FAILED at 158.
  Hunk #4 FAILED at 263.
  Hunk #5 FAILED at 284.
  Hunk #6 FAILED at 341.
  Hunk #7 FAILED at 353.
  Hunk #8 FAILED at 365.
  Hunk #9 FAILED at 375.
  Hunk #10 FAILED at 449.
  Hunk #11 FAILED at 481.
  10 out of 11 hunks FAILED -- saving rejects to file
chrome/browser/extensions/api/dial/dial_service.cc.rej

Patch:       chrome/browser/extensions/api/dial/dial_service.cc
Index: chrome/browser/extensions/api/dial/dial_service.cc
diff --git a/chrome/browser/extensions/api/dial/dial_service.cc
b/chrome/browser/extensions/api/dial/dial_service.cc
index
f7f8112487036375de952dd597e88c60da11bcd1..96f5402500c0d4dd55bdad893c88adfd9665b1fc
100644
--- a/chrome/browser/extensions/api/dial/dial_service.cc
+++ b/chrome/browser/extensions/api/dial/dial_service.cc
@@ -5,6 +5,8 @@
 #include "chrome/browser/extensions/api/dial/dial_service.h"
 
 #include <algorithm>
+#include <set>
+#include <utility>
 
 #include "base/basictypes.h"
 #include "base/callback.h"
@@ -132,7 +134,6 @@ IPAddressNumber GetBestBindAddressByType(
     return IPAddressNumber();
   }
   if (bind_ip_address.size() != net::kIPv4AddressSize) {
-    LOG(ERROR) << "Default network is not using IPv4.";
     return IPAddressNumber();
   }
 
@@ -156,102 +157,36 @@ IPAddressNumber GetBestBindAddressChromeOS() {
 
 }  // namespace
 
-DialServiceImpl::DialServiceImpl(net::NetLog* net_log)
-  : is_writing_(false),
-    is_reading_(false),
-    discovery_active_(false),
-    num_requests_sent_(0),
-    max_requests_(kDialMaxRequests),
-    finish_delay_(TimeDelta::FromMilliseconds((kDialMaxRequests - 1) *
-                                              kDialRequestIntervalMillis) +
-                  TimeDelta::FromSeconds(kDialResponseTimeoutSecs)),
-    request_interval_(TimeDelta::FromMilliseconds(kDialRequestIntervalMillis))
{
-  IPAddressNumber address;
-  bool result = net::ParseIPLiteralToNumber(kDialRequestAddress, &address);
-  DCHECK(result);
-  send_address_ = IPEndPoint(address, kDialRequestPort);
-  send_buffer_ = new StringIOBuffer(BuildRequest());
-  net_log_ = net_log;
-  net_log_source_.type = net::NetLog::SOURCE_UDP_SOCKET;
-  net_log_source_.id = net_log_->NextID();
-}
-
-DialServiceImpl::~DialServiceImpl() {
-  DCHECK(thread_checker_.CalledOnValidThread());
+DialServiceImpl::DialSocket::DialSocket(
+    const base::Closure& discovery_request_cb,
+    const base::Callback<void(const DialDeviceData&)>& device_discovered_cb,
+    const base::Closure& on_error_cb)
+    : discovery_request_cb_(discovery_request_cb),
+      device_discovered_cb_(device_discovered_cb),
+      on_error_cb_(on_error_cb),
+      is_writing_(false),
+      is_reading_(false) {
 }
 
-void DialServiceImpl::AddObserver(Observer* observer) {
+DialServiceImpl::DialSocket::~DialSocket() {
   DCHECK(thread_checker_.CalledOnValidThread());
-  observer_list_.AddObserver(observer);
 }
 
-void DialServiceImpl::RemoveObserver(Observer* observer) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  observer_list_.RemoveObserver(observer);
-}
-
-bool DialServiceImpl::HasObserver(Observer* observer) {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  return observer_list_.HasObserver(observer);
-}
-
-bool DialServiceImpl::Discover() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  if (discovery_active_)
-    return false;
-  discovery_active_ = true;
-
-  VLOG(2) << "Discovery started.";
-
-  StartDiscovery();
-  return true;
-}
-
-void DialServiceImpl::StartDiscovery() {
-  DCHECK(thread_checker_.CalledOnValidThread());
-  DCHECK(discovery_active_);
-  if (socket_.get())
-    return;
-
-#if defined(OS_CHROMEOS)
-  // The ChromeOS specific version of getting network interfaces does not
-  // require trampolining to another thread, and contains additional interface
-  // information such as interface types (i.e. wifi vs cellular).
-  BindSocketAndSendRequest(GetBestBindAddressChromeOS());
-#else
-  BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, base::Bind(
-      &GetNetworkListOnFileThread,
-      base::MessageLoopProxy::current(), base::Bind(
-          &DialServiceImpl::SendNetworkList, AsWeakPtr())));
-#endif
-}
-
-bool DialServiceImpl::BindSocketAndSendRequest(
-      const IPAddressNumber& bind_ip_address) {
+bool DialServiceImpl::DialSocket::CreateAndBindSocket(
+    const IPAddressNumber& bind_ip_address,
+    net::NetLog* net_log,
+    net::NetLog::Source net_log_source) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(!socket_.get());
-
-  if (bind_ip_address.size() == 0) {
-    DVLOG(1) << "Could not find a valid interface to bind.";
-    FinishDiscovery();
-    return false;
-  }
+  DCHECK(bind_ip_address.size() == net::kIPv4AddressSize);
 
   net::RandIntCallback rand_cb = base::Bind(&base::RandInt);
   socket_.reset(new UDPSocket(net::DatagramSocket::RANDOM_BIND,
                               rand_cb,
-                              net_log_,
-                              net_log_source_));
+                              net_log,
+                              net_log_source));
   socket_->AllowBroadcast();
 
-  // Schedule a timer to finish the discovery process (and close the socket).
-  if (finish_delay_ > TimeDelta::FromSeconds(0)) {
-    finish_timer_.Start(FROM_HERE,
-                        finish_delay_,
-                        this,
-                        &DialServiceImpl::FinishDiscovery);
-  }
-
   // 0 means bind a random port
   IPEndPoint address(bind_ip_address, 0);
 
@@ -261,18 +196,12 @@ bool DialServiceImpl::BindSocketAndSendRequest(
   DCHECK(socket_.get());
 
   recv_buffer_ = new IOBufferWithSize(kDialRecvBufferSize);
-  if (!ReadSocket())
-    return false;
-  SendOneRequest();
-  return true;
+  return ReadSocket();
 }
 
-void DialServiceImpl::SendOneRequest() {
-  if (num_requests_sent_ == max_requests_) {
-    request_timer_.Stop();
-    return;
-  }
-  num_requests_sent_++;
+void DialServiceImpl::DialSocket::SendOneRequest(
+    const net::IPEndPoint& send_address,
+    const scoped_refptr<net::StringIOBuffer>& send_buffer) {
   if (!socket_.get()) {
     DLOG(WARNING) << "Socket not connected.";
     return;
@@ -282,45 +211,60 @@ void DialServiceImpl::SendOneRequest() {
     VLOG(2) << "Already writing.";
     return;
   }
-  VLOG(2) << "Sending request " << num_requests_sent_ << "/"
-          << max_requests_;
+
   is_writing_ = true;
   int result = socket_->SendTo(
-      send_buffer_.get(), send_buffer_->size(), send_address_,
-      base::Bind(&DialServiceImpl::OnSocketWrite, AsWeakPtr()));
+      send_buffer.get(), send_buffer->size(), send_address,
+      base::Bind(&DialServiceImpl::DialSocket::OnSocketWrite,
+                 base::Unretained(this),
+                 send_buffer->size()));
   bool result_ok = CheckResult("SendTo", result);
   if (result_ok && result > 0) {
     // Synchronous write.
-    OnSocketWrite(result);
+    OnSocketWrite(send_buffer->size(), result);
   }
 }
 
-void DialServiceImpl::OnSocketWrite(int result) {
+bool DialServiceImpl::DialSocket::IsClosed() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  return !socket_.get();
+}
+
+bool DialServiceImpl::DialSocket::CheckResult(const char* operation,
+                                              int result) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  VLOG(2) << "Operation " << operation << " result " << result;
+  if (result < net::OK && result != net::ERR_IO_PENDING) {
+    Close();
+    std::string error_str(net::ErrorToString(result));
+    DVLOG(0) << "dial socket error: " << error_str;
+    on_error_cb_.Run();
+    return false;
+  }
+  return true;
+}
+
+void DialServiceImpl::DialSocket::Close() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  is_reading_ = false;
+  is_writing_ = false;
+  socket_.reset();
+}
+
+void DialServiceImpl::DialSocket::OnSocketWrite(int send_buffer_size,
+                                                int result) {
   DCHECK(thread_checker_.CalledOnValidThread());
   is_writing_ = false;
   if (!CheckResult("OnSocketWrite", result))
     return;
-
-  if (result != send_buffer_->size()) {
+  if (result != send_buffer_size) {
     DLOG(ERROR) << "Sent " << result << " chars, expected "
-               << send_buffer_->size() << " chars";
-  }
-  // If discovery is inactive, no reason to notify observers.
-  if (!discovery_active_) {
-    VLOG(2) << "Request sent after discovery finished.  Ignoring.";
-    return;
-  }
-  FOR_EACH_OBSERVER(Observer, observer_list_, OnDiscoveryRequest(this));
-  // If we need to send additional requests, schedule a timer to do so.
-  if (num_requests_sent_ < max_requests_ && num_requests_sent_ == 1) {
-    request_timer_.Start(FROM_HERE,
-                         request_interval_,
-                         this,
-                         &DialServiceImpl::SendOneRequest);
+                << send_buffer_size << " chars";
   }
+  discovery_request_cb_.Run();
 }
 
-bool DialServiceImpl::ReadSocket() {
+bool DialServiceImpl::DialSocket::ReadSocket() {
   DCHECK(thread_checker_.CalledOnValidThread());
   if (!socket_.get()) {
     DLOG(WARNING) << "Socket not connected.";
@@ -339,7 +283,8 @@ bool DialServiceImpl::ReadSocket() {
     result = socket_->RecvFrom(
         recv_buffer_.get(),
         kDialRecvBufferSize, &recv_address_,
-        base::Bind(&DialServiceImpl::OnSocketRead, AsWeakPtr()));
+        base::Bind(&DialServiceImpl::DialSocket::OnSocketRead,
+                   base::Unretained(this)));
     result_ok = CheckResult("RecvFrom", result);
     if (result != net::ERR_IO_PENDING)
       is_reading_ = false;
@@ -351,7 +296,7 @@ bool DialServiceImpl::ReadSocket() {
   return result_ok;
 }
 
-void DialServiceImpl::OnSocketRead(int result) {
+void DialSe…
(message too large)

[imcheng, 2013-12-19 13:38:07]

Hey Alpha, I fixed the merge issue.

Mark, can you please take a look as well? Thanks.

[mark a. foltz, 2013-12-19 17:27:41]

LGTM

https://codereview.chromium.org/117853004/diff/200001/chrome/browser/extensio...
File chrome/browser/extensions/api/dial/dial_service_unittest.cc (right):

https://codereview.chromium.org/117853004/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/dial/dial_service_unittest.cc:70: EXPECT_EQ(1u,
dial_service_.dial_sockets_.size());
I don't think it's necessary to designate constants as unsigned here.

https://codereview.chromium.org/117853004/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/dial/dial_service_unittest.cc:98: EXPECT_EQ(3u,
dial_service_.dial_sockets_.size());
Ditto.

[Alpha Left Google, 2013-12-19 19:22:26]

On 2013/12/19 17:27:41, mark a. foltz wrote:
> LGTM
> 
>
https://codereview.chromium.org/117853004/diff/200001/chrome/browser/extensio...
> File chrome/browser/extensions/api/dial/dial_service_unittest.cc (right):
> 
>
https://codereview.chromium.org/117853004/diff/200001/chrome/browser/extensio...
> chrome/browser/extensions/api/dial/dial_service_unittest.cc:70: EXPECT_EQ(1u,
> dial_service_.dial_sockets_.size());
> I don't think it's necessary to designate constants as unsigned here.
> 
>
https://codereview.chromium.org/117853004/diff/200001/chrome/browser/extensio...
> chrome/browser/extensions/api/dial/dial_service_unittest.cc:98: EXPECT_EQ(3u,
> dial_service_.dial_sockets_.size());
> Ditto.

RSLGTM given Mark's LGTM.

[commit-bot: I haz the power, 2013-12-19 19:25:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/imcheng@chromium.org/117853004/200001

[commit-bot: I haz the power, 2013-12-20 00:44:14]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-12-20 00:49:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/imcheng@chromium.org/117853004/200001

[commit-bot: I haz the power, 2013-12-20 09:05:07]

Retried try job too often on linux_aura for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_aura...

[commit-bot: I haz the power, 2013-12-20 11:49:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/imcheng@chromium.org/117853004/200001

[commit-bot: I haz the power, 2013-12-20 13:14:10]

Change committed as 242090