Report signling errors for register-support-host requests.

remoting/host/register_support_host_request.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/host/register_support_host_request.h"
 
 #include <stdint.h>
 
 #include "base/bind.h"
 #include "base/callback_helpers.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringize_macros.h"
 #include "base/time/time.h"
 #include "remoting/base/constants.h"
 #include "remoting/host/host_config.h"
 #include "remoting/host/host_details.h"
+#include "remoting/protocol/errors.h"
 #include "remoting/signaling/iq_sender.h"
 #include "remoting/signaling/jid_util.h"
 #include "remoting/signaling/signal_strategy.h"
 #include "remoting/signaling/signaling_address.h"
 #include "third_party/libjingle_xmpp/xmllite/xmlelement.h"
 #include "third_party/libjingle_xmpp/xmpp/constants.h"
 
 using buzz::QName;
 using buzz::XmlElement;
 
 namespace remoting {
 
 namespace {
 // Strings used in the request message we send to the bot.
 const char kRegisterQueryTag[] = "register-support-host";
 const char kPublicKeyTag[] = "public-key";
 const char kSignatureTag[] = "signature";
 const char kSignatureTimeAttr[] = "time";
 const char kHostVersionTag[] = "host-version";
 const char kHostOsNameTag[] = "host-os-name";
 const char kHostOsVersionTag[] = "host-os-version";
 
 // Strings used to parse responses received from the bot.
 const char kRegisterQueryResultTag[] = "register-support-host-result";
 const char kSupportIdTag[] = "support-id";
 const char kSupportIdLifetimeTag[] = "support-id-lifetime";
+
+// The signaling timeout for register support host requests.
+const int kRegisterRequestTimeoutInSeconds = 10;

[Sergey Ulanov, 2017/06/07 06:41:30]

nit: use constexpr base::TimeDelta: 

constexpr base::TimeDelta kRegisterRequestTimeout =
base::TimeDelta::FromSeconds(10);

[kelvinp, 2017/06/12 21:31:26]

Done.

 }
 
 RegisterSupportHostRequest::RegisterSupportHostRequest(
     SignalStrategy* signal_strategy,
     scoped_refptr<RsaKeyPair> key_pair,
     const std::string& directory_bot_jid,
     const RegisterCallback& callback)
     : signal_strategy_(signal_strategy),
       key_pair_(key_pair),
       directory_bot_jid_(directory_bot_jid),
       callback_(callback) {
   DCHECK(signal_strategy_);
   DCHECK(key_pair_.get());
   signal_strategy_->AddListener(this);
   iq_sender_.reset(new IqSender(signal_strategy_));
 }
 
 RegisterSupportHostRequest::~RegisterSupportHostRequest() {
   if (signal_strategy_)
     signal_strategy_->RemoveListener(this);
 }
 
 void RegisterSupportHostRequest::OnSignalStrategyStateChange(
     SignalStrategy::State state) {
   if (state == SignalStrategy::CONNECTED) {
     DCHECK(!callback_.is_null());
-
     request_ = iq_sender_->SendIq(
         buzz::STR_SET, directory_bot_jid_,
         CreateRegistrationRequest(signal_strategy_->GetLocalAddress().jid()),
         base::Bind(&RegisterSupportHostRequest::ProcessResponse,
                    base::Unretained(this)));
+    if (!request_.get()) {

[Sergey Ulanov, 2017/06/07 06:41:29]

nit: don't need .get()

[kelvinp, 2017/06/12 21:31:25]

Done.

+      std::string error_message =
+          protocol::ErrorCodeToString(protocol::ErrorCode::SIGNALING_ERROR);

[Sergey Ulanov, 2017/06/07 06:41:29]

error_message should a human-readable message that's logged in the console for
debugging.

[kelvinp, 2017/06/12 21:31:26]

Done.

+      LOG(ERROR) << error_message;
+      CallCallback(std::string(), base::TimeDelta(), error_message);
+      return;
+    }
+
+    request_->SetTimeout(
+        base::TimeDelta::FromSeconds(kRegisterRequestTimeoutInSeconds));
+
   } else if (state == SignalStrategy::DISCONNECTED) {
     // We will reach here if signaling fails to connect.
     std::string error_message = "Signal strategy disconnected.";
     LOG(ERROR) << error_message;
     CallCallback(std::string(), base::TimeDelta(), error_message);
   }
 }
 
 bool RegisterSupportHostRequest::OnSignalStrategyIncomingStanza(
     const buzz::XmlElement* stanza) {
@@ skipping 49 matching lines @@
 
   return signature_tag;
 }
 
 void RegisterSupportHostRequest::ParseResponse(const XmlElement* response,
                                                std::string* support_id,
                                                base::TimeDelta* lifetime,
                                                std::string* error_message) {
   std::ostringstream error;
 
+  if (!response) {
+    *error_message =
+        protocol::ErrorCodeToString(protocol::ErrorCode::SIGNALING_TIMEOUT);

[Sergey Ulanov, 2017/06/07 06:41:30]

same here. Use a readable message please.

[kelvinp, 2017/06/12 21:31:25]

Done.

+    LOG(ERROR) << *error_message;

[Sergey Ulanov, 2017/06/07 06:41:30]

I don't think we need to log the message here. Or at least move it to the
ProcessResponse(), so it doesn't have to be duplicated for each error here.

[kelvinp, 2017/06/12 21:31:25]

We log the error message in all other branches of this function though.
Nevertheless, I moved the error message logging to ProcessResponse().

+    return;
+  }
+
   std::string type = response->Attr(buzz::QN_TYPE);
   if (type == buzz::STR_ERROR) {
     error << "Received error in response to heartbeat: " << response->Str();
     *error_message = error.str();
     LOG(ERROR) << *error_message;
     return;
   }
 
   // This method must only be called for error or result stanzas.
   if (type != buzz::STR_RESULT) {
@@ skipping 69 matching lines @@
   // Cleanup state before calling the callback.
   request_.reset();
   iq_sender_.reset();
   signal_strategy_->RemoveListener(this);
   signal_strategy_ = nullptr;
 
   base::ResetAndReturn(&callback_).Run(support_id, lifetime, error_message);
 }
 
 }  // namespace remoting