Reporting of policy errors via host-offline-reason: part 3

remoting/host/heartbeat_sender.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/heartbeat_sender.h"
 
 #include <math.h>
 
 #include "base/bind.h"
-#include "base/message_loop/message_loop_proxy.h"
 #include "base/rand_util.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/strings/stringize_macros.h"
+#include "base/thread_task_runner_handle.h"
 #include "base/time/time.h"
 #include "remoting/base/constants.h"
 #include "remoting/base/logging.h"
 #include "remoting/host/server_log_entry_host.h"
 #include "remoting/signaling/iq_sender.h"
 #include "remoting/signaling/server_log_entry.h"
 #include "remoting/signaling/signal_strategy.h"
 #include "third_party/webrtc/libjingle/xmllite/xmlelement.h"
 #include "third_party/webrtc/libjingle/xmpp/constants.h"
 
@@ skipping 23 matching lines @@
 const int64 kResendDelayOnHostNotFoundMs = 10 * 1000; // 10 seconds.
 const int kMaxResendOnHostNotFoundCount = 12;  // 2 minutes (12 x 10 seconds).
 
 }  // namespace
 
 HeartbeatSender::HeartbeatSender(
     const base::Closure& on_heartbeat_successful_callback,
     const base::Closure& on_unknown_host_id_error,
     const std::string& host_id,
     SignalStrategy* signal_strategy,
-    scoped_refptr<RsaKeyPair> key_pair,
+    const scoped_refptr<const RsaKeyPair>& host_key_pair,
     const std::string& directory_bot_jid)
     : on_heartbeat_successful_callback_(on_heartbeat_successful_callback),
       on_unknown_host_id_error_(on_unknown_host_id_error),
       host_id_(host_id),
       signal_strategy_(signal_strategy),
-      key_pair_(key_pair),
+      host_key_pair_(host_key_pair),
       directory_bot_jid_(directory_bot_jid),
       interval_ms_(kDefaultHeartbeatIntervalMs),
       sequence_id_(0),
       sequence_id_was_set_(false),
       sequence_id_recent_set_num_(0),
       heartbeat_succeeded_(false),
       failed_startup_heartbeat_count_(0) {
   DCHECK(signal_strategy_);
-  DCHECK(key_pair_.get());
+  DCHECK(host_key_pair_.get());
   DCHECK(thread_checker_.CalledOnValidThread());
 
   signal_strategy_->AddListener(this);
 
   // Start heartbeats if the |signal_strategy_| is already connected.
   OnSignalStrategyStateChange(signal_strategy_->GetState());
 }
 
 HeartbeatSender::~HeartbeatSender() {
   signal_strategy_->RemoveListener(this);
@@ skipping 12 matching lines @@
     timer_.Stop();
     timer_resend_.Stop();
   }
 }
 
 bool HeartbeatSender::OnSignalStrategyIncomingStanza(
     const buzz::XmlElement* stanza) {
   return false;
 }
 
+void HeartbeatSender::OnHostOfflineReasonTimeout() {
+  DCHECK(!host_offline_reason_ack_callback_.is_null());
+
+  base::Callback<void(bool)> local_callback = host_offline_reason_ack_callback_;
+  host_offline_reason_ack_callback_.Reset();
+  local_callback.Run(false /* success */);

[Wez, 2015/01/08 01:49:50]

s/success/failure :)

[Łukasz Anforowicz, 2015/01/08 23:50:31]

Ok, I can see the ambiguity now.  What I meant was that success == false.  Let
me just remove this comment to avoid the ambiguity (and the comment for "true"
below as well).

[Wez, 2015/01/09 02:55:45]

Actually what I meant was that you comment /* success */ on both the true and
false values!  In general I'd expect a function with a bool return-value to
convey success (i.e. true == succeeded) rather than failure.  Of course, this is
good motivation for using an enum after all, for clarity, I suppose. :-/

[Łukasz Anforowicz, 2015/01/09 19:00:13]

Yes - avoiding this kind of confusion was why I wanted to introduce the
AckOrTimeout enum.

The "/* success */" comment was meant to say that "false" is bound to "success"
parameter, not that "false" indicates success.  C#'s named parameters address
this case nicely (+ give compile-time validation so that names used here do not
get out of sync with parameter declaration [unlike what can easily happen to
comments]).

I'll leave it as-is, but without a /* success */ or a /* failure */ comment
(saing "failure" would be just as confusing - is failure=false [and therefore
success=true]?  or does false mean failure [i.e. success=false]).

+}
+
+void HeartbeatSender::OnHostOfflineReasonAck() {
+  if (host_offline_reason_ack_callback_.is_null()) {
+    DCHECK(!host_offline_reason_timeout_timer_.IsRunning());
+    return;
+  }
+
+  DCHECK(host_offline_reason_timeout_timer_.IsRunning());
+  host_offline_reason_timeout_timer_.Stop();
+
+  // Run the ACK callback under a clean stack via PostTask() (because the
+  // callback can end up deleting |this| HeartbeatSender [i.e. when used from
+  // HostSignalingManager]).
+  base::Closure fully_bound_ack_callback =
+      base::Bind(host_offline_reason_ack_callback_, true /* success */);
+  host_offline_reason_ack_callback_.Reset();
+  base::ThreadTaskRunnerHandle::Get()->PostTask(FROM_HERE,
+                                                fully_bound_ack_callback);
+}
+
 void HeartbeatSender::SetHostOfflineReason(
     const std::string& host_offline_reason,
-    const base::Closure& ack_callback) {
+    const base::TimeDelta& timeout,
+    const base::Callback<void(bool success)>& ack_callback) {
   DCHECK(thread_checker_.CalledOnValidThread());
   DCHECK(host_offline_reason_ack_callback_.is_null());
   host_offline_reason_ = host_offline_reason;
   host_offline_reason_ack_callback_ = ack_callback;
+  host_offline_reason_timeout_timer_.Start(
+      FROM_HERE, timeout, this, &HeartbeatSender::OnHostOfflineReasonTimeout);
   if (signal_strategy_->GetState() == SignalStrategy::CONNECTED) {
     DoSendStanza();
   }
 }
 
 void HeartbeatSender::SendStanza() {
   // Make sure we don't send another heartbeat before the heartbeat interval
   // has expired.
   timer_resend_.Stop();
   DoSendStanza();
@@ skipping 101 matching lines @@
       sequence_id_recent_set_num_ = 0;
 
       // Notify listener of the first successful heartbeat.
       if (!heartbeat_succeeded_) {
         on_heartbeat_successful_callback_.Run();
       }
       heartbeat_succeeded_ = true;
 
       // Notify caller of SetHostOfflineReason that we got an ack.
       if (is_offline_heartbeat_response) {
-        if (!host_offline_reason_ack_callback_.is_null()) {
-          base::MessageLoop::current()->PostTask(
-              FROM_HERE,
-              host_offline_reason_ack_callback_);
-          host_offline_reason_ack_callback_.Reset();
-        }
+        OnHostOfflineReasonAck();
       }
     }
   }
 }
 
 void HeartbeatSender::SetInterval(int interval) {
   if (interval != interval_ms_) {
     interval_ms_ = interval;
 
     // Restart the timer with the new interval.
@@ skipping 54 matching lines @@
   heartbeat->AddElement(log.release());
   return heartbeat.Pass();
 }
 
 scoped_ptr<XmlElement> HeartbeatSender::CreateSignature() {
   scoped_ptr<XmlElement> signature_tag(new XmlElement(
       QName(kChromotingXmlNamespace, kHeartbeatSignatureTag)));
 
   std::string message = signal_strategy_->GetLocalJid() + ' ' +
       base::IntToString(sequence_id_);
-  std::string signature(key_pair_->SignMessage(message));
+  std::string signature(host_key_pair_->SignMessage(message));
   signature_tag->AddText(signature);
 
   return signature_tag.Pass();
 }
 
 }  // namespace remoting