[CrOS PhoneHub] Fail gracefully if no phone status is received.

If the phone is observed to be in the connected state, but no phone
status is observed after 10 seconds, disconnect from the phone.

Fixed: 1155652
Bug: 1106937
Change-Id: I18d2a396d4624220405fcbd479ff36bc78c2a0bc
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2580518
Commit-Queue: Regan Hsu <hsuregan@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#836603}

chromeos/components/phonehub/BUILD.gn

@@ -38,6 +38,8 @@ static_library("phonehub") {
     "find_my_device_controller.h",
     "find_my_device_controller_impl.cc",
     "find_my_device_controller_impl.h",
+    "invalid_connection_disconnector.cc",
+    "invalid_connection_disconnector.h",
     "message_receiver.cc",
     "message_receiver.h",
     "message_receiver_impl.cc",
@@ -179,6 +181,7 @@ source_set("unit_tests") {
     "do_not_disturb_controller_impl_unittest.cc",
     "feature_status_provider_impl_unittest.cc",
     "find_my_device_controller_impl_unittest.cc",
+    "invalid_connection_disconnector_unittest.cc",
     "message_receiver_unittest.cc",
     "message_sender_unittest.cc",
     "multidevice_setup_state_updater_unittest.cc",

chromeos/components/phonehub/invalid_connection_disconnector.cc

+// Copyright 2020 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 "chromeos/components/phonehub/invalid_connection_disconnector.h"
+
+#include "base/time/time.h"
+#include "base/timer/timer.h"
+#include "chromeos/components/multidevice/logging/logging.h"
+#include "chromeos/components/phonehub/phone_model.h"
+
+namespace chromeos {
+namespace phonehub {
+namespace {
+
+// The grace period time for the phone status model to remain non-empty when
+// the phone is connected. If the phone status model is still empty after this
+// period of time, ConnectionManager should call Disconnect().
+constexpr base::TimeDelta kEmptyPhoneStatusModelGracePeriodTimeDelta =
+    base::TimeDelta::FromSeconds(10);
+
+}  // namespace
+
+InvalidConnectionDisconnector::InvalidConnectionDisconnector(
+    ConnectionManager* connection_manager,
+    PhoneModel* phone_model)
+    : InvalidConnectionDisconnector(connection_manager,
+                                    phone_model,
+                                    std::make_unique<base::OneShotTimer>()) {}
+
+InvalidConnectionDisconnector::InvalidConnectionDisconnector(
+    ConnectionManager* connection_manager,
+    PhoneModel* phone_model,
+    std::unique_ptr<base::OneShotTimer> timer)
+    : connection_manager_(connection_manager),
+      phone_model_(phone_model),
+      timer_(std::move(timer)) {
+  connection_manager_->AddObserver(this);
+}
+
+InvalidConnectionDisconnector::~InvalidConnectionDisconnector() {
+  connection_manager_->RemoveObserver(this);
+}
+
+void InvalidConnectionDisconnector::OnConnectionStatusChanged() {
+  timer_->AbandonAndStop();
+
+  if (IsPhoneConnected() && !DoesPhoneStatusModelExist()) {
+    timer_->Start(FROM_HERE, kEmptyPhoneStatusModelGracePeriodTimeDelta,
+                  base::BindOnce(&InvalidConnectionDisconnector::OnTimerFired,
+                                 base::Unretained(this)));
+  }
+}
+
+void InvalidConnectionDisconnector::OnTimerFired() {
+  if (!IsPhoneConnected() || DoesPhoneStatusModelExist())
+    return;
+
+  PA_LOG(INFO) << "Grace period ended for an empty PhoneStatusModel while in "
+                  "the connected state; disconnecting from phone";
+
+  connection_manager_->Disconnect();
+}
+
+bool InvalidConnectionDisconnector::IsPhoneConnected() const {
+  return connection_manager_->GetStatus() ==
+         ConnectionManager::Status::kConnected;
+}
+
+bool InvalidConnectionDisconnector::DoesPhoneStatusModelExist() const {
+  return phone_model_->phone_status_model().has_value();
+}
+
+}  // namespace phonehub
+}  // namespace chromeos

chromeos/components/phonehub/invalid_connection_disconnector.h

+// Copyright 2020 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.
+
+#ifndef CHROMEOS_COMPONENTS_PHONEHUB_INVALID_CONNECTION_DISCONNECTOR_H_
+#define CHROMEOS_COMPONENTS_PHONEHUB_INVALID_CONNECTION_DISCONNECTOR_H_
+
+#include "chromeos/components/phonehub/connection_manager.h"
+
+namespace base {
+class OneShotTimer;
+}  // namespace base
+
+namespace chromeos {
+namespace phonehub {
+
+class PhoneModel;
+
+// Disconnects the phone if the ConnectionManager is in the kConnected
+// state, but the PhoneStatusModel remains empty after a grace period.
+class InvalidConnectionDisconnector : public ConnectionManager::Observer {
+ public:
+  InvalidConnectionDisconnector(ConnectionManager* connection_manager,
+                                PhoneModel* phone_model);
+  ~InvalidConnectionDisconnector() override;
+
+  InvalidConnectionDisconnector(const InvalidConnectionDisconnector&) = delete;
+  InvalidConnectionDisconnector* operator=(
+      const InvalidConnectionDisconnector&) = delete;
+
+ private:
+  friend class InvalidConnectionDisconnectorTest;
+
+  InvalidConnectionDisconnector(ConnectionManager* connection_manager,
+                                PhoneModel* phone_model,
+                                std::unique_ptr<base::OneShotTimer> timer);
+
+  // ConnectionManager::Observer:
+  void OnConnectionStatusChanged() override;
+
+  void UpdateTimer();
+  void OnTimerFired();
+
+  bool IsPhoneConnected() const;
+  bool DoesPhoneStatusModelExist() const;
+
+  ConnectionManager* connection_manager_;
+  PhoneModel* phone_model_;
+  std::unique_ptr<base::OneShotTimer> timer_;
+};
+
+}  // namespace phonehub
+}  // namespace chromeos
+
+#endif  // CHROMEOS_COMPONENTS_PHONEHUB_INVALID_CONNECTION_DISCONNECTOR_H_

chromeos/components/phonehub/invalid_connection_disconnector_unittest.cc

+// Copyright 2020 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 "chromeos/components/phonehub/invalid_connection_disconnector.h"
+
+#include "base/memory/ptr_util.h"
+#include "base/timer/mock_timer.h"
+#include "chromeos/components/phonehub/fake_connection_manager.h"
+#include "chromeos/components/phonehub/mutable_phone_model.h"
+#include "chromeos/components/phonehub/phone_model_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace chromeos {
+namespace phonehub {
+
+class InvalidConnectionDisconnectorTest : public testing::Test {
+ public:
+  InvalidConnectionDisconnectorTest() = default;
+  InvalidConnectionDisconnectorTest(const InvalidConnectionDisconnectorTest&) =
+      delete;
+  InvalidConnectionDisconnectorTest& operator=(
+      const InvalidConnectionDisconnectorTest&) = delete;
+  ~InvalidConnectionDisconnectorTest() override = default;
+
+  // testing::Test:
+  void SetUp() override {
+    fake_connection_manager_ = std::make_unique<FakeConnectionManager>();
+
+    auto timer = std::make_unique<base::MockOneShotTimer>();
+    timer_ = timer.get();
+
+    invalid_connection_disconnector_ =
+        base::WrapUnique(new InvalidConnectionDisconnector(
+            fake_connection_manager_.get(), &fake_phone_model_,
+            std::move(timer)));
+  }
+
+  void FireTimer() { timer_->Fire(); }
+
+  size_t num_disconnect_calls() const {
+    return fake_connection_manager_->num_disconnect_calls();
+  }
+
+  void SetPhoneConnected() {
+    fake_connection_manager_->SetStatus(ConnectionManager::Status::kConnected);
+  }
+
+  void SetPhoneDisconnected() {
+    fake_connection_manager_->SetStatus(
+        ConnectionManager::Status::kDisconnected);
+  }
+
+  void ClearPhoneModel() {
+    fake_phone_model_.SetPhoneStatusModel(base::nullopt);
+  }
+
+  void SetPhoneModel() {
+    fake_phone_model_.SetPhoneStatusModel(CreateFakePhoneStatusModel());
+  }
+
+  bool IsTimerRunning() const { return timer_->IsRunning(); }
+
+ private:
+  std::unique_ptr<FakeConnectionManager> fake_connection_manager_;
+  MutablePhoneModel fake_phone_model_;
+  base::MockOneShotTimer* timer_;
+  std::unique_ptr<InvalidConnectionDisconnector>
+      invalid_connection_disconnector_;
+};
+
+TEST_F(InvalidConnectionDisconnectorTest, DisconnectFlows) {
+  // A disconnection should occur when the phone becomes connected and phone
+  // status model is empty after grace period.
+  ClearPhoneModel();
+  EXPECT_FALSE(IsTimerRunning());
+  SetPhoneConnected();
+  EXPECT_TRUE(IsTimerRunning());
+  FireTimer();
+  EXPECT_EQ(num_disconnect_calls(), 1U);
+
+  // No disconnection should occur when the phone becomes connected and phone
+  // status model is non-empty before grace period.
+  EXPECT_FALSE(IsTimerRunning());
+  SetPhoneConnected();
+  EXPECT_TRUE(IsTimerRunning());
+  SetPhoneModel();
+  FireTimer();
+  EXPECT_EQ(num_disconnect_calls(), 1U);
+
+  // A change in connection status while the timer is running will cause the
+  // timer to stop.
+  ClearPhoneModel();
+  SetPhoneDisconnected();
+  EXPECT_FALSE(IsTimerRunning());
+  SetPhoneConnected();
+  EXPECT_TRUE(IsTimerRunning());
+  SetPhoneDisconnected();
+  EXPECT_FALSE(IsTimerRunning());
+}
+
+}  // namespace phonehub
+}  // namespace chromeos

chromeos/components/phonehub/phone_hub_manager_impl.cc

@@ -13,6 +13,7 @@
 #include "chromeos/components/phonehub/do_not_disturb_controller_impl.h"
 #include "chromeos/components/phonehub/feature_status_provider_impl.h"
 #include "chromeos/components/phonehub/find_my_device_controller_impl.h"
+#include "chromeos/components/phonehub/invalid_connection_disconnector.h"
 #include "chromeos/components/phonehub/message_receiver_impl.h"
 #include "chromeos/components/phonehub/message_sender_impl.h"
 #include "chromeos/components/phonehub/multidevice_setup_state_updater.h"
@@ -105,7 +106,11 @@ PhoneHubManagerImpl::PhoneHubManagerImpl(
           std::make_unique<MultideviceSetupStateUpdater>(
               pref_service,
               multidevice_setup_client,
-              notification_access_manager_.get())) {}
+              notification_access_manager_.get())),
+      invalid_connection_disconnector_(
+          std::make_unique<InvalidConnectionDisconnector>(
+              connection_manager_.get(),
+              phone_model_.get())) {}
 
 PhoneHubManagerImpl::~PhoneHubManagerImpl() = default;
 
@@ -152,6 +157,7 @@ UserActionRecorder* PhoneHubManagerImpl::GetUserActionRecorder() {
 // These should be destroyed in the opposite order of how these objects are
 // initialized in the constructor.
 void PhoneHubManagerImpl::Shutdown() {
+  invalid_connection_disconnector_.reset();
   multidevice_setup_state_updater_.reset();
   browser_tabs_model_controller_.reset();
   browser_tabs_model_provider_.reset();

chromeos/components/phonehub/phone_hub_manager_impl.h

@@ -33,6 +33,7 @@ class BrowserTabsModelController;
 class BrowserTabsModelProvider;
 class ConnectionManager;
 class CrosStateSender;
+class InvalidConnectionDisconnector;
 class MessageSender;
 class MessageReceiver;
 class MultideviceSetupStateUpdater;
@@ -88,6 +89,8 @@ class PhoneHubManagerImpl : public PhoneHubManager, public KeyedService {
   std::unique_ptr<BrowserTabsModelController> browser_tabs_model_controller_;
   std::unique_ptr<MultideviceSetupStateUpdater>
       multidevice_setup_state_updater_;
+  std::unique_ptr<InvalidConnectionDisconnector>
+      invalid_connection_disconnector_;
 };
 
 }  // namespace phonehub