Update error-handling logic in ChromiumPacketSocketFactory.

PepperPacketSocketFactory has been updated to handle transient errors
properly in https://codereview.chromium.org/336113002/. This is
correpsonding change for ChromiumPacketSocketFactory. Particularly
ChromiumPacketSocketFactory now tries resending each packet after
EHOSTUNREACH.

Review URL: https://codereview.chromium.org/339503006

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@278007 0039d316-1c4b-4281-b951-d872f2087c98

remoting/client/plugin/pepper_packet_socket_factory.cc

@@ -7,10 +7,12 @@
 #include "base/bind.h"
 #include "base/logging.h"
 #include "net/base/io_buffer.h"
+#include "net/base/net_errors.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 "remoting/jingle_glue/socket_util.h"
 #include "third_party/libjingle/source/talk/base/asyncpacketsocket.h"
 
 namespace remoting {
@@ -26,41 +28,56 @@ 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 to perform if sendto() fails with |error|.
-ErrorAction GetErrorAction(int error) {
+int PepperErrorToNetError(int error) {
   switch (error) {
-    // UDP is connectionless, so we may receive ICMP unreachable or reset errors
-    // for previous sends to different addresses.
-    case PP_ERROR_ADDRESS_UNREACHABLE:
+    case PP_OK:
+      return net::OK;
+    case PP_OK_COMPLETIONPENDING:
+      return net::ERR_IO_PENDING;
+    case PP_ERROR_ABORTED:
+      return net::ERR_ABORTED;
+    case PP_ERROR_BADARGUMENT:
+      return net::ERR_INVALID_ARGUMENT;
+    case PP_ERROR_FILENOTFOUND:
+      return net::ERR_FILE_NOT_FOUND;
+    case PP_ERROR_TIMEDOUT:
+      return net::ERR_TIMED_OUT;
+    case PP_ERROR_FILETOOBIG:
+      return net::ERR_FILE_TOO_BIG;
+    case PP_ERROR_NOTSUPPORTED:
+      return net::ERR_NOT_IMPLEMENTED;
+    case PP_ERROR_NOMEMORY:
+      return net::ERR_OUT_OF_MEMORY;
+    case PP_ERROR_FILEEXISTS:
+      return net::ERR_FILE_EXISTS;
+    case PP_ERROR_NOSPACE:
+      return net::ERR_FILE_NO_SPACE;
+    case PP_ERROR_CONNECTION_CLOSED:
+      return net::ERR_CONNECTION_CLOSED;
     case PP_ERROR_CONNECTION_RESET:
-      return ERROR_ACTION_RETRY;
-
-    // Target address is invalid. The socket is still usable for different
-    // target addresses and the error can be ignored.
+      return net::ERR_CONNECTION_RESET;
+    case PP_ERROR_CONNECTION_REFUSED:
+      return net::ERR_CONNECTION_REFUSED;
+    case PP_ERROR_CONNECTION_ABORTED:
+      return net::ERR_CONNECTION_ABORTED;
+    case PP_ERROR_CONNECTION_FAILED:
+      return net::ERR_CONNECTION_FAILED;
+    case PP_ERROR_NAME_NOT_RESOLVED:
+      return net::ERR_NAME_NOT_RESOLVED;
     case PP_ERROR_ADDRESS_INVALID:
-      return ERROR_ACTION_IGNORE;
-
-    // May be returned when the packet is blocked by local firewall (see
-    // https://code.google.com/p/webrtc/issues/detail?id=1207). The firewall may
-    // still allow us to send to other addresses, so ignore the error for this
-    // particular send.
+      return net::ERR_ADDRESS_INVALID;
+    case PP_ERROR_ADDRESS_UNREACHABLE:
+      return net::ERR_ADDRESS_UNREACHABLE;
+    case PP_ERROR_CONNECTION_TIMEDOUT:
+      return net::ERR_CONNECTION_TIMED_OUT;
     case PP_ERROR_NOACCESS:
-      return ERROR_ACTION_IGNORE;
-
-    // Indicates that the buffer in the network adapter is full, so drop this
-    // packet and assume the socket is still usable.
-    case PP_ERROR_NOMEMORY:
-      return ERROR_ACTION_IGNORE;
-
+      return net::ERR_NETWORK_ACCESS_DENIED;
+    case PP_ERROR_MESSAGE_TOO_BIG:
+      return net::ERR_MSG_TOO_BIG;
+    case PP_ERROR_ADDRESS_IN_USE:
+      return net::ERR_ADDRESS_IN_USE;
     default:
-      return ERROR_ACTION_FAIL;
+      return net::ERR_FAILED;
   }
 }
 
@@ -322,14 +339,15 @@ void UdpPacketSocket::OnSendCompleted(int result) {
   send_pending_ = false;
 
   if (result < 0) {
-    ErrorAction action = GetErrorAction(result);
+    int net_error = PepperErrorToNetError(result);
+    SocketErrorAction action = GetSocketErrorAction(net_error);
     switch (action) {
-      case ERROR_ACTION_FAIL:
+      case SOCKET_ERROR_ACTION_FAIL:
         LOG(ERROR) << "Send failed on a UDP socket: " << result;
         error_ = EINVAL;
         return;
 
-      case ERROR_ACTION_RETRY:
+      case SOCKET_ERROR_ACTION_RETRY:
         // Retry resending only once.
         if (!send_queue_.front().retried) {
           send_queue_.front().retried = true;
@@ -338,7 +356,7 @@ void UdpPacketSocket::OnSendCompleted(int result) {
         }
         break;
 
-      case ERROR_ACTION_IGNORE:
+      case SOCKET_ERROR_ACTION_IGNORE:
         break;
     }
   }
@@ -365,7 +383,7 @@ void UdpPacketSocket::OnReadCompleted(int result, pp::NetAddress address) {
   }
 }
 
-  void UdpPacketSocket::HandleReadResult(int result, pp::NetAddress address) {
+void UdpPacketSocket::HandleReadResult(int result, pp::NetAddress address) {
   if (result > 0) {
     talk_base::SocketAddress socket_address;
     PpNetAddressToSocketAddress(address, &socket_address);

remoting/jingle_glue/chromium_socket_factory.cc

@@ -12,6 +12,7 @@
 #include "net/base/ip_endpoint.h"
 #include "net/base/net_errors.h"
 #include "net/udp/udp_server_socket.h"
+#include "remoting/jingle_glue/socket_util.h"
 #include "third_party/libjingle/source/talk/base/asyncpacketsocket.h"
 #include "third_party/libjingle/source/talk/base/nethelpers.h"
 
@@ -28,13 +29,6 @@ const int kReceiveBufferSize = 65536;
 // reached under normal conditions.
 const int kMaxSendBufferSize = 256 * 1024;
 
-// Defines set of transient errors. These errors are ignored when we get them
-// from sendto() calls.
-bool IsTransientError(int error) {
-  return error == net::ERR_ADDRESS_UNREACHABLE ||
-      error == net::ERR_ADDRESS_INVALID;
-}
-
 class UdpPacketSocket : public talk_base::AsyncPacketSocket {
  public:
   UdpPacketSocket();
@@ -66,6 +60,7 @@ class UdpPacketSocket : public talk_base::AsyncPacketSocket {
 
     scoped_refptr<net::IOBufferWithSize> data;
     net::IPEndPoint address;
+    bool retried;
   };
 
   void OnBindCompleted(int error);
@@ -100,7 +95,8 @@ UdpPacketSocket::PendingPacket::PendingPacket(
     int buffer_size,
     const net::IPEndPoint& address)
     : data(new net::IOBufferWithSize(buffer_size)),
-      address(address) {
+      address(address),
+      retried(false) {
   memcpy(data->data(), buffer, buffer_size);
 }
 
@@ -285,10 +281,24 @@ void UdpPacketSocket::OnSendCompleted(int result) {
   send_pending_ = false;
 
   if (result < 0) {
-    if (!IsTransientError(result)) {
-      LOG(ERROR) << "Send failed on a UDP socket: " << result;
-      error_ = EINVAL;
-      return;
+    SocketErrorAction action = GetSocketErrorAction(result);
+    switch (action) {
+      case SOCKET_ERROR_ACTION_FAIL:
+        LOG(ERROR) << "Send failed on a UDP socket: " << result;
+        error_ = EINVAL;
+        return;
+
+      case SOCKET_ERROR_ACTION_RETRY:
+        // Retry resending only once.
+        if (!send_queue_.front().retried) {
+          send_queue_.front().retried = true;
+          DoSend();
+          return;
+        }
+        break;
+
+      case SOCKET_ERROR_ACTION_IGNORE:
+        break;
     }
   }
 

remoting/jingle_glue/chromium_socket_factory_unittest.cc

@@ -37,6 +37,24 @@ class ChromiumSocketFactoryTest : public testing::Test,
     run_loop_.Quit();
   }
 
+  void VerifyCanSendAndReceive(talk_base::AsyncPacketSocket* sender) {
+    // UDP packets may be lost, so we have to retry sending it more than once.
+    const int kMaxAttempts = 3;
+    const base::TimeDelta kAttemptPeriod = base::TimeDelta::FromSeconds(1);
+    std::string test_packet("TEST PACKET");
+    int attempts = 0;
+    talk_base::PacketOptions options;
+    while (last_packet_.empty() && attempts++ < kMaxAttempts) {
+      sender->SendTo(test_packet.data(), test_packet.size(),
+                     socket_->GetLocalAddress(), options);
+      message_loop_.PostDelayedTask(FROM_HERE, run_loop_.QuitClosure(),
+                                    kAttemptPeriod);
+      run_loop_.Run();
+    }
+    EXPECT_EQ(test_packet, last_packet_);
+    EXPECT_EQ(sender->GetLocalAddress(), last_address_);
+  }
+
  protected:
   base::MessageLoopForIO message_loop_;
   base::RunLoop run_loop_;
@@ -49,32 +67,14 @@ class ChromiumSocketFactoryTest : public testing::Test,
 };
 
 TEST_F(ChromiumSocketFactoryTest, SendAndReceive) {
-  // UDP packets may be lost, so we have to retry sending it more than once.
-  const int kMaxAttempts = 3;
-  const base::TimeDelta kAttemptPeriod = base::TimeDelta::FromSeconds(1);
-
-  scoped_ptr<talk_base::AsyncPacketSocket> sending_socket;
-  talk_base::SocketAddress address;
-
-  sending_socket.reset(socket_factory_->CreateUdpSocket(
-      talk_base::SocketAddress("127.0.0.1", 0), 0, 0));
+  scoped_ptr<talk_base::AsyncPacketSocket> sending_socket(
+      socket_factory_->CreateUdpSocket(
+          talk_base::SocketAddress("127.0.0.1", 0), 0, 0));
   ASSERT_TRUE(sending_socket.get() != NULL);
   EXPECT_EQ(sending_socket->GetState(),
             talk_base::AsyncPacketSocket::STATE_BOUND);
-  address = sending_socket->GetLocalAddress();
-
-  std::string test_packet("TEST PACKET");
-  int attempts = 0;
-  talk_base::PacketOptions options;
-  while (last_packet_.empty() && attempts++ < kMaxAttempts) {
-    sending_socket->SendTo(test_packet.data(), test_packet.size(),
-                           socket_->GetLocalAddress(), options);
-    message_loop_.PostDelayedTask(FROM_HERE, run_loop_.QuitClosure(),
-                                  kAttemptPeriod);
-    run_loop_.Run();
-  }
-  EXPECT_EQ(test_packet, last_packet_);
-  EXPECT_EQ(address, last_address_);
+
+  VerifyCanSendAndReceive(sending_socket.get());
 }
 
 TEST_F(ChromiumSocketFactoryTest, SetOptions) {
@@ -93,4 +93,21 @@ TEST_F(ChromiumSocketFactoryTest, PortRange) {
   EXPECT_LE(socket_->GetLocalAddress().port(), kMaxPort);
 }
 
+TEST_F(ChromiumSocketFactoryTest, TransientError) {
+  scoped_ptr<talk_base::AsyncPacketSocket> sending_socket(
+      socket_factory_->CreateUdpSocket(
+          talk_base::SocketAddress("127.0.0.1", 0), 0, 0));
+  std::string test_packet("TEST");
+
+  // Try sending a packet to an IPv6 address from a socket that's bound to an
+  // IPv4 address. This send is expected to fail, but the socket should still be
+  // functional.
+  sending_socket->SendTo(test_packet.data(), test_packet.size(),
+                         talk_base::SocketAddress("::1", 0),
+                         talk_base::PacketOptions());
+
+  // Verify that socket is still usable.
+  VerifyCanSendAndReceive(sending_socket.get());
+}
+
 }  // namespace remoting

remoting/jingle_glue/socket_util.cc

+// Copyright 2014 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/jingle_glue/socket_util.h"
+
+#include "net/base/net_errors.h"
+
+namespace remoting {
+
+SocketErrorAction GetSocketErrorAction(int error) {
+  switch (error) {
+    // UDP is connectionless, so we may receive ICMP unreachable or reset errors
+    // for previous sends to different addresses.
+    case net::ERR_ADDRESS_UNREACHABLE:
+    case net::ERR_CONNECTION_RESET:
+      return SOCKET_ERROR_ACTION_RETRY;
+
+    // Target address is invalid. The socket is still usable for different
+    // target addresses and the error can be ignored.
+    case net::ERR_ADDRESS_INVALID:
+      return SOCKET_ERROR_ACTION_IGNORE;
+
+    // May be returned when the packet is blocked by local firewall (see
+    // https://code.google.com/p/webrtc/issues/detail?id=1207). The firewall may
+    // still allow us to send to other addresses, so ignore the error for this
+    // particular send.
+    case net::ERR_ACCESS_DENIED:
+      return SOCKET_ERROR_ACTION_IGNORE;
+
+    // Indicates that the buffer in the network adapter is full, so drop this
+    // packet and assume the socket is still usable.
+    case net::ERR_OUT_OF_MEMORY:
+      return SOCKET_ERROR_ACTION_IGNORE;
+
+    default:
+      return SOCKET_ERROR_ACTION_FAIL;
+  }
+}
+
+
+}  // namespace remoting

remoting/jingle_glue/socket_util.h

+// Copyright 2014 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.
+
+#ifndef REMOTING_JINGLE_GLUE_SOCKET_UTIL_H_
+#define REMOTING_JINGLE_GLUE_SOCKET_UTIL_H_
+
+namespace remoting {
+
+// Enum for different actions that can be taken after sendto() returns an error.
+enum SocketErrorAction {
+  SOCKET_ERROR_ACTION_FAIL,
+  SOCKET_ERROR_ACTION_IGNORE,
+  SOCKET_ERROR_ACTION_RETRY,
+};
+
+// Returns true if |error| must be ignored when returned from sendto(). |retry|
+// is set set when sentto() should be called for the same packet again.
+SocketErrorAction GetSocketErrorAction(int error);
+
+}  // namespace remoting
+
+#endif  // REMOTING_JINGLE_GLUE_SOCKET_UTIL_H_

remoting/remoting_srcs.gypi

@@ -78,6 +78,8 @@
       'jingle_glue/server_log_entry.cc',
       'jingle_glue/server_log_entry.h',
       'jingle_glue/signal_strategy.h',
+      'jingle_glue/socket_util.cc',
+      'jingle_glue/socket_util.h',
       'jingle_glue/xmpp_signal_strategy.cc',
       'jingle_glue/xmpp_signal_strategy.h',
       'protocol/audio_reader.cc',