The chromoting host uses heartbeat sequence IDs.

BUG=112112
TEST=Start a host, stop it, start it again, wait 10 minutes, and check the host is online.


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

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

3 files changed, 227 insertions, 53 deletions

diff --git a/remoting/host/heartbeat_sender.cc b/remoting/host/heartbeat_sender.cc
index d0eaba5..df9fa8c 100644
--- a/remoting/host/heartbeat_sender.cc
+++ b/remoting/host/heartbeat_sender.cc
@@ -4,9 +4,12 @@
 
 #include "remoting/host/heartbeat_sender.h"
 
+#include <math.h>
+
 #include "base/bind.h"
 #include "base/logging.h"
 #include "base/message_loop_proxy.h"
+#include "base/rand_util.h"
 #include "base/string_number_conversions.h"
 #include "base/time.h"
 #include "remoting/base/constants.h"
@@ -26,15 +29,17 @@ namespace {
 const char kHeartbeatQueryTag[] = "heartbeat";
 const char kHostIdAttr[] = "hostid";
 const char kHeartbeatSignatureTag[] = "signature";
-const char kSignatureTimeAttr[] = "time";
+const char kSequenceIdAttr[] = "sequence-id";
 
 const char kErrorTag[] = "error";
 const char kNotFoundTag[] = "item-not-found";
 
 const char kHeartbeatResultTag[] = "heartbeat-result";
 const char kSetIntervalTag[] = "set-interval";
+const char kExpectedSequenceIdTag[] = "expected-sequence-id";
 
 const int64 kDefaultHeartbeatIntervalMs = 5 * 60 * 1000;  // 5 minutes.
+const int64 kResendDelayMs = 10 * 1000;  // 10 seconds.
 
 }  // namespace
 
@@ -45,7 +50,10 @@ HeartbeatSender::HeartbeatSender(
     : host_id_(host_id),
       signal_strategy_(signal_strategy),
       key_pair_(key_pair),
-      interval_ms_(kDefaultHeartbeatIntervalMs) {
+      interval_ms_(kDefaultHeartbeatIntervalMs),
+      sequence_id_(0),
+      sequence_id_was_set_(false),
+      sequence_id_recent_set_num_(0) {
   DCHECK(signal_strategy_);
   DCHECK(key_pair_);
 
@@ -62,22 +70,38 @@ HeartbeatSender::~HeartbeatSender() {
 void HeartbeatSender::OnSignalStrategyStateChange(SignalStrategy::State state) {
   if (state == SignalStrategy::CONNECTED) {
     iq_sender_.reset(new IqSender(signal_strategy_));
-    DoSendStanza();
+    SendStanza();
     timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(interval_ms_),
-                 this, &HeartbeatSender::DoSendStanza);
+                 this, &HeartbeatSender::SendStanza);
   } else if (state == SignalStrategy::DISCONNECTED) {
     request_.reset();
     iq_sender_.reset();
     timer_.Stop();
+    timer_resend_.Stop();
   }
 }
 
+void HeartbeatSender::SendStanza() {
+  DoSendStanza();
+  // Make sure we don't send another heartbeat before the heartbeat interval
+  // has expired.
+  timer_resend_.Stop();
+}
+
+void HeartbeatSender::ResendStanza() {
+  DoSendStanza();
+  // Make sure we don't send another heartbeat before the heartbeat interval
+  // has expired.
+  timer_.Reset();
+}
+
 void HeartbeatSender::DoSendStanza() {
   VLOG(1) << "Sending heartbeat stanza to " << kChromotingBotJid;
   request_.reset(iq_sender_->SendIq(
       buzz::STR_SET, kChromotingBotJid, CreateHeartbeatMessage(),
       base::Bind(&HeartbeatSender::ProcessResponse,
                  base::Unretained(this))));
+  ++sequence_id_;
 }
 
 void HeartbeatSender::ProcessResponse(const XmlElement* response) {
@@ -119,6 +143,29 @@ void HeartbeatSender::ProcessResponse(const XmlElement* response) {
         SetInterval(interval * base::Time::kMillisecondsPerSecond);
       }
     }
+
+    bool did_set_sequence_id = false;
+    const XmlElement* expected_sequence_id_element =
+        result_element->FirstNamed(QName(kChromotingXmlNamespace,
+                                         kExpectedSequenceIdTag));
+    if (expected_sequence_id_element) {
+      // The sequence ID sent in the previous heartbeat was not what the server
+      // expected, so send another heartbeat with the expected sequence ID.
+      const std::string& expected_sequence_id_str =
+          expected_sequence_id_element->BodyText();
+      int expected_sequence_id;
+      if (!base::StringToInt(expected_sequence_id_str, &expected_sequence_id)) {
+        LOG(ERROR) << "Received invalid " << kExpectedSequenceIdTag << ": " <<
+            expected_sequence_id_element->Str();
+      } else {
+        SetSequenceId(expected_sequence_id);
+        sequence_id_recent_set_num_++;
+        did_set_sequence_id = true;
+      }
+    }
+    if (!did_set_sequence_id) {
+      sequence_id_recent_set_num_ = 0;
+    }
   }
 }
 
@@ -130,15 +177,40 @@ void HeartbeatSender::SetInterval(int interval) {
     if (timer_.IsRunning()) {
       timer_.Stop();
       timer_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(interval_ms_),
-                   this, &HeartbeatSender::DoSendStanza);
+                   this, &HeartbeatSender::SendStanza);
+    }
+  }
+}
+
+void HeartbeatSender::SetSequenceId(int sequence_id) {
+  sequence_id_ = sequence_id;
+  // Setting the sequence ID may be a symptom of a temporary server-side
+  // problem, which would affect many hosts, so don't send a new heartbeat
+  // immediately, as many hosts doing so may overload the server.
+  // But the server will usually set the sequence ID when it receives the first
+  // heartbeat from a host. In that case, we can send a new heartbeat
+  // immediately, as that only happens once per host instance.
+  if (!sequence_id_was_set_) {
+    ResendStanza();
+  } else {
+    LOG(INFO) << "The heartbeat sequence ID has been set more than once: "
+              << "the new value is " << sequence_id;
+    double delay = pow(2.0, sequence_id_recent_set_num_) *
+        (1 + base::RandDouble()) * kResendDelayMs;
+    if (delay <= interval_ms_) {
+      timer_resend_.Start(FROM_HERE, base::TimeDelta::FromMilliseconds(delay),
+                          this, &HeartbeatSender::ResendStanza);
     }
   }
+  sequence_id_was_set_ = true;
 }
 
 XmlElement* HeartbeatSender::CreateHeartbeatMessage() {
   XmlElement* query = new XmlElement(
       QName(kChromotingXmlNamespace, kHeartbeatQueryTag));
   query->AddAttr(QName(kChromotingXmlNamespace, kHostIdAttr), host_id_);
+  query->AddAttr(QName(kChromotingXmlNamespace, kSequenceIdAttr),
+                 base::IntToString(sequence_id_));
   query->AddElement(CreateSignature());
   return query;
 }
@@ -147,12 +219,8 @@ XmlElement* HeartbeatSender::CreateSignature() {
   XmlElement* signature_tag = new XmlElement(
       QName(kChromotingXmlNamespace, kHeartbeatSignatureTag));
 
-  int64 time = static_cast<int64>(base::Time::Now().ToDoubleT());
-  std::string time_str(base::Int64ToString(time));
-  signature_tag->AddAttr(
-      QName(kChromotingXmlNamespace, kSignatureTimeAttr), time_str);
-
-  std::string message = signal_strategy_->GetLocalJid() + ' ' + time_str;
+  std::string message = signal_strategy_->GetLocalJid() + ' ' +
+      base::IntToString(sequence_id_);
   std::string signature(key_pair_->GetSignature(message));
   signature_tag->AddText(signature);
 
diff --git a/remoting/host/heartbeat_sender.h b/remoting/host/heartbeat_sender.h
index 24a45ba..652183e 100644
--- a/remoting/host/heartbeat_sender.h
+++ b/remoting/host/heartbeat_sender.h
@@ -35,25 +35,32 @@ class IqSender;
 // <iq type="set" to="remoting@bot.talk.google.com"
 //     from="user@gmail.com/chromoting123123" id="5" xmlns="jabber:client">
 //   <rem:heartbeat rem:hostid="a1ddb11e-8aef-11df-bccf-18a905b9cb5a"
+//                  rem:sequence-id="456"
 //                  xmlns:rem="google:remoting">
-//     <rem:signature rem:time="1279061748">.signature.</rem:signature>
+//     <rem:signature>.signature.</rem:signature>
 //   </rem:heartbeat>
 // </iq>
 //
-// The time attribute of the signature is the decimal time when the message
-// was sent in second since the epoch (01/01/1970). The signature is a BASE64
-// encoded SHA-1/RSA signature created with the host's private key. The message
-// being signed is the full Jid concatenated with the time value, separated by
-// space. For example, for the heartbeat stanza above the message that is being
-// signed is "user@gmail.com/chromoting123123 1279061748".
+// The sequence-id attribute of the heartbeat is a zero-based incrementally
+// increasing integer unique to each heartbeat from a single host.
+// The Bot checks the value, and if it is incorrect, includes the
+// correct value in the result stanza. The host should then send another
+// heartbeat, with the correct sequence-id, and increment the sequence-id in
+// susbequent heartbeats.
+// The signature is a base-64 encoded SHA-1 hash, signed with the host's
+// private RSA key. The message being signed is the full Jid concatenated with
+// the sequence-id, separated by one space. For example, for the heartbeat
+// stanza above, the message that is signed is
+// "user@gmail.com/chromoting123123 456".
 //
-// Bot sends the following result stanza in response to each heartbeat:
+// The Bot sends the following result stanza in response to each successful
+// heartbeat:
 //
 //  <iq type="set" from="remoting@bot.talk.google.com"
 //      to="user@gmail.com/chromoting123123" id="5" xmlns="jabber:client">
 //    <rem:heartbeat-result xmlns:rem="google:remoting">
 //      <rem:set-interval>300</rem:set-interval>
-//    </rem:heartbeat>
+//    </rem:heartbeat-result>
 //  </iq>
 //
 // The set-interval tag is used to specify desired heartbeat interval
@@ -61,6 +68,15 @@ class IqSender;
 // optional. Host uses default heartbeat interval if it doesn't find
 // set-interval tag in the result Iq stanza it receives from the
 // server.
+// If the heartbeat's sequence-id was incorrect, the Bot sends a result
+// stanza of this form:
+//
+//  <iq type="set" from="remoting@bot.talk.google.com"
+//      to="user@gmail.com/chromoting123123" id="5" xmlns="jabber:client">
+//    <rem:heartbeat-result xmlns:rem="google:remoting">
+//      <rem:expected-sequence-id>654</rem:expected-sequence-id>
+//    </rem:heartbeat-result>
+//  </iq>
 class HeartbeatSender : public SignalStrategy::Listener {
  public:
   // Doesn't take ownership of |signal_strategy| or |key_pair|. Both
@@ -77,12 +93,19 @@ class HeartbeatSender : public SignalStrategy::Listener {
 
  private:
   FRIEND_TEST_ALL_PREFIXES(HeartbeatSenderTest, DoSendStanza);
+  FRIEND_TEST_ALL_PREFIXES(HeartbeatSenderTest,
+                           DoSendStanzaWithExpectedSequenceId);
   FRIEND_TEST_ALL_PREFIXES(HeartbeatSenderTest, CreateHeartbeatMessage);
-  FRIEND_TEST_ALL_PREFIXES(HeartbeatSenderTest, ProcessResponse);
+  FRIEND_TEST_ALL_PREFIXES(HeartbeatSenderTest, ProcessResponseSetInterval);
+  FRIEND_TEST_ALL_PREFIXES(HeartbeatSenderTest,
+                           ProcessResponseExpectedSequenceId);
 
+  void SendStanza();
+  void ResendStanza();
   void DoSendStanza();
   void ProcessResponse(const buzz::XmlElement* response);
   void SetInterval(int interval);
+  void SetSequenceId(int sequence_id);
 
   // Helper methods used by DoSendStanza() to generate heartbeat stanzas.
   // Caller owns the result.
@@ -96,6 +119,10 @@ class HeartbeatSender : public SignalStrategy::Listener {
   scoped_ptr<IqRequest> request_;
   int interval_ms_;
   base::RepeatingTimer<HeartbeatSender> timer_;
+  base::OneShotTimer<HeartbeatSender> timer_resend_;
+  int sequence_id_;
+  bool sequence_id_was_set_;
+  int sequence_id_recent_set_num_;
 
   DISALLOW_COPY_AND_ASSIGN(HeartbeatSender);
 };
diff --git a/remoting/host/heartbeat_sender_unittest.cc b/remoting/host/heartbeat_sender_unittest.cc
index 0b619af..111f19d 100644
--- a/remoting/host/heartbeat_sender_unittest.cc
+++ b/remoting/host/heartbeat_sender_unittest.cc
@@ -36,7 +36,6 @@ namespace remoting {
 namespace {
 const char kHostId[] = "0";
 const char kTestJid[] = "user@gmail.com/chromoting123";
-const int64 kTestTime = 123123123;
 const char kStanzaId[] = "123";
 }  // namespace
 
@@ -71,6 +70,9 @@ class HeartbeatSenderTest : public testing::Test {
     EXPECT_TRUE(signal_strategy_listeners_.empty());
   }
 
+  void ValidateHeartbeatStanza(XmlElement* stanza,
+                               const char* expectedSequenceId);
+
   MessageLoop message_loop_;
   MockSignalStrategy signal_strategy_;
   std::set<SignalStrategy::Listener*> signal_strategy_listeners_;
@@ -78,8 +80,7 @@ class HeartbeatSenderTest : public testing::Test {
   scoped_ptr<HeartbeatSender> heartbeat_sender_;
 };
 
-// Call Start() followed by Stop(), and makes sure an Iq stanza is
-// being sent.
+// Call Start() followed by Stop(), and make sure a valid heartbeat is sent.
 TEST_F(HeartbeatSenderTest, DoSendStanza) {
   XmlElement* sent_iq = NULL;
   EXPECT_CALL(signal_strategy_, GetLocalJid())
@@ -94,49 +95,101 @@ TEST_F(HeartbeatSenderTest, DoSendStanza) {
 
   scoped_ptr<XmlElement> stanza(sent_iq);
   ASSERT_TRUE(stanza != NULL);
+  ValidateHeartbeatStanza(stanza.get(), "0");
 
-  EXPECT_EQ(stanza->Attr(buzz::QName("", "to")),
-            std::string(kChromotingBotJid));
-  EXPECT_EQ(stanza->Attr(buzz::QName("", "type")), "set");
+  heartbeat_sender_->OnSignalStrategyStateChange(SignalStrategy::DISCONNECTED);
+  message_loop_.RunAllPending();
+}
+
+// Call Start() followed by Stop(), twice, and make sure two valid heartbeats
+// are sent, with the correct sequence IDs.
+TEST_F(HeartbeatSenderTest, DoSendStanzaTwice) {
+  XmlElement* sent_iq = NULL;
+  EXPECT_CALL(signal_strategy_, GetLocalJid())
+      .WillRepeatedly(Return(kTestJid));
+  EXPECT_CALL(signal_strategy_, GetNextId())
+      .WillOnce(Return(kStanzaId));
+  EXPECT_CALL(signal_strategy_, SendStanza(NotNull()))
+      .WillOnce(DoAll(SaveArg<0>(&sent_iq), Return(true)));
+
+  heartbeat_sender_->OnSignalStrategyStateChange(SignalStrategy::CONNECTED);
+  message_loop_.RunAllPending();
+
+  scoped_ptr<XmlElement> stanza(sent_iq);
+  ASSERT_TRUE(stanza != NULL);
+  ValidateHeartbeatStanza(stanza.get(), "0");
+
+  heartbeat_sender_->OnSignalStrategyStateChange(SignalStrategy::DISCONNECTED);
+  message_loop_.RunAllPending();
+
+  EXPECT_CALL(signal_strategy_, GetLocalJid())
+      .WillRepeatedly(Return(kTestJid));
+  EXPECT_CALL(signal_strategy_, GetNextId())
+      .WillOnce(Return(kStanzaId + 1));
+  EXPECT_CALL(signal_strategy_, SendStanza(NotNull()))
+      .WillOnce(DoAll(SaveArg<0>(&sent_iq), Return(true)));
+
+  heartbeat_sender_->OnSignalStrategyStateChange(SignalStrategy::CONNECTED);
+  message_loop_.RunAllPending();
+
+  scoped_ptr<XmlElement> stanza2(sent_iq);
+  ValidateHeartbeatStanza(stanza2.get(), "1");
 
   heartbeat_sender_->OnSignalStrategyStateChange(SignalStrategy::DISCONNECTED);
   message_loop_.RunAllPending();
 }
 
-// Validate format of the heartbeat stanza.
-TEST_F(HeartbeatSenderTest, CreateHeartbeatMessage) {
-  int64 start_time = static_cast<int64>(base::Time::Now().ToDoubleT());
+// Call Start() followed by Stop(), make sure a valid Iq stanza is sent,
+// reply with an expected sequence ID, and make sure two valid heartbeats
+// are sent, with the correct sequence IDs.
+TEST_F(HeartbeatSenderTest, DoSendStanzaWithExpectedSequenceId) {
+  XmlElement* sent_iq = NULL;
+  EXPECT_CALL(signal_strategy_, GetLocalJid())
+      .WillRepeatedly(Return(kTestJid));
+  EXPECT_CALL(signal_strategy_, GetNextId())
+      .WillOnce(Return(kStanzaId));
+  EXPECT_CALL(signal_strategy_, SendStanza(NotNull()))
+      .WillOnce(DoAll(SaveArg<0>(&sent_iq), Return(true)));
+
+  heartbeat_sender_->OnSignalStrategyStateChange(SignalStrategy::CONNECTED);
+  message_loop_.RunAllPending();
 
-  scoped_ptr<XmlElement> stanza(heartbeat_sender_->CreateHeartbeatMessage());
-  ASSERT_TRUE(stanza.get() != NULL);
+  scoped_ptr<XmlElement> stanza(sent_iq);
+  ASSERT_TRUE(stanza != NULL);
+  ValidateHeartbeatStanza(stanza.get(), "0");
 
-  EXPECT_TRUE(QName(kChromotingXmlNamespace, "heartbeat") ==
-              stanza->Name());
-  EXPECT_EQ(std::string(kHostId),
-            stanza->Attr(QName(kChromotingXmlNamespace, "hostid")));
+  XmlElement* sent_iq2 = NULL;
+  EXPECT_CALL(signal_strategy_, GetLocalJid())
+      .WillRepeatedly(Return(kTestJid));
+  EXPECT_CALL(signal_strategy_, GetNextId())
+      .WillOnce(Return(kStanzaId + 1));
+  EXPECT_CALL(signal_strategy_, SendStanza(NotNull()))
+      .WillOnce(DoAll(SaveArg<0>(&sent_iq2), Return(true)));
 
-  QName signature_tag(kChromotingXmlNamespace, "signature");
-  XmlElement* signature = stanza->FirstNamed(signature_tag);
-  ASSERT_TRUE(signature != NULL);
-  EXPECT_TRUE(stanza->NextNamed(signature_tag) == NULL);
+  scoped_ptr<XmlElement> response(new XmlElement(buzz::QN_IQ));
+  response->AddAttr(QName("", "type"), "result");
+  XmlElement* result = new XmlElement(
+      QName(kChromotingXmlNamespace, "heartbeat-result"));
+  response->AddElement(result);
+  XmlElement* expected_sequence_id = new XmlElement(
+      QName(kChromotingXmlNamespace, "expected-sequence-id"));
+  result->AddElement(expected_sequence_id);
+  const int kExpectedSequenceId = 456;
+  expected_sequence_id->AddText(base::IntToString(kExpectedSequenceId));
+  heartbeat_sender_->ProcessResponse(response.get());
+  message_loop_.RunAllPending();
 
-  std::string time_str =
-      signature->Attr(QName(kChromotingXmlNamespace, "time"));
-  int64 time;
-  EXPECT_TRUE(base::StringToInt64(time_str, &time));
-  int64 now = static_cast<int64>(base::Time::Now().ToDoubleT());
-  EXPECT_LE(start_time, time);
-  EXPECT_GE(now, time);
+  scoped_ptr<XmlElement> stanza2(sent_iq2);
+  ASSERT_TRUE(stanza2 != NULL);
+  ValidateHeartbeatStanza(stanza2.get(),
+                          base::IntToString(kExpectedSequenceId).c_str());
 
-  HostKeyPair key_pair;
-  key_pair.LoadFromString(kTestHostKeyPair);
-  std::string expected_signature =
-      key_pair.GetSignature(std::string(kTestJid) + ' ' + time_str);
-  EXPECT_EQ(expected_signature, signature->BodyText());
+  heartbeat_sender_->OnSignalStrategyStateChange(SignalStrategy::DISCONNECTED);
+  message_loop_.RunAllPending();
 }
 
 // Verify that ProcessResponse parses set-interval result.
-TEST_F(HeartbeatSenderTest, ProcessResponse) {
+TEST_F(HeartbeatSenderTest, ProcessResponseSetInterval) {
   scoped_ptr<XmlElement> response(new XmlElement(buzz::QN_IQ));
   response->AddAttr(QName("", "type"), "result");
 
@@ -156,4 +209,30 @@ TEST_F(HeartbeatSenderTest, ProcessResponse) {
   EXPECT_EQ(kTestInterval * 1000, heartbeat_sender_->interval_ms_);
 }
 
+// Validate a heartbeat stanza.
+void HeartbeatSenderTest::ValidateHeartbeatStanza(
+    XmlElement* stanza, const char* expectedSequenceId) {
+  EXPECT_EQ(stanza->Attr(buzz::QName("", "to")),
+            std::string(kChromotingBotJid));
+  EXPECT_EQ(stanza->Attr(buzz::QName("", "type")), "set");
+  XmlElement* heartbeat_stanza =
+      stanza->FirstNamed(QName(kChromotingXmlNamespace, "heartbeat"));
+  ASSERT_TRUE(heartbeat_stanza != NULL);
+  EXPECT_EQ(expectedSequenceId, heartbeat_stanza->Attr(
+      buzz::QName(kChromotingXmlNamespace, "sequence-id")));
+  EXPECT_EQ(std::string(kHostId),
+            heartbeat_stanza->Attr(QName(kChromotingXmlNamespace, "hostid")));
+
+  QName signature_tag(kChromotingXmlNamespace, "signature");
+  XmlElement* signature = heartbeat_stanza->FirstNamed(signature_tag);
+  ASSERT_TRUE(signature != NULL);
+  EXPECT_TRUE(heartbeat_stanza->NextNamed(signature_tag) == NULL);
+
+  HostKeyPair key_pair;
+  key_pair.LoadFromString(kTestHostKeyPair);
+  std::string expected_signature =
+      key_pair.GetSignature(std::string(kTestJid) + ' ' + expectedSequenceId);
+  EXPECT_EQ(expected_signature, signature->BodyText());
+}
+
 }  // namespace remoting