Improve IT2Me error logging.

When a host is unable to set-up sharing for IT2Me, there is currently no
reportable error message (with the exception of failure due to domain
policy). This CL adds an error message to the native messaging protocol,
logging it to the console. In the longer term, as we identify common
failures, it makes sense to treat them like we do kInvalidDomainError,
and show a proper error message, but for now this will help us track
down what the common failures are.

As part of cleaning this up, I've deprecated web-app support for errors
being reported as a host state change message. These were being handled
the same way as the top-level "error" message, and the latter has the
advantage of not being tied to a host state change.

BUG=505627

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

Cr-Commit-Position: refs/heads/master@{#336706}

5 files changed, 45 insertions, 31 deletions

diff --git a/remoting/host/it2me/it2me_host.cc b/remoting/host/it2me/it2me_host.cc
index 79d5f45..49cca62 100644
--- a/remoting/host/it2me/it2me_host.cc
+++ b/remoting/host/it2me/it2me_host.cc
@@ -95,8 +95,8 @@ void It2MeHost::Disconnect() {
       return;
 
     case kStarting:
-      SetState(kDisconnecting);
-      SetState(kDisconnected);
+      SetState(kDisconnecting, "");
+      SetState(kDisconnected, "");
       ShutdownOnNetworkThread();
       return;
 
@@ -104,10 +104,10 @@ void It2MeHost::Disconnect() {
       return;
 
     default:
-      SetState(kDisconnecting);
+      SetState(kDisconnecting, "");
 
       if (!host_) {
-        SetState(kDisconnected);
+        SetState(kDisconnected, "");
         ShutdownOnNetworkThread();
         return;
       }
@@ -136,7 +136,7 @@ void It2MeHost::RequestNatPolicy() {
 void It2MeHost::ShowConfirmationPrompt() {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
 
-  SetState(kStarting);
+  SetState(kStarting, "");
 
   scoped_ptr<It2MeConfirmationDialog> confirmation_dialog =
       confirmation_dialog_factory_->Create();
@@ -198,7 +198,7 @@ void It2MeHost::FinishConnect() {
   if (!required_host_domain_.empty() &&
       !base::EndsWith(xmpp_server_config_.username,
                       std::string("@") + required_host_domain_, false)) {
-    SetState(kInvalidDomainError);
+    SetState(kInvalidDomainError, "");
     return;
   }
 
@@ -269,7 +269,7 @@ void It2MeHost::FinishConnect() {
   signal_strategy_->Connect();
   host_->Start(xmpp_server_config_.username);
 
-  SetState(kRequestedAccessCode);
+  SetState(kRequestedAccessCode, "");
   return;
 }
 
@@ -287,7 +287,7 @@ void It2MeHost::ShutdownOnNetworkThread() {
     register_request_.reset();
     host_status_logger_.reset();
     signal_strategy_.reset();
-    SetState(kDisconnected);
+    SetState(kDisconnected, "");
   }
 
   host_context_->ui_task_runner()->PostTask(
@@ -341,7 +341,7 @@ void It2MeHost::OnClientAuthenticated(const std::string& jid) {
       FROM_HERE, base::Bind(&It2MeHost::Observer::OnClientAuthenticated,
                             observer_, client_username));
 
-  SetState(kConnected);
+  SetState(kConnected, "");
 }
 
 void It2MeHost::OnClientDisconnected(const std::string& jid) {
@@ -419,7 +419,8 @@ It2MeHost::~It2MeHost() {
   DCHECK(!policy_watcher_.get());
 }
 
-void It2MeHost::SetState(It2MeHostState state) {
+void It2MeHost::SetState(It2MeHostState state,
+                         const std::string& error_message) {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
 
   switch (state_) {
@@ -464,7 +465,7 @@ void It2MeHost::SetState(It2MeHostState state) {
   // Post a state-change notification to the web-app.
   task_runner_->PostTask(
       FROM_HERE, base::Bind(&It2MeHost::Observer::OnStateChanged,
-                            observer_, state));
+                            observer_, state, error_message));
 }
 
 bool It2MeHost::IsConnected() const {
@@ -473,13 +474,13 @@ bool It2MeHost::IsConnected() const {
 }
 
 void It2MeHost::OnReceivedSupportID(
-    bool success,
     const std::string& support_id,
-    const base::TimeDelta& lifetime) {
+    const base::TimeDelta& lifetime,
+    const std::string& error_message) {
   DCHECK(host_context_->network_task_runner()->BelongsToCurrentThread());
 
-  if (!success) {
-    SetState(kError);
+  if (!error_message.empty()) {
+    SetState(kError, error_message);
     Disconnect();
     return;
   }
@@ -489,8 +490,9 @@ void It2MeHost::OnReceivedSupportID(
 
   std::string local_certificate = host_key_pair_->GenerateCertificate();
   if (local_certificate.empty()) {
-    LOG(ERROR) << "Failed to generate host certificate.";
-    SetState(kError);
+    std::string error_message = "Failed to generate host certificate.";
+    LOG(ERROR) << error_message;
+    SetState(kError, error_message);
     Disconnect();
     return;
   }
@@ -505,7 +507,7 @@ void It2MeHost::OnReceivedSupportID(
       FROM_HERE, base::Bind(&It2MeHost::Observer::OnStoreAccessCode,
                             observer_, access_code, lifetime));
 
-  SetState(kReceivedAccessCode);
+  SetState(kReceivedAccessCode, "");
 }
 
 It2MeHostFactory::It2MeHostFactory() : policy_service_(nullptr) {
diff --git a/remoting/host/it2me/it2me_host.h b/remoting/host/it2me/it2me_host.h
index cbc835c..0b16085 100644
--- a/remoting/host/it2me/it2me_host.h
+++ b/remoting/host/it2me/it2me_host.h
@@ -57,7 +57,8 @@ class It2MeHost : public base::RefCountedThreadSafe<It2MeHost>,
     virtual void OnStoreAccessCode(const std::string& access_code,
                                    base::TimeDelta access_code_lifetime) = 0;
     virtual void OnNatPolicyChanged(bool nat_traversal_enabled) = 0;
-    virtual void OnStateChanged(It2MeHostState state) = 0;
+    virtual void OnStateChanged(It2MeHostState state,
+                                const std::string& error_message) = 0;
   };
 
   It2MeHost(
@@ -86,7 +87,10 @@ class It2MeHost : public base::RefCountedThreadSafe<It2MeHost>,
   void OnClientAuthenticated(const std::string& jid) override;
   void OnClientDisconnected(const std::string& jid) override;
 
-  void SetStateForTesting(It2MeHostState state) { SetState(state); }
+  void SetStateForTesting(It2MeHostState state,
+                          const std::string& error_message) {
+    SetState(state, error_message);
+  }
 
  protected:
   friend class base::RefCountedThreadSafe<It2MeHost>;
@@ -101,7 +105,7 @@ class It2MeHost : public base::RefCountedThreadSafe<It2MeHost>,
 
  private:
   // Updates state of the host. Can be called only on the network thread.
-  void SetState(It2MeHostState state);
+  void SetState(It2MeHostState state, const std::string& error_message);
 
   // Returns true if the host is connected.
   bool IsConnected() const;
@@ -120,9 +124,9 @@ class It2MeHost : public base::RefCountedThreadSafe<It2MeHost>,
   void FinishConnect();
 
   // Called when the support host registration completes.
-  void OnReceivedSupportID(bool success,
-                           const std::string& support_id,
-                           const base::TimeDelta& lifetime);
+  void OnReceivedSupportID(const std::string& support_id,
+                           const base::TimeDelta& lifetime,
+                           const std::string& error_message);
 
   // Shuts down |host_| on the network thread and posts ShutdownOnUiThread()
   // to shut down UI thread resources.
diff --git a/remoting/host/it2me/it2me_native_messaging_host.cc b/remoting/host/it2me/it2me_native_messaging_host.cc
index c1841f0..ddd58d3 100644
--- a/remoting/host/it2me/it2me_native_messaging_host.cc
+++ b/remoting/host/it2me/it2me_native_messaging_host.cc
@@ -243,7 +243,9 @@ void It2MeNativeMessagingHost::SendErrorAndExit(
   client_->CloseChannel(std::string());
 }
 
-void It2MeNativeMessagingHost::OnStateChanged(It2MeHostState state) {
+void It2MeNativeMessagingHost::OnStateChanged(
+    It2MeHostState state,
+    const std::string& error_message) {
   DCHECK(task_runner()->BelongsToCurrentThread());
 
   state_ = state;
@@ -269,6 +271,14 @@ void It2MeNativeMessagingHost::OnStateChanged(It2MeHostState state) {
       client_username_.clear();
       break;
 
+    case kError:
+      // kError is an internal-only state, sent to the web-app by a separate
+      // "error" message so that errors that occur before the "connect" message
+      // is sent can be communicated.
+      message->SetString("type", "error");
+      message->SetString("description", error_message);
+      break;
+
     default:
       ;
   }
@@ -316,4 +326,3 @@ std::string It2MeNativeMessagingHost::HostStateToString(
 }
 
 }  // namespace remoting
-
diff --git a/remoting/host/it2me/it2me_native_messaging_host.h b/remoting/host/it2me/it2me_native_messaging_host.h
index 0603c1a..02379ba 100644
--- a/remoting/host/it2me/it2me_native_messaging_host.h
+++ b/remoting/host/it2me/it2me_native_messaging_host.h
@@ -41,7 +41,8 @@ class It2MeNativeMessagingHost : public It2MeHost::Observer,
   void OnStoreAccessCode(const std::string& access_code,
                                  base::TimeDelta access_code_lifetime) override;
   void OnNatPolicyChanged(bool nat_traversal_enabled) override;
-  void OnStateChanged(It2MeHostState state) override;
+  void OnStateChanged(It2MeHostState state,
+                      const std::string& error_message) override;
 
   static std::string HostStateToString(It2MeHostState host_state);
 
@@ -85,4 +86,3 @@ class It2MeNativeMessagingHost : public It2MeHost::Observer,
 }  // namespace remoting
 
 #endif  // REMOTING_HOST_IT2ME_IT2ME_NATIVE_MESSAGING_HOST_H_
-
diff --git a/remoting/host/it2me/it2me_native_messaging_host_unittest.cc b/remoting/host/it2me/it2me_native_messaging_host_unittest.cc
index 9cb68e9..fa0b088 100644
--- a/remoting/host/it2me/it2me_native_messaging_host_unittest.cc
+++ b/remoting/host/it2me/it2me_native_messaging_host_unittest.cc
@@ -142,9 +142,9 @@ void MockIt2MeHost::RequestNatPolicy() {}
 void MockIt2MeHost::RunSetState(It2MeHostState state) {
   if (!host_context()->network_task_runner()->BelongsToCurrentThread()) {
     host_context()->network_task_runner()->PostTask(
-        FROM_HERE, base::Bind(&It2MeHost::SetStateForTesting, this, state));
+        FROM_HERE, base::Bind(&It2MeHost::SetStateForTesting, this, state, ""));
   } else {
-    SetStateForTesting(state);
+    SetStateForTesting(state, "");
   }
 }
 
@@ -547,4 +547,3 @@ TEST_F(It2MeNativeMessagingHostTest, InvalidType) {
 }
 
 }  // namespace remoting
-