Avoid error messages from UDP sockets in Chromoting client.

remoting/client/plugin/pepper_packet_socket_factory.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "remoting/client/plugin/pepper_packet_socket_factory.h"
 
 #include "base/bind.h"
 #include "base/logging.h"
 #include "net/base/io_buffer.h"
 #include "ppapi/cpp/net_address.h"
 #include "ppapi/cpp/udp_socket.h"
 #include "ppapi/utility/completion_callback_factory.h"
 #include "remoting/client/plugin/pepper_util.h"
 #include "third_party/libjingle/source/talk/base/asyncpacketsocket.h"
 
 namespace remoting {
 
 namespace {
 
 // Size of the buffer to allocate for RecvFrom().
 const int kReceiveBufferSize = 65536;
 
 // Maximum amount of data in the send buffers. This is necessary to
 // prevent out-of-memory crashes if the caller sends data faster than
 // Pepper's UDP API can handle it. This maximum should never be
 // 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);
   virtual ~UdpPacketSocket();
 
   // |min_port| and |max_port| are set to zero if the port number
   // should be assigned by the OS.
   bool Init(const talk_base::SocketAddress& local_address,
             int min_port,
             int max_port);
@@ skipping 15 matching lines @@
   virtual void SetError(int error) OVERRIDE;
 
  private:
   struct PendingPacket {
     PendingPacket(const void* buffer,
                   int buffer_size,
                   const pp::NetAddress& address);
 
     scoped_refptr<net::IOBufferWithSize> data;
     pp::NetAddress address;
+    bool retried;
   };
 
   void OnBindCompleted(int error);
 
   void DoSend();
   void OnSendCompleted(int result);
 
   void DoRead();
   void OnReadCompleted(int result, pp::NetAddress address);
   void HandleReadResult(int result, pp::NetAddress address);
@@ skipping 21 matching lines @@
   pp::CompletionCallbackFactory<UdpPacketSocket> callback_factory_;
 
   DISALLOW_COPY_AND_ASSIGN(UdpPacketSocket);
 };
 
 UdpPacketSocket::PendingPacket::PendingPacket(
     const void* buffer,
     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);
 }
 
 UdpPacketSocket::UdpPacketSocket(const pp::InstanceHandle& instance)
     : instance_(instance),
       socket_(instance),
       state_(STATE_CLOSED),
       error_(0),
       min_port_(0),
       max_port_(0),
@@ skipping 54 matching lines @@
     ++min_port_;
     pp::NetAddress pp_local_address;
     if (SocketAddressToPpNetAddressWithPort(
             instance_, local_address_, &pp_local_address, min_port_)) {
       pp::CompletionCallback callback =
           callback_factory_.NewCallback(&UdpPacketSocket::OnBindCompleted);
       int result = socket_.Bind(pp_local_address, callback);
       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;
   }
 }
 
 talk_base::SocketAddress UdpPacketSocket::GetLocalAddress() const {
   DCHECK_EQ(state_, STATE_BOUND);
   return local_address_;
 }
 
 talk_base::SocketAddress UdpPacketSocket::GetRemoteAddress() const {
   // UDP sockets are not connected - this method should never be called.
@@ skipping 83 matching lines @@
   if (result == PP_ERROR_ABORTED) {
     // Send is aborted when the socket is being destroyed.
     // |send_queue_| may be already destroyed, it's not safe to access
     // it here.
     return;
   }
 
   send_pending_ = false;
 
   if (result < 0) {
-    LOG(ERROR) << "Send failed on a UDP socket: " << result;
+    ErrorAction action = GetErrorAction(result);
+    switch (action) {
+      case ERROR_ACTION_FAIL:
+        LOG(ERROR) << "Send failed on a UDP socket: " << result;
+        error_ = EINVAL;
+        return;
 
-    // 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.
+      case ERROR_ACTION_RETRY:
+        // Retry resending only once.
+        if (!send_queue_.front().retried) {
+          send_queue_.front().retried = true;
+          DoSend();
+          return;
+        }
+        break;
 
-    // error_ = EINVAL;
-    // return;
+      case ERROR_ACTION_IGNORE:
+        break;
+    }
   }
 
   send_queue_size_ -= send_queue_.front().data->size();
   send_queue_.pop_front();
   DoSend();
 }
 
 void UdpPacketSocket::DoRead() {
   receive_buffer_.resize(kReceiveBufferSize);
   pp::CompletionCallbackWithOutput<pp::NetAddress> callback =
@@ skipping 63 matching lines @@
   return NULL;
 }
 
 talk_base::AsyncResolverInterface*
 PepperPacketSocketFactory::CreateAsyncResolver() {
   NOTREACHED();
   return NULL;
 }
 
 }  // namespace remoting