[GCM] Fixing the client interval interaction with heartbeat ack

This patch fixes the case, where heartbeat interval is lowered to
1 minute after a heartbeat is initiated by the gcm client.

It also handles the case, where custom interval was set to a higher
value, but because of no reconnection, we are keeping the lowest
known value. (Caught by existing unit test).

BUG=591490
R=zea@chromium.org

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

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

3 files changed, 61 insertions, 28 deletions

diff --git a/google_apis/gcm/engine/heartbeat_manager.cc b/google_apis/gcm/engine/heartbeat_manager.cc
index 8ac70b8..c7ccea1 100644
--- a/google_apis/gcm/engine/heartbeat_manager.cc
+++ b/google_apis/gcm/engine/heartbeat_manager.cc
@@ -71,6 +71,9 @@ void HeartbeatManager::Start(
   if (monitor)
     monitor->AddObserver(this);
 
+  // Calculated the heartbeat interval just before we start the timer.
+  UpdateHeartbeatInterval();
+
   // Kicks off the timer.
   waiting_for_ack_ = false;
   RestartTimer();
@@ -106,6 +109,10 @@ void HeartbeatManager::UpdateHeartbeatConfig(
   }
   DVLOG(1) << "Updating server heartbeat interval to " << config.interval_ms();
   server_interval_ms_ = config.interval_ms();
+
+  // Make sure heartbeat interval is recalculated when new server interval is
+  // available.
+  UpdateHeartbeatInterval();
 }
 
 base::TimeTicks HeartbeatManager::GetNextHeartbeatTime() const {
@@ -166,39 +173,20 @@ void HeartbeatManager::OnHeartbeatTriggered() {
 }
 
 void HeartbeatManager::RestartTimer() {
-  if (!waiting_for_ack_) {
-    // Recalculate the timer interval based network type.
-    // Server interval takes precedence over client interval, even if the latter
-    // is less.
-    if (server_interval_ms_ != 0) {
-      // If a server interval is set, it overrides any local one.
-      heartbeat_interval_ms_ = server_interval_ms_;
-    } else if (HasClientHeartbeatInterval()) {
-      // Client interval might have been adjusted up, which should only take
-      // effect during a reconnection.
-      if (client_interval_ms_ < heartbeat_interval_ms_ ||
-          heartbeat_interval_ms_ == 0) {
-        heartbeat_interval_ms_ = client_interval_ms_;
-      }
-    } else {
-      heartbeat_interval_ms_ = GetDefaultHeartbeatInterval();
-    }
-    DVLOG(1) << "Sending next heartbeat in "
-             << heartbeat_interval_ms_ << " ms.";
+  int interval_ms = heartbeat_interval_ms_;
+  if (waiting_for_ack_) {
+    interval_ms = kHeartbeatAckDefaultMs;
+    DVLOG(1) << "Resetting timer for ack within " << interval_ms << " ms.";
   } else {
-    heartbeat_interval_ms_ = kHeartbeatAckDefaultMs;
-    DVLOG(1) << "Resetting timer for ack with "
-             << heartbeat_interval_ms_ << " ms interval.";
+    DVLOG(1) << "Sending next heartbeat in " << interval_ms << " ms.";
   }
 
   heartbeat_expected_time_ =
-      base::Time::Now() +
-      base::TimeDelta::FromMilliseconds(heartbeat_interval_ms_);
+      base::Time::Now() + base::TimeDelta::FromMilliseconds(interval_ms);
   heartbeat_timer_->Start(FROM_HERE,
-                         base::TimeDelta::FromMilliseconds(
-                             heartbeat_interval_ms_),
-                         base::Bind(&HeartbeatManager::OnHeartbeatTriggered,
-                                    weak_ptr_factory_.GetWeakPtr()));
+                          base::TimeDelta::FromMilliseconds(interval_ms),
+                          base::Bind(&HeartbeatManager::OnHeartbeatTriggered,
+                                     weak_ptr_factory_.GetWeakPtr()));
 
 #if defined(OS_LINUX) && !defined(OS_CHROMEOS)
   // Windows, Mac, Android, iOS, and Chrome OS all provide a way to be notified
@@ -235,6 +223,25 @@ void HeartbeatManager::CheckForMissedHeartbeat() {
 #endif  // defined(OS_LINUX) && !defined(OS_CHROMEOS)
 }
 
+void HeartbeatManager::UpdateHeartbeatInterval() {
+  // Server interval takes precedence over client interval, even if the latter
+  // is less.
+  if (server_interval_ms_ != 0) {
+    // If a server interval is set, it overrides any local one.
+    heartbeat_interval_ms_ = server_interval_ms_;
+  } else if (HasClientHeartbeatInterval() &&
+             (client_interval_ms_ < heartbeat_interval_ms_ ||
+              heartbeat_interval_ms_ == 0)) {
+    // Client interval might have been adjusted up, which should only take
+    // effect during a reconnection.
+    heartbeat_interval_ms_ = client_interval_ms_;
+  } else if (heartbeat_interval_ms_ == 0) {
+    // If interval is still 0, recalculate it based on network type.
+    heartbeat_interval_ms_ = GetDefaultHeartbeatInterval();
+  }
+  DCHECK_GT(heartbeat_interval_ms_, 0);
+}
+
 int HeartbeatManager::GetDefaultHeartbeatInterval() {
   // For unknown connections, use the longer cellular heartbeat interval.
   int heartbeat_interval_ms = kCellHeartbeatDefaultMs;
diff --git a/google_apis/gcm/engine/heartbeat_manager.h b/google_apis/gcm/engine/heartbeat_manager.h
index fe3bba6..b8efe03 100644
--- a/google_apis/gcm/engine/heartbeat_manager.h
+++ b/google_apis/gcm/engine/heartbeat_manager.h
@@ -91,6 +91,9 @@ class GCM_EXPORT HeartbeatManager : public base::PowerObserver {
   // Restarts the heartbeat timer.
   void RestartTimer();
 
+  // Calculates and sets the current heartbeat interval.
+  void UpdateHeartbeatInterval();
+
   // Calculates default heartbeat interval, depending on current network.
   int GetDefaultHeartbeatInterval();
 
diff --git a/google_apis/gcm/engine/heartbeat_manager_unittest.cc b/google_apis/gcm/engine/heartbeat_manager_unittest.cc
index b7ee684..7efd2f7 100644
--- a/google_apis/gcm/engine/heartbeat_manager_unittest.cc
+++ b/google_apis/gcm/engine/heartbeat_manager_unittest.cc
@@ -287,6 +287,29 @@ TEST_F(HeartbeatManagerTest, ClientIntervalInvalid) {
       manager()->GetMaxClientHeartbeatIntervalMs()));
 }
 
+// Verifies that client interval is reset appropriately after the heartbeat is
+// triggered. See http://crbug.com/591490 for details.
+TEST_F(HeartbeatManagerTest, ClientIntervalAfterHeartbeatTriggered) {
+  const int kCustomIntervalMs = 180 * 1000;  // 180 seconds.
+  manager()->SetClientHeartbeatIntervalMs(kCustomIntervalMs);
+  StartManager();
+
+  // This changes the interval as manager awaits a heartbeat ack.
+  manager()->TriggerHearbeat();
+  const int kDefaultAckIntervalMs = 60 * 1000;  // 60 seconds.
+  base::TimeTicks heartbeat = manager()->GetNextHeartbeatTime();
+  EXPECT_LE(heartbeat - base::TimeTicks::Now(),
+            base::TimeDelta::FromMilliseconds(kDefaultAckIntervalMs));
+
+  // This should reset the interval to the custom interval.
+  manager()->OnHeartbeatAcked();
+  heartbeat = manager()->GetNextHeartbeatTime();
+  EXPECT_GT(heartbeat - base::TimeTicks::Now(),
+            base::TimeDelta::FromMilliseconds(kDefaultAckIntervalMs));
+  EXPECT_LE(heartbeat - base::TimeTicks::Now(),
+            base::TimeDelta::FromMilliseconds(kCustomIntervalMs));
+}
+
 }  // namespace
 
 }  // namespace gcm