Cleanup error handling in the client plugin.

- Added new ErrorCode enum to pass error codes everywhere except between 
  the webapp and the plugin.
- Signaling timeout when connection now is interpreted as disconnect instead of
  error condition (see bug 112739).
- Fixed webapp to properly handle unknown error codes.

BUG=112739


Review URL: http://codereview.chromium.org/9567033

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

remoting/client/chromoting_client.cc

@@ -166,7 +166,7 @@ void ChromotingClient::DispatchPacket() {
 
 void ChromotingClient::OnConnectionState(
     protocol::ConnectionToHost::State state,
-    protocol::ConnectionToHost::Error error) {
+    protocol::ErrorCode error) {
   DCHECK(message_loop()->BelongsToCurrentThread());
   VLOG(1) << "ChromotingClient::OnConnectionState(" << state << ")";
   if (state == protocol::ConnectionToHost::CONNECTED)

remoting/client/chromoting_client.h

@@ -52,7 +52,7 @@ class ChromotingClient : public protocol::ConnectionToHost::HostEventCallback,
   // ConnectionToHost::HostEventCallback implementation.
   virtual void OnConnectionState(
       protocol::ConnectionToHost::State state,
-      protocol::ConnectionToHost::Error error) OVERRIDE;
+      protocol::ErrorCode error) OVERRIDE;
 
   // VideoStub implementation.
   virtual void ProcessVideoPacket(const VideoPacket* packet,

remoting/client/chromoting_view.h

@@ -25,7 +25,7 @@ class ChromotingView {
 
   // Record the update the state of the connection, updating the UI as needed.
   virtual void SetConnectionState(protocol::ConnectionToHost::State state,
-                                  protocol::ConnectionToHost::Error error) = 0;
+                                  protocol::ErrorCode error) = 0;
 };
 
 }  // namespace remoting

remoting/client/plugin/pepper_view.cc

@@ -31,18 +31,30 @@ namespace {
 // The maximum number of image buffers to be allocated at any point of time.
 const size_t kMaxPendingBuffersCount = 2;
 
+// TODO(sergeyu): Ideally we should just pass ErrorCode to the webapp
+// and let it handle it, but it would be hard to fix it now because
+// client plugin and webapp versions may not be in sync. It should be
+// easy to do after we are finished moving the client plugin to NaCl.
 ChromotingInstance::ConnectionError ConvertConnectionError(
-    protocol::ConnectionToHost::Error error) {
+    protocol::ErrorCode error) {
   switch (error) {
-    case protocol::ConnectionToHost::OK:
+    case protocol::OK:
       return ChromotingInstance::ERROR_NONE;
-    case protocol::ConnectionToHost::HOST_IS_OFFLINE:
+
+    case protocol::PEER_IS_OFFLINE:
       return ChromotingInstance::ERROR_HOST_IS_OFFLINE;
-    case protocol::ConnectionToHost::SESSION_REJECTED:
+
+    case protocol::SESSION_REJECTED:
+    case protocol::AUTHENTICATION_FAILED:
       return ChromotingInstance::ERROR_SESSION_REJECTED;
-    case protocol::ConnectionToHost::INCOMPATIBLE_PROTOCOL:
+
+    case protocol::INCOMPATIBLE_PROTOCOL:
       return ChromotingInstance::ERROR_INCOMPATIBLE_PROTOCOL;
-    case protocol::ConnectionToHost::NETWORK_FAILURE:
+
+    case protocol::CHANNEL_CONNECTION_ERROR:
+    case protocol::SIGNALING_ERROR:
+    case protocol::SIGNALING_TIMEOUT:
+    case protocol::UNKNOWN_ERROR:
       return ChromotingInstance::ERROR_NETWORK_FAILURE;
   }
   DLOG(FATAL) << "Unknown error code" << error;
@@ -97,7 +109,7 @@ void PepperView::TearDown() {
 }
 
 void PepperView::SetConnectionState(protocol::ConnectionToHost::State state,
-                                    protocol::ConnectionToHost::Error error) {
+                                    protocol::ErrorCode error) {
   DCHECK(context_->main_message_loop()->BelongsToCurrentThread());
 
   switch (state) {

remoting/client/plugin/pepper_view.h

@@ -38,7 +38,7 @@ class PepperView : public ChromotingView,
   virtual void TearDown() OVERRIDE;
   virtual void SetConnectionState(
       protocol::ConnectionToHost::State state,
-      protocol::ConnectionToHost::Error error) OVERRIDE;
+      protocol::ErrorCode error) OVERRIDE;
 
   // FrameConsumer implementation.
   virtual void ApplyBuffer(const SkISize& view_size,

remoting/host/client_session.cc

@@ -109,10 +109,10 @@ void ClientSession::OnConnectionClosed(
 
 void ClientSession::OnConnectionFailed(
     protocol::ConnectionToClient* connection,
-    protocol::Session::Error error) {
+    protocol::ErrorCode error) {
   DCHECK(CalledOnValidThread());
   DCHECK_EQ(connection_.get(), connection);
-  if (error == protocol::Session::AUTHENTICATION_FAILED)
+  if (error == protocol::AUTHENTICATION_FAILED)
     event_handler_->OnSessionAuthenticationFailed(this);
   // TODO(sergeyu): Log failure reason?
   event_handler_->OnSessionClosed(this);

remoting/host/client_session.h

@@ -74,7 +74,7 @@ class ClientSession : public protocol::HostStub,
   virtual void OnConnectionClosed(
       protocol::ConnectionToClient* connection) OVERRIDE;
   virtual void OnConnectionFailed(protocol::ConnectionToClient* connection,
-                                  protocol::Session::Error error) OVERRIDE;
+                                  protocol::ErrorCode error) OVERRIDE;
   virtual void OnSequenceNumberUpdated(
       protocol::ConnectionToClient* connection, int64 sequence_number) OVERRIDE;
   virtual void OnRouteChange(

remoting/protocol/connection_to_client.h

@@ -45,7 +45,7 @@ class ConnectionToClient : public base::NonThreadSafe {
 
     // Called when the network connection has failed.
     virtual void OnConnectionFailed(ConnectionToClient* connection,
-                                    Session::Error error) = 0;
+                                    ErrorCode error) = 0;
 
     // Called when sequence number is updated.
     virtual void OnSequenceNumberUpdated(ConnectionToClient* connection,

remoting/protocol/connection_to_client_unittest.cc

-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// 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.
 
@@ -100,9 +100,8 @@ TEST_F(ConnectionToClientTest, StateChange) {
   session_->state_change_callback().Run(protocol::Session::CLOSED);
   message_loop_.RunAllPending();
 
-  EXPECT_CALL(handler_, OnConnectionFailed(
-      viewer_.get(), Session::SESSION_REJECTED));
-  session_->set_error(Session::SESSION_REJECTED);
+  EXPECT_CALL(handler_, OnConnectionFailed(viewer_.get(), SESSION_REJECTED));
+  session_->set_error(SESSION_REJECTED);
   session_->state_change_callback().Run(protocol::Session::FAILED);
   message_loop_.RunAllPending();
 }

remoting/protocol/connection_to_host.cc

@@ -16,6 +16,7 @@
 #include "remoting/protocol/client_control_dispatcher.h"
 #include "remoting/protocol/client_event_dispatcher.h"
 #include "remoting/protocol/client_stub.h"
+#include "remoting/protocol/errors.h"
 #include "remoting/protocol/jingle_session_manager.h"
 #include "remoting/protocol/pepper_transport_factory.h"
 #include "remoting/protocol/video_reader.h"
@@ -114,7 +115,7 @@ void ConnectionToHost::OnSignalStrategyStateChange(
     VLOG(1) << "Connected as: " << signal_strategy_->GetLocalJid();
   } else if (state == SignalStrategy::DISCONNECTED) {
     VLOG(1) << "Connection closed.";
-    CloseOnError(NETWORK_FAILURE);
+    CloseOnError(SIGNALING_ERROR);
   }
 }
 
@@ -176,24 +177,19 @@ void ConnectionToHost::OnSessionStateChange(
       break;
 
     case Session::FAILED:
-      switch (session_->error()) {
-        case Session::PEER_IS_OFFLINE:
-          CloseOnError(HOST_IS_OFFLINE);
-          break;
-        case Session::SESSION_REJECTED:
-        case Session::AUTHENTICATION_FAILED:
-          CloseOnError(SESSION_REJECTED);
-          break;
-        case Session::INCOMPATIBLE_PROTOCOL:
-          CloseOnError(INCOMPATIBLE_PROTOCOL);
-          break;
-        case Session::CHANNEL_CONNECTION_ERROR:
-        case Session::UNKNOWN_ERROR:
-          CloseOnError(NETWORK_FAILURE);
-          break;
-        case Session::OK:
-          DLOG(FATAL) << "Error code isn't set";
-          CloseOnError(NETWORK_FAILURE);
+      // If we were connected then treat signaling timeout error as if
+      // the connection was closed by the peer.
+      //
+      // TODO(sergeyu): This logic belongs to the webapp, but we
+      // currently don't expose this error code to the webapp, and it
+      // would be hard to add it because client plugin and webapp
+      // versions may not be in sync. It should be easy to do after we
+      // are finished moving the client plugin to NaCl.
+      if (state_ == CONNECTED && session_->error() == SIGNALING_TIMEOUT) {
+        CloseChannels();
+        SetState(CLOSED, OK);
+      } else {
+        CloseOnError(session_->error());
       }
       break;
   }
@@ -202,7 +198,7 @@ void ConnectionToHost::OnSessionStateChange(
 void ConnectionToHost::OnChannelInitialized(bool successful) {
   if (!successful) {
     LOG(ERROR) << "Failed to connect video channel";
-    CloseOnError(NETWORK_FAILURE);
+    CloseOnError(CHANNEL_CONNECTION_ERROR);
     return;
   }
 
@@ -220,7 +216,7 @@ void ConnectionToHost::NotifyIfChannelsReady() {
   }
 }
 
-void ConnectionToHost::CloseOnError(Error error) {
+void ConnectionToHost::CloseOnError(ErrorCode error) {
   CloseChannels();
   SetState(FAILED, error);
 }
@@ -232,7 +228,7 @@ void ConnectionToHost::CloseChannels() {
   video_reader_.reset();
 }
 
-void ConnectionToHost::SetState(State state, Error error) {
+void ConnectionToHost::SetState(State state, ErrorCode error) {
   DCHECK(message_loop_->BelongsToCurrentThread());
   // |error| should be specified only when |state| is set to FAILED.
   DCHECK(state == FAILED || error == OK);

remoting/protocol/connection_to_host.h

@@ -12,6 +12,7 @@
 #include "base/memory/scoped_ptr.h"
 #include "remoting/jingle_glue/signal_strategy.h"
 #include "remoting/proto/internal.pb.h"
+#include "remoting/protocol/errors.h"
 #include "remoting/protocol/input_filter.h"
 #include "remoting/protocol/message_reader.h"
 #include "remoting/protocol/session.h"
@@ -52,20 +53,12 @@ class ConnectionToHost : public SignalStrategy::Listener,
     CLOSED,
   };
 
-  enum Error {
-    OK,
-    HOST_IS_OFFLINE,
-    SESSION_REJECTED,
-    INCOMPATIBLE_PROTOCOL,
-    NETWORK_FAILURE,
-  };
-
   class HostEventCallback {
    public:
     virtual ~HostEventCallback() {}
 
     // Called when state of the connection changes.
-    virtual void OnConnectionState(State state, Error error) = 0;
+    virtual void OnConnectionState(State state, ErrorCode error) = 0;
   };
 
   ConnectionToHost(base::MessageLoopProxy* message_loop,
@@ -116,12 +109,12 @@ class ConnectionToHost : public SignalStrategy::Listener,
   // Callback for |video_reader_|.
   void OnVideoPacket(VideoPacket* packet);
 
-  void CloseOnError(Error error);
+  void CloseOnError(ErrorCode error);
 
   // Stops writing in the channels.
   void CloseChannels();
 
-  void SetState(State state, Error error);
+  void SetState(State state, ErrorCode error);
 
   scoped_refptr<base::MessageLoopProxy> message_loop_;
   pp::Instance* pp_instance_;
@@ -148,7 +141,7 @@ class ConnectionToHost : public SignalStrategy::Listener,
 
   // Internal state of the connection.
   State state_;
-  Error error_;
+  ErrorCode error_;
 
  private:
   DISALLOW_COPY_AND_ASSIGN(ConnectionToHost);

remoting/protocol/errors.h

+// 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.
+
+#ifndef REMOTING_PROTOCOL_ERROR_H_
+#define REMOTING_PROTOCOL_ERROR_H_
+
+namespace remoting {
+namespace protocol {
+
+enum ErrorCode {
+  OK = 0,
+  PEER_IS_OFFLINE,
+  SESSION_REJECTED,
+  INCOMPATIBLE_PROTOCOL,
+  AUTHENTICATION_FAILED,
+  CHANNEL_CONNECTION_ERROR,
+  SIGNALING_ERROR,
+  SIGNALING_TIMEOUT,
+  UNKNOWN_ERROR,
+};
+
+}  // namespace protocol
+}  // namespace remoting
+
+#endif  // REMOTING_PROTOCOL_ERROR_H_

remoting/protocol/fake_session.cc

@@ -247,7 +247,7 @@ void FakeSession::SetRouteChangeCallback(const RouteChangeCallback& callback) {
   NOTIMPLEMENTED();
 }
 
-Session::Error FakeSession::error() {
+ErrorCode FakeSession::error() {
   return error_;
 }
 

remoting/protocol/fake_session.h

@@ -140,7 +140,7 @@ class FakeSession : public Session {
     message_loop_ = message_loop;
   }
 
-  void set_error(Session::Error error) { error_ = error; }
+  void set_error(ErrorCode error) { error_ = error; }
 
   bool is_closed() const { return closed_; }
 
@@ -154,7 +154,7 @@ class FakeSession : public Session {
   virtual void SetRouteChangeCallback(
       const RouteChangeCallback& callback) OVERRIDE;
 
-  virtual Session::Error error() OVERRIDE;
+  virtual ErrorCode error() OVERRIDE;
 
   virtual void CreateStreamChannel(
       const std::string& name, const StreamChannelCallback& callback) OVERRIDE;
@@ -182,7 +182,7 @@ class FakeSession : public Session {
 
   std::string jid_;
 
-  Session::Error error_;
+  ErrorCode error_;
   bool closed_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeSession);

remoting/protocol/jingle_session.cc

@@ -38,16 +38,16 @@ const int kTransportInfoSendDelayMs = 2;
 // |transport-info|.
 const int kMessageResponseTimeoutSeconds = 10;
 
-Session::Error AuthRejectionReasonToError(
+ErrorCode AuthRejectionReasonToErrorCode(
     Authenticator::RejectionReason reason) {
   switch (reason) {
     case Authenticator::INVALID_CREDENTIALS:
-      return Session::AUTHENTICATION_FAILED;
+      return AUTHENTICATION_FAILED;
     case Authenticator::PROTOCOL_ERROR:
-      return Session::INCOMPATIBLE_PROTOCOL;
+      return INCOMPATIBLE_PROTOCOL;
   }
   NOTREACHED();
-  return Session::UNKNOWN_ERROR;
+  return UNKNOWN_ERROR;
 }
 
 }  // namespace
@@ -79,7 +79,7 @@ void JingleSession::SetRouteChangeCallback(
   route_change_callback_ = callback;
 }
 
-Session::Error JingleSession::error() {
+ErrorCode JingleSession::error() {
   DCHECK(CalledOnValidThread());
   return error_;
 }
@@ -148,7 +148,7 @@ void JingleSession::AcceptIncomingConnection(
   DCHECK_EQ(authenticator_->state(), Authenticator::WAITING_MESSAGE);
   authenticator_->ProcessMessage(first_auth_message);
   if (authenticator_->state() == Authenticator::REJECTED) {
-    CloseInternal(AuthRejectionReasonToError(
+    CloseInternal(AuthRejectionReasonToErrorCode(
         authenticator_->rejection_reason()));
     return;
   }
@@ -290,15 +290,12 @@ void JingleSession::OnMessageResponse(
     JingleMessage::ActionType request_type,
     IqRequest* request,
     const buzz::XmlElement* response) {
-  Error error = OK;
-
   std::string type_str = JingleMessage::GetActionName(request_type);
+  CleanupPendingRequests(request);
 
   if (!response) {
     LOG(ERROR) << type_str << " request timed out.";
-    // Most likely the session-initiate timeout indicates a problem
-    // with the signaling.
-    error = UNKNOWN_ERROR;
+    CloseInternal(SIGNALING_TIMEOUT);
   } else {
     const std::string& type = response->Attr(buzz::QName("", "type"));
     if (type != "result") {
@@ -310,21 +307,16 @@ void JingleSession::OnMessageResponse(
         case JingleMessage::SESSION_INFO:
           // session-info is used for the new authentication protocol,
           // and wasn't previously supported.
-          error = INCOMPATIBLE_PROTOCOL;
+          CloseInternal(INCOMPATIBLE_PROTOCOL);
+          break;
 
         default:
           // TODO(sergeyu): There may be different reasons for error
           // here. Parse the response stanza to find failure reason.
-          error = PEER_IS_OFFLINE;
+          CloseInternal(PEER_IS_OFFLINE);
       }
     }
   }
-
-  CleanupPendingRequests(request);
-
-  if (error != OK) {
-    CloseInternal(error);
-  }
 }
 
 void JingleSession::CleanupPendingRequests(IqRequest* request) {
@@ -528,7 +520,7 @@ void JingleSession::ProcessAuthenticationStep() {
   if (authenticator_->state() == Authenticator::ACCEPTED) {
     SetState(AUTHENTICATED);
   } else if (authenticator_->state() == Authenticator::REJECTED) {
-    CloseInternal(AuthRejectionReasonToError(
+    CloseInternal(AuthRejectionReasonToErrorCode(
         authenticator_->rejection_reason()));
   }
 }
@@ -539,7 +531,7 @@ void JingleSession::SendTransportInfo() {
   SendMessage(message);
 }
 
-void JingleSession::CloseInternal(Error error) {
+void JingleSession::CloseInternal(ErrorCode error) {
   DCHECK(CalledOnValidThread());
 
   if (state_ == CONNECTING || state_ == CONNECTED || state_ == AUTHENTICATED) {

remoting/protocol/jingle_session.h

@@ -43,7 +43,7 @@ class JingleSession : public Session,
       const StateChangeCallback& callback) OVERRIDE;
   virtual void SetRouteChangeCallback(
       const RouteChangeCallback& callback) OVERRIDE;
-  virtual Error error() OVERRIDE;
+  virtual ErrorCode error() OVERRIDE;
   virtual void CreateStreamChannel(
       const std::string& name,
       const StreamChannelCallback& callback) OVERRIDE;
@@ -119,7 +119,7 @@ class JingleSession : public Session,
   // Terminates the session and sends session-terminate if it is
   // necessary. |error| specifies the error code in case when the
   // session is being closed due to an error.
-  void CloseInternal(Error error);
+  void CloseInternal(ErrorCode error);
 
   // Sets |state_| to |new_state| and calls state change callback.
   void SetState(State new_state);
@@ -132,7 +132,7 @@ class JingleSession : public Session,
 
   std::string session_id_;
   State state_;
-  Error error_;
+  ErrorCode error_;
 
   SessionConfig config_;
   bool config_is_set_;

remoting/protocol/protocol_mock_objects.h

@@ -46,7 +46,7 @@ class MockConnectionToClientEventHandler :
   MOCK_METHOD1(OnConnectionOpened, void(ConnectionToClient* connection));
   MOCK_METHOD1(OnConnectionClosed, void(ConnectionToClient* connection));
   MOCK_METHOD2(OnConnectionFailed, void(ConnectionToClient* connection,
-                                        Session::Error error));
+                                        ErrorCode error));
   MOCK_METHOD2(OnSequenceNumberUpdated, void(ConnectionToClient* connection,
                                              int64 sequence_number));
   MOCK_METHOD4(OnRouteChange, void(
@@ -111,7 +111,7 @@ class MockSession : public Session {
                void(const StateChangeCallback& callback));
   MOCK_METHOD1(SetRouteChangeCallback,
                void(const RouteChangeCallback& callback));
-  MOCK_METHOD0(error, Session::Error());
+  MOCK_METHOD0(error, ErrorCode());
   MOCK_METHOD2(CreateStreamChannel, void(
       const std::string& name, const StreamChannelCallback& callback));
   MOCK_METHOD2(CreateDatagramChannel, void(

remoting/protocol/session.h

@@ -10,6 +10,7 @@
 #include "base/callback.h"
 #include "base/threading/non_thread_safe.h"
 #include "remoting/protocol/buffered_socket_writer.h"
+#include "remoting/protocol/errors.h"
 #include "remoting/protocol/session_config.h"
 
 namespace net {
@@ -53,17 +54,6 @@ class Session : public base::NonThreadSafe {
     FAILED,
   };
 
-  // TODO(sergeyu): Move error codes to a separate file.
-  enum Error {
-    OK = 0,
-    PEER_IS_OFFLINE,
-    SESSION_REJECTED,
-    INCOMPATIBLE_PROTOCOL,
-    AUTHENTICATION_FAILED,
-    CHANNEL_CONNECTION_ERROR,
-    UNKNOWN_ERROR,
-  };
-
   // State change callbacks are called after session state has
   // changed. It is not safe to destroy the session from within the
   // handler unless |state| is CLOSED or FAILED.
@@ -95,7 +85,7 @@ class Session : public base::NonThreadSafe {
   virtual void SetRouteChangeCallback(const RouteChangeCallback& callback) = 0;
 
   // Returns error code for a failed session.
-  virtual Error error() = 0;
+  virtual ErrorCode error() = 0;
 
   // Creates new channels for this connection. The specified callback
   // is called when then new channel is created and connected. The

remoting/remoting.gyp

@@ -792,6 +792,7 @@
         'protocol/connection_to_host.h',
         'protocol/content_description.cc',
         'protocol/content_description.h',
+        'protocol/errors.h',
         'protocol/host_control_dispatcher.cc',
         'protocol/host_control_dispatcher.h',
         'protocol/host_event_dispatcher.cc',

remoting/webapp/client_plugin_async.js

@@ -119,16 +119,22 @@ remoting.ClientPluginAsync.prototype.handleMessage_ = function(message_str) {
   } else if (message.method == 'onConnectionStatus') {
     if (typeof message.data['state'] != 'string' ||
         !(message.data['state'] in remoting.ClientSession.State) ||
-        typeof message.data['error'] != 'string' ||
-        !(message.data['error'] in remoting.ClientSession.ConnectionError)) {
+        typeof message.data['error'] != 'string') {
       console.error('Received invalid onConnectionState message: ' +
                     message_str);
       return;
     }
+
     /** @type {remoting.ClientSession.State} */
     var state = remoting.ClientSession.State[message.data['state']];
-    /** @type {remoting.ClientSession.ConnectionError} */
-    var error = remoting.ClientSession.ConnectionError[message.data['error']];
+    var error;
+    if (message.data['error'] in remoting.ClientSession.ConnectionError) {
+      error = /** @type {remoting.ClientSession.ConnectionError} */
+          remoting.ClientSession.ConnectionError[message.data['error']];
+    } else {
+      error = remoting.ClientSession.ConnectionError.UNKNOWN;
+    }
+
     this.onConnectionStatusUpdateHandler(state, error);
   } else if (message.method == 'onDesktopSize') {
     if (typeof message.data['width'] != 'number' ||

remoting/webapp/client_session.js

@@ -83,6 +83,7 @@ remoting.ClientSession.State = {
 
 /** @enum {number} */
 remoting.ClientSession.ConnectionError = {
+  UNKNOWN: -1,
   NONE: 0,
   HOST_IS_OFFLINE: 1,
   SESSION_REJECTED: 2,

remoting/webapp/server_log_entry.js

@@ -88,6 +88,8 @@ remoting.ServerLogEntry.getValueForConnectionError =
       return 'incompatible-protocol';
     case remoting.ClientSession.ConnectionError.NETWORK_FAILURE:
       return 'network-failure';
+    case remoting.ClientSession.ConnectionError.UNKNOWN:
+      return 'unknown';
     default:
       return 'unknown-' + connectionError;
   }