Avoid error messages from UDP sockets in Chromoting client.

remoting/client/plugin/pepper_packet_socket_factory.cc

Index: remoting/client/plugin/pepper_packet_socket_factory.cc
diff --git a/remoting/client/plugin/pepper_packet_socket_factory.cc b/remoting/client/plugin/pepper_packet_socket_factory.cc
index 90d7c61bd94cdb5b345ec0cf456be00776c1d66b..1d546451ea6cc385fb5614458ca2fdf8452f343a 100644
--- a/remoting/client/plugin/pepper_packet_socket_factory.cc
+++ b/remoting/client/plugin/pepper_packet_socket_factory.cc
@@ -26,6 +26,42 @@ const int kReceiveBufferSize = 65536;
 // reached under normal conditions.
 const int kMaxSendBufferSize = 256 * 1024;
 
+// Enum for different actions that can be taken after sendto() returns an error.
+enum ErrorAction {
+  ERROR_ACTION_FAIL,
+  ERROR_ACTION_IGNORE,
+  ERROR_ACTION_RETRY,
+};
+
+// Returns ErrorAction that must be taken after |error| returned from sendto().

[Wez, 2014/06/17 02:34:10]

nit: is returned (which triggers line-wrap, so I'd suggest "Returns ErrorAction
to perform if sendto() fails with |error|."

[Sergey Ulanov, 2014/06/17 02:54:13]

Done.

+ErrorAction GetErrorAction(int error) {
+  switch (error) {
+    // These errors are returned when one of the previous datagrams was sent to
+    // an unreachable address.

[Wez, 2014/06/17 02:34:10]

nit: Be explicit that the error could correspond to an earlier send, e.g. "UDP
is connectionless, so we may receive ICMP unreachable or reset errors for
previous sends to different addresses."

This makes me wonder whether the sendto() we just performed actually gets
performed even though the stack returns the error?

[Sergey Ulanov, 2014/06/17 02:54:13]

Done, though I think the previous comment is explicit enough about it: "...one
of the previous datagrams was sent..."
No, it doesn't.

+    case PP_ERROR_ADDRESS_UNREACHABLE:
+    case PP_ERROR_CONNECTION_RESET:
+      return ERROR_ACTION_RETRY;
+
+    // Target address is invalid.
+    case PP_ERROR_ADDRESS_INVALID:
+      return ERROR_ACTION_IGNORE;

[Wez, 2014/06/17 02:34:10]

Again, it would be good to clarify why we want to ignore this case; surely it
indicates that we've been sent a completely bogus address, which is a real
error?

[Sergey Ulanov, 2014/06/17 02:54:13]

Done.

+
+    // May be returned when the packet is blocked by local firewall (see
+    // https://code.google.com/p/webrtc/issues/detail?id=1207). The error is
+    // transient because the firewall may block only some target addresses.

[Wez, 2014/06/17 02:34:10]

nit: Suggest for the second sentence "The firewall may still allow us to send to
other addresses, so ignore the error for this particular send."

[Sergey Ulanov, 2014/06/17 02:54:13]

Done.

+    case PP_ERROR_NOACCESS:
+      return ERROR_ACTION_IGNORE;
+
+    // Indicates that the buffer in the network adapter is full. The socket is
+    // still usable.

[Wez, 2014/06/17 02:34:10]

nit: Suggest extending second sentence ".... is full, so drop this packet and
assume the socket is still usable."

[Sergey Ulanov, 2014/06/17 02:54:13]

Done.

+    case PP_ERROR_NOMEMORY:
+      return ERROR_ACTION_IGNORE;
+
+    default:
+      return ERROR_ACTION_FAIL;
+  }
+}
+
 class UdpPacketSocket : public talk_base::AsyncPacketSocket {
  public:
   explicit UdpPacketSocket(const pp::InstanceHandle& instance);
@@ -61,6 +97,7 @@ class UdpPacketSocket : public talk_base::AsyncPacketSocket {
 
     scoped_refptr<net::IOBufferWithSize> data;
     pp::NetAddress address;
+    bool retried;
   };
 
   void OnBindCompleted(int error);
@@ -102,7 +139,8 @@ UdpPacketSocket::PendingPacket::PendingPacket(
     int buffer_size,
     const pp::NetAddress& address)
     : data(new net::IOBufferWithSize(buffer_size)),
-      address(address) {
+      address(address),
+      retried(true) {
   memcpy(data->data(), buffer, buffer_size);
 }
 
@@ -177,7 +215,8 @@ void UdpPacketSocket::OnBindCompleted(int result) {
       DCHECK_EQ(result, PP_OK_COMPLETIONPENDING);
     }
   } else {
-    LOG(ERROR) << "Failed to bind UDP socket: " << result;
+    LOG(ERROR) << "Failed to bind UDP socket to " << local_address_.ToString()
+               << ", error: " << result;
   }
 }
 
@@ -281,25 +320,25 @@ void UdpPacketSocket::OnSendCompleted(int result) {
   send_pending_ = false;
 
   if (result < 0) {
-    LOG(ERROR) << "Send failed on a UDP socket: " << result;
-
-    // OS (e.g. OSX) may return EHOSTUNREACH when the peer has the
-    // same subnet address as the local host but connected to a
-    // different network. That error must be ingored because the
-    // socket may still be useful for other ICE canidadates (e.g. for
-    // STUN candidates with a different address). Unfortunately pepper
-    // interface currently returns PP_ERROR_FAILED for any error (see
-    // crbug.com/136406). It's not possible to distinguish that case
-    // from other errors and so we have to ingore all of them. This
-    // behavior matchers the libjingle's AsyncUDPSocket used by the
-    // host.
-    //
-    // TODO(sergeyu): Once implementation of the Pepper UDP interface
-    // is fixed, uncomment the code below, but ignore
-    // host-unreacheable error.
-
-    // error_ = EINVAL;
-    // return;
+    ErrorAction action = GetErrorAction(result);
+    switch (action) {
+      case ERROR_ACTION_FAIL:
+        LOG(ERROR) << "Send failed on a UDP socket: " << result;
+        error_ = EINVAL;
+        return;
+
+      case ERROR_ACTION_RETRY:
+        // Retry resending only once.
+        if (!send_queue_.front().retried) {
+          send_queue_.front().retried = true;
+          DoSend();
+          return;
+        }
+        break;
+
+      case ERROR_ACTION_IGNORE:
+        break;
+    }
   }
 
   send_queue_size_ -= send_queue_.front().data->size();