[CrOS PhoneHub] Retry sending phone status if phone status model empty.

If after sending the phone status to the phone, and CrOS does not
receive a response from the phone that updates the phone status model,
resend the phone status after waiting 2 secs, then 4 sec, then 8 sec,
etc until the phone status model is populated or the connection status
is no longer connected.

Bug: 1166844
Change-Id: Icf456c941a48704119af2beb3c5a694b2e39b2ef
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2639307
Commit-Queue: Regan Hsu <hsuregan@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#845690}

chromeos/components/phonehub/cros_state_sender.cc

@@ -6,10 +6,24 @@
 
 #include "chromeos/components/multidevice/logging/logging.h"
 #include "chromeos/components/phonehub/message_sender.h"
+#include "chromeos/components/phonehub/phone_model.h"
 #include "chromeos/services/multidevice_setup/public/mojom/multidevice_setup.mojom.h"
 
 namespace chromeos {
 namespace phonehub {
+namespace {
+
+// The minimum time to wait before checking whether the phone has responded to
+// status messages sent by CrosStateSender, and re-sending the status messages
+// if there was no response (no phone status model exists).
+constexpr base::TimeDelta kMinimumRetryDelay = base::TimeDelta::FromSeconds(2u);
+
+// The amount the previous delay is multiplied by to determine the new amount
+// of time to wait before determining whether CrosStateSender should resend the
+// CrOS State. Follows a doubling sequence, e.g 2 sec, 4 sec, 8 sec... etc.
+constexpr int kRetryDelayMultiplier = 2;
+
+}  // namespace
 
 using multidevice_setup::mojom::Feature;
 using multidevice_setup::mojom::FeatureState;
@@ -17,13 +31,31 @@ using multidevice_setup::mojom::FeatureState;
 CrosStateSender::CrosStateSender(
     MessageSender* message_sender,
     ConnectionManager* connection_manager,
-    multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client)
+    multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
+    PhoneModel* phone_model)
+    : CrosStateSender(message_sender,
+                      connection_manager,
+                      multidevice_setup_client,
+                      phone_model,
+                      std::make_unique<base::OneShotTimer>()) {}
+
+CrosStateSender::CrosStateSender(
+    MessageSender* message_sender,
+    ConnectionManager* connection_manager,
+    multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
+    PhoneModel* phone_model,
+    std::unique_ptr<base::OneShotTimer> timer)
     : message_sender_(message_sender),
       connection_manager_(connection_manager),
-      multidevice_setup_client_(multidevice_setup_client) {
+      multidevice_setup_client_(multidevice_setup_client),
+      phone_model_(phone_model),
+      retry_timer_(std::move(timer)),
+      retry_delay_(kMinimumRetryDelay) {
   DCHECK(message_sender_);
   DCHECK(connection_manager_);
   DCHECK(multidevice_setup_client_);
+  DCHECK(phone_model_);
+  DCHECK(retry_timer_);
 
   connection_manager_->AddObserver(this);
   multidevice_setup_client_->AddObserver(this);
@@ -34,16 +66,25 @@ CrosStateSender::~CrosStateSender() {
   multidevice_setup_client_->RemoveObserver(this);
 }
 
-void CrosStateSender::AttemptUpdateCrosState() const {
+void CrosStateSender::AttemptUpdateCrosState() {
+  // Stop and cancel old timer if it is running, and reset the |retry_delay_| to
+  // |kMinimumRetryDelay|.
+  retry_timer_->Stop();
+  retry_delay_ = kMinimumRetryDelay;
+
   // Wait for connection to be established.
   if (connection_manager_->GetStatus() !=
       ConnectionManager::Status::kConnected) {
-    PA_LOG(INFO) << "Could not start AttemptUpdateCrosState() because "
+    PA_LOG(VERBOSE) << "Could not start AttemptUpdateCrosState() because "
                     << "connection manager status is: "
                     << connection_manager_->GetStatus();
     return;
   }
 
+  PerformUpdateCrosState();
+}
+
+void CrosStateSender::PerformUpdateCrosState() {
   bool are_notifications_enabled =
       multidevice_setup_client_->GetFeatureState(
           Feature::kPhoneHubNotifications) == FeatureState::kEnabledByUser;
@@ -51,6 +92,25 @@ void CrosStateSender::AttemptUpdateCrosState() const {
   PA_LOG(INFO) << "Attempting to send cros state with notifications enabled "
                << "state as: " << are_notifications_enabled;
   message_sender_->SendCrosState(are_notifications_enabled);
+
+  retry_timer_->Start(FROM_HERE, retry_delay_,
+                      base::BindOnce(&CrosStateSender::OnRetryTimerFired,
+                                     base::Unretained(this)));
+}
+
+void CrosStateSender::OnRetryTimerFired() {
+  // If the phone status model is non-null, implying that the phone has
+  // responded to the previous PerformUpdateCrosState(), or if the
+  // connection status is no longer in the connected state, do not
+  // retry sending the cros state.
+  if (phone_model_->phone_status_model().has_value() ||
+      connection_manager_->GetStatus() !=
+          ConnectionManager::Status::kConnected) {
+    return;
+  }
+
+  retry_delay_ *= kRetryDelayMultiplier;
+  PerformUpdateCrosState();
 }
 
 void CrosStateSender::OnConnectionStatusChanged() {

chromeos/components/phonehub/cros_state_sender.h

@@ -5,6 +5,7 @@
 #ifndef CHROMEOS_COMPONENTS_PHONEHUB_CROS_STATE_SENDER_H_
 #define CHROMEOS_COMPONENTS_PHONEHUB_CROS_STATE_SENDER_H_
 
+#include "base/timer/timer.h"
 #include "chromeos/components/phonehub/connection_manager.h"
 #include "chromeos/services/multidevice_setup/public/cpp/multidevice_setup_client.h"
 
@@ -12,6 +13,7 @@ namespace chromeos {
 namespace phonehub {
 
 class MessageSender;
+class PhoneModel;
 
 // Responsible for sending the Chrome OS's device state to the user's
 // phone.
@@ -22,11 +24,21 @@ class CrosStateSender
   CrosStateSender(
       MessageSender* message_sender,
       ConnectionManager* connection_manager,
-      multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client);
+      multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
+      PhoneModel* phone_model);
   ~CrosStateSender() override;
 
  private:
-  void AttemptUpdateCrosState() const;
+  friend class CrosStateSenderTest;
+
+  CrosStateSender(
+      MessageSender* message_sender,
+      ConnectionManager* connection_manager,
+      multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client,
+      PhoneModel* phone_model,
+      std::unique_ptr<base::OneShotTimer> timer);
+
+  void AttemptUpdateCrosState();
 
   // ConnectionManager::Observer:
   void OnConnectionStatusChanged() override;
@@ -36,9 +48,17 @@ class CrosStateSender
       const multidevice_setup::MultiDeviceSetupClient::FeatureStatesMap&
           feature_states_map) override;
 
+  // Sends the cros state to the phone, and initiates a retry after
+  // |retry_delay_| if the message was not successfully sent.
+  void PerformUpdateCrosState();
+  void OnRetryTimerFired();
+
   MessageSender* message_sender_;
   ConnectionManager* connection_manager_;
   multidevice_setup::MultiDeviceSetupClient* multidevice_setup_client_;
+  PhoneModel* phone_model_;
+  std::unique_ptr<base::OneShotTimer> retry_timer_;
+  base::TimeDelta retry_delay_;
 };
 
 }  // namespace phonehub

chromeos/components/phonehub/cros_state_sender_unittest.cc

@@ -6,8 +6,11 @@
 
 #include <memory>
 
+#include "base/timer/mock_timer.h"
 #include "chromeos/components/phonehub/fake_connection_manager.h"
 #include "chromeos/components/phonehub/fake_message_sender.h"
+#include "chromeos/components/phonehub/mutable_phone_model.h"
+#include "chromeos/components/phonehub/phone_model_test_util.h"
 #include "chromeos/services/multidevice_setup/public/cpp/fake_multidevice_setup_client.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -27,53 +30,115 @@ class CrosStateSenderTest : public testing::Test {
 
   // testing::Test:
   void SetUp() override {
+    auto timer = std::make_unique<base::MockOneShotTimer>();
+    mock_timer_ = timer.get();
+
     fake_message_sender_ = std::make_unique<FakeMessageSender>();
     fake_connection_manager_ = std::make_unique<FakeConnectionManager>();
     fake_multidevice_setup_client_ =
         std::make_unique<multidevice_setup::FakeMultiDeviceSetupClient>();
-    cros_state_sender_ = std::make_unique<CrosStateSender>(
+    phone_model_ = std::make_unique<MutablePhoneModel>();
+    cros_state_sender_ = base::WrapUnique(new CrosStateSender(
         fake_message_sender_.get(), fake_connection_manager_.get(),
-        fake_multidevice_setup_client_.get());
+        fake_multidevice_setup_client_.get(), phone_model_.get(),
+        std::move(timer)));
   }
 
+  base::TimeDelta GetRetryDelay() { return cros_state_sender_->retry_delay_; }
+
   std::unique_ptr<FakeMessageSender> fake_message_sender_;
   std::unique_ptr<FakeConnectionManager> fake_connection_manager_;
   std::unique_ptr<multidevice_setup::FakeMultiDeviceSetupClient>
       fake_multidevice_setup_client_;
+  std::unique_ptr<MutablePhoneModel> phone_model_;
+  base::MockOneShotTimer* mock_timer_;
 
  private:
   std::unique_ptr<CrosStateSender> cros_state_sender_;
 };
 
+TEST_F(CrosStateSenderTest, PerformUpdateCrosStateRetrySequence) {
+  fake_connection_manager_->SetStatus(ConnectionManager::Status::kConnected);
+  EXPECT_EQ(1u, fake_message_sender_->GetCrosStateCallCount());
+
+  // The retry time follows a doubling sequence.
+  EXPECT_EQ(base::TimeDelta::FromSeconds(2u), GetRetryDelay());
+  mock_timer_->Fire();
+  EXPECT_TRUE(mock_timer_->IsRunning());
+  EXPECT_EQ(2u, fake_message_sender_->GetCrosStateCallCount());
+
+  EXPECT_EQ(base::TimeDelta::FromSeconds(4u), GetRetryDelay());
+  mock_timer_->Fire();
+  EXPECT_TRUE(mock_timer_->IsRunning());
+  EXPECT_EQ(3u, fake_message_sender_->GetCrosStateCallCount());
+
+  EXPECT_EQ(base::TimeDelta::FromSeconds(8u), GetRetryDelay());
+  mock_timer_->Fire();
+  EXPECT_TRUE(mock_timer_->IsRunning());
+  EXPECT_EQ(4u, fake_message_sender_->GetCrosStateCallCount());
+
+  // The phone model becomes populated, stops retrying.
+  EXPECT_EQ(base::TimeDelta::FromSeconds(16u), GetRetryDelay());
+  phone_model_->SetPhoneStatusModel(CreateFakePhoneStatusModel());
+  mock_timer_->Fire();
+  EXPECT_FALSE(mock_timer_->IsRunning());
+  EXPECT_EQ(4u, fake_message_sender_->GetCrosStateCallCount());
+  EXPECT_EQ(base::TimeDelta::FromSeconds(16u), GetRetryDelay());
+
+  fake_connection_manager_->SetStatus(ConnectionManager::Status::kConnecting);
+  EXPECT_FALSE(mock_timer_->IsRunning());
+
+  // Cancellation of the retry timer occurs properly when an attempt is
+  // reinitiated but the status is not connecting.
+  fake_connection_manager_->SetStatus(ConnectionManager::Status::kConnected);
+  EXPECT_TRUE(mock_timer_->IsRunning());
+  fake_connection_manager_->SetStatus(ConnectionManager::Status::kConnecting);
+  EXPECT_FALSE(mock_timer_->IsRunning());
+}
+
 TEST_F(CrosStateSenderTest, UpdatesOnConnected) {
   // Set notification feature to be enabled.
   fake_multidevice_setup_client_->SetFeatureState(
       Feature::kPhoneHubNotifications, FeatureState::kEnabledByUser);
   // Expect no new messages since connection has not been established.
   EXPECT_EQ(0u, fake_message_sender_->GetCrosStateCallCount());
+  EXPECT_FALSE(mock_timer_->IsRunning());
 
   // Update connection state to connecting.
   fake_connection_manager_->SetStatus(ConnectionManager::Status::kConnecting);
   // Connecting state does not trigger a request message.
   EXPECT_EQ(0u, fake_message_sender_->GetCrosStateCallCount());
+  EXPECT_FALSE(mock_timer_->IsRunning());
 
   // Simulate connected state. Expect a new message to be sent.
   fake_connection_manager_->SetStatus(ConnectionManager::Status::kConnected);
   EXPECT_TRUE(fake_message_sender_->GetRecentCrosState());
   EXPECT_EQ(1u, fake_message_sender_->GetCrosStateCallCount());
 
+  // Phone model is populated.
+  phone_model_->SetPhoneStatusModel(CreateFakePhoneStatusModel());
+  mock_timer_->Fire();
+  EXPECT_EQ(1u, fake_message_sender_->GetCrosStateCallCount());
+
   // Simulate disconnected state, this should not trigger a new request.
   fake_connection_manager_->SetStatus(ConnectionManager::Status::kDisconnected);
   EXPECT_TRUE(fake_message_sender_->GetRecentCrosState());
   EXPECT_EQ(1u, fake_message_sender_->GetCrosStateCallCount());
+  EXPECT_FALSE(mock_timer_->IsRunning());
 }
 
 TEST_F(CrosStateSenderTest, NotificationFeatureStateChanged) {
   // Set connection state to be connected.
   fake_connection_manager_->SetStatus(ConnectionManager::Status::kConnected);
+
+  // Phone model is populated.
+  phone_model_->SetPhoneStatusModel(CreateFakePhoneStatusModel());
+  EXPECT_TRUE(mock_timer_->IsRunning());
+
   // Expect new messages to be sent when connection state is connected.
   EXPECT_FALSE(fake_message_sender_->GetRecentCrosState());
   EXPECT_EQ(1u, fake_message_sender_->GetCrosStateCallCount());
+  mock_timer_->Fire();
 
   // Simulate enabling notification feature state and expect cros state to be
   // enabled.
@@ -81,6 +146,7 @@ TEST_F(CrosStateSenderTest, NotificationFeatureStateChanged) {
       Feature::kPhoneHubNotifications, FeatureState::kEnabledByUser);
   EXPECT_TRUE(fake_message_sender_->GetRecentCrosState());
   EXPECT_EQ(2u, fake_message_sender_->GetCrosStateCallCount());
+  mock_timer_->Fire();
 
   // Update a different feature state and expect that it did not affect the
   // cros state.
@@ -88,6 +154,7 @@ TEST_F(CrosStateSenderTest, NotificationFeatureStateChanged) {
       Feature::kSmartLock, FeatureState::kDisabledByUser);
   EXPECT_TRUE(fake_message_sender_->GetRecentCrosState());
   EXPECT_EQ(3u, fake_message_sender_->GetCrosStateCallCount());
+  mock_timer_->Fire();
 
   // Simulate disabling notification feature state and expect cros state to be
   // disabled.
@@ -95,6 +162,10 @@ TEST_F(CrosStateSenderTest, NotificationFeatureStateChanged) {
       Feature::kPhoneHubNotifications, FeatureState::kDisabledByUser);
   EXPECT_FALSE(fake_message_sender_->GetRecentCrosState());
   EXPECT_EQ(4u, fake_message_sender_->GetCrosStateCallCount());
+
+  // Firing the timer does not cause the cros state to be sent again.
+  mock_timer_->Fire();
+  EXPECT_EQ(4u, fake_message_sender_->GetCrosStateCallCount());
 }
 
 }  // namespace phonehub

chromeos/components/phonehub/phone_hub_manager_impl.cc

@@ -53,10 +53,12 @@ PhoneHubManagerImpl::PhoneHubManagerImpl(
           std::make_unique<MessageReceiverImpl>(connection_manager_.get())),
       message_sender_(
           std::make_unique<MessageSenderImpl>(connection_manager_.get())),
+      phone_model_(std::make_unique<MutablePhoneModel>()),
       cros_state_sender_(
           std::make_unique<CrosStateSender>(message_sender_.get(),
                                             connection_manager_.get(),
-                                            multidevice_setup_client)),
+                                            multidevice_setup_client,
+                                            phone_model_.get())),
       do_not_disturb_controller_(std::make_unique<DoNotDisturbControllerImpl>(
           message_sender_.get(),
           user_action_recorder_.get())),
@@ -81,7 +83,6 @@ PhoneHubManagerImpl::PhoneHubManagerImpl(
           feature_status_provider_.get(),
           multidevice_setup_client,
           show_multidevice_setup_dialog_callback)),
-      phone_model_(std::make_unique<MutablePhoneModel>()),
       phone_status_processor_(std::make_unique<PhoneStatusProcessor>(
           do_not_disturb_controller_.get(),
           feature_status_provider_.get(),
@@ -166,7 +167,6 @@ void PhoneHubManagerImpl::Shutdown() {
   browser_tabs_model_provider_.reset();
   tether_controller_.reset();
   phone_status_processor_.reset();
-  phone_model_.reset();
   onboarding_ui_tracker_.reset();
   notification_manager_.reset();
   notification_access_manager_.reset();
@@ -174,6 +174,7 @@ void PhoneHubManagerImpl::Shutdown() {
   connection_scheduler_.reset();
   do_not_disturb_controller_.reset();
   cros_state_sender_.reset();
+  phone_model_.reset();
   message_sender_.reset();
   message_receiver_.reset();
   feature_status_provider_.reset();

chromeos/components/phonehub/phone_hub_manager_impl.h

@@ -76,6 +76,7 @@ class PhoneHubManagerImpl : public PhoneHubManager, public KeyedService {
   std::unique_ptr<FeatureStatusProvider> feature_status_provider_;
   std::unique_ptr<MessageReceiver> message_receiver_;
   std::unique_ptr<MessageSender> message_sender_;
+  std::unique_ptr<MutablePhoneModel> phone_model_;
   std::unique_ptr<CrosStateSender> cros_state_sender_;
   std::unique_ptr<DoNotDisturbController> do_not_disturb_controller_;
   std::unique_ptr<ConnectionScheduler> connection_scheduler_;
@@ -83,7 +84,6 @@ class PhoneHubManagerImpl : public PhoneHubManager, public KeyedService {
   std::unique_ptr<NotificationAccessManager> notification_access_manager_;
   std::unique_ptr<NotificationManager> notification_manager_;
   std::unique_ptr<OnboardingUiTracker> onboarding_ui_tracker_;
-  std::unique_ptr<MutablePhoneModel> phone_model_;
   std::unique_ptr<PhoneStatusProcessor> phone_status_processor_;
   std::unique_ptr<TetherController> tether_controller_;
   std::unique_ptr<BrowserTabsModelProvider> browser_tabs_model_provider_;