[CrOS Tether] Create CrashRecoveryManager, which restores Tether state after a crash.

Previously, if a Tether connection was active and the browser crashed, the UI would start back up and show the device connected to the raw Wi-Fi network instead of the special Tether network; furthermore, all host scan results were gone and the device would not continue to send KeepAliveTickle messages to the active host, resulting in the phone's hotspot shutting down automatically.

This CL creates a class which is run at the end of initialization of the Tether module, before a host scan begins. If it detects that there was saved active host data in persistent storage, it knows that the last session ended due to a crash since persistent active host data is cleared under the normal shutdown flow. If this is the case, CrashRecoveryManager restores the connection.

Bug: 672263, 737273
Change-Id: Id51967e20855edb086285f5bfdb92c4677bee77c
Reviewed-on: https://chromium-review.googlesource.com/572840
Commit-Queue: Kyle Horimoto <khorimoto@chromium.org>
Reviewed-by: Ryan Hansberry <hansberry@chromium.org>
Cr-Commit-Position: refs/heads/master@{#487282}

chromeos/components/tether/BUILD.gn

@@ -22,6 +22,8 @@ static_library("tether") {
     "ble_scanner.h",
     "connect_tethering_operation.cc",
     "connect_tethering_operation.h",
+    "crash_recovery_manager.cc",
+    "crash_recovery_manager.h",
     "device_id_tether_network_guid_map.cc",
     "device_id_tether_network_guid_map.h",
     "device_status_util.cc",
@@ -156,6 +158,7 @@ source_set("unit_tests") {
     "ble_connection_manager_unittest.cc",
     "ble_scanner_unittest.cc",
     "connect_tethering_operation_unittest.cc",
+    "crash_recovery_manager_unittest.cc",
     "device_status_util_unittest.cc",
     "disconnect_tethering_operation_unittest.cc",
     "host_connection_metrics_logger_unittest.cc",

chromeos/components/tether/active_host.h

@@ -140,6 +140,8 @@ class ActiveHost {
       const std::string& new_wifi_network_guid);
 
  private:
+  friend class CrashRecoveryManager;
+
   void SetActiveHost(ActiveHostStatus active_host_status,
                      const std::string& active_host_device_id,
                      const std::string& tether_network_guid,

chromeos/components/tether/crash_recovery_manager.cc

+// Copyright 2017 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/tether/crash_recovery_manager.h"
+
+#include "base/bind.h"
+#include "base/memory/weak_ptr.h"
+#include "chromeos/components/tether/host_scan_cache.h"
+#include "chromeos/network/network_state.h"
+#include "chromeos/network/network_state_handler.h"
+#include "components/cryptauth/remote_device.h"
+#include "components/proximity_auth/logging/logging.h"
+
+namespace chromeos {
+
+namespace tether {
+
+CrashRecoveryManager::CrashRecoveryManager(
+    NetworkStateHandler* network_state_handler,
+    ActiveHost* active_host,
+    HostScanCache* host_scan_cache)
+    : network_state_handler_(network_state_handler),
+      active_host_(active_host),
+      host_scan_cache_(host_scan_cache),
+      weak_ptr_factory_(this) {}
+
+CrashRecoveryManager::~CrashRecoveryManager() {}
+
+void CrashRecoveryManager::RestorePreCrashStateIfNecessary(
+    const base::Closure& on_restoration_finished) {
+  ActiveHost::ActiveHostStatus status = active_host_->GetActiveHostStatus();
+  std::string active_host_device_id = active_host_->GetActiveHostDeviceId();
+  std::string wifi_network_guid = active_host_->GetWifiNetworkGuid();
+  std::string tether_network_guid = active_host_->GetTetherNetworkGuid();
+
+  if (status == ActiveHost::ActiveHostStatus::DISCONNECTED) {
+    // There was no active Tether session, so either the last Tether component
+    // shutdown occurred normally (i.e., without a crash), or it occurred due
+    // to a crash and there was no active host at the time of the crash.
+    on_restoration_finished.Run();
+    return;
+  }
+
+  if (status == ActiveHost::ActiveHostStatus::CONNECTING) {
+    // If a connection attempt was in progress when the crash occurred, abandon
+    // the connection attempt. Set ActiveHost back to DISCONNECTED; the user can
+    // attempt another connection if desired.
+    // TODO(khorimoto): Explore whether we should attempt to restore an
+    // in-progress connection attempt. This is a low-priority edge case which is
+    // difficult to solve.
+    PA_LOG(WARNING) << "Browser crashed while Tether connection attempt was in "
+                    << "progress. Abandoning connection attempt.";
+    active_host_->SetActiveHostDisconnected();
+    on_restoration_finished.Run();
+    return;
+  }
+
+  RestoreConnectedState(on_restoration_finished, active_host_device_id,
+                        tether_network_guid, wifi_network_guid);
+}
+
+void CrashRecoveryManager::RestoreConnectedState(
+    const base::Closure& on_restoration_finished,
+    const std::string& active_host_device_id,
+    const std::string& tether_network_guid,
+    const std::string& wifi_network_guid) {
+  // Since the host was connected, both a Wi-Fi and Tether network GUID are
+  // expected to be present.
+  DCHECK(!wifi_network_guid.empty() && !tether_network_guid.empty());
+
+  if (!host_scan_cache_->ExistsInCache(tether_network_guid)) {
+    // If a crash occurred, HostScanCache is expected to have restored its state
+    // via its persistent scan results. If that did not happen correctly, there
+    // is no way to restore the active host, so abandon the connection. Note
+    // that this is not an expected error condition.
+    PA_LOG(ERROR)
+        << "Browser crashed while a Tether connection was active, "
+        << "but the scan result for the active host was lost. Setting "
+        << "the active host to DISCONNECTED.";
+    active_host_->SetActiveHostDisconnected();
+    on_restoration_finished.Run();
+    return;
+  }
+
+  // Since the associated scan result exists in the cache, it is expected to be
+  // present in the network stack.
+  const NetworkState* tether_state =
+      network_state_handler_->GetNetworkStateFromGuid(tether_network_guid);
+  DCHECK(tether_state);
+
+  const NetworkState* wifi_state =
+      network_state_handler_->GetNetworkStateFromGuid(wifi_network_guid);
+  if (!wifi_state || !wifi_state->IsConnectedState()) {
+    // If the Wi-Fi network corresponding to the Tether hotspot is not present
+    // or is no longer connected, then this device is no longer truly connected
+    // to the active host.
+    PA_LOG(ERROR) << "Browser crashed while a Tether connection was active, "
+                  << "but underlying Wi-Fi network corresponding to the Tether "
+                  << "connection is no longer present. Setting the active host "
+                  << "to DISCONNECTED.";
+    active_host_->SetActiveHostDisconnected();
+    on_restoration_finished.Run();
+    return;
+  }
+
+  // Because the NetworkState object representing the Wi-Fi network was lost
+  // during the crash, the association between it and the Tether NetworkState
+  // has been broken. Restore it now.
+  DCHECK(wifi_state->tether_guid().empty());
+  network_state_handler_->AssociateTetherNetworkStateWithWifiNetwork(
+      tether_network_guid, wifi_network_guid);
+
+  active_host_->GetActiveHost(
+      base::Bind(&CrashRecoveryManager::OnActiveHostFetched,
+                 weak_ptr_factory_.GetWeakPtr(), on_restoration_finished));
+}
+
+void CrashRecoveryManager::OnActiveHostFetched(
+    const base::Closure& on_restoration_finished,
+    ActiveHost::ActiveHostStatus active_host_status,
+    std::shared_ptr<cryptauth::RemoteDevice> active_host,
+    const std::string& tether_network_guid,
+    const std::string& wifi_network_guid) {
+  DCHECK(ActiveHost::ActiveHostStatus::CONNECTED == active_host_status);
+  DCHECK(active_host_);
+
+  // Even though the active host has not actually changed, fire an active host
+  // changed update so that classes listening on ActiveHost (e.g.,
+  // ActiveHostNetworkStateUpdater and KeepAliveScheduler) will be notified
+  // that there is an active connection.
+  //
+  // Note: SendActiveHostChangedUpdate() is a protected function of ActiveHost
+  // which is only invoked here because CrashRecoveryManager is a friend class.
+  // It is invoked directly here because we are sending out a "fake" change
+  // event which has equal old and new values.
+  active_host_->SendActiveHostChangedUpdate(
+      ActiveHost::ActiveHostStatus::CONNECTED /* old_status */,
+      active_host->GetDeviceId() /* old_active_host_id */,
+      tether_network_guid /* old_tether_network_guid */,
+      wifi_network_guid /* old_wifi_network_guid */,
+      ActiveHost::ActiveHostStatus::CONNECTED /* new_status */,
+      active_host /* new_active_host */,
+      tether_network_guid /* new_tether_network_guid */,
+      wifi_network_guid /* new_wifi_network_guid */);
+  on_restoration_finished.Run();
+}
+
+}  // namespace tether
+
+}  // namespace chromeos

chromeos/components/tether/crash_recovery_manager.h

+// Copyright 2017 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_TETHER_CRASH_RECOVERY_MANAGER_H_
+#define CHROMEOS_COMPONENTS_TETHER_CRASH_RECOVERY_MANAGER_H_
+
+#include <string>
+
+#include "base/callback.h"
+#include "base/macros.h"
+#include "chromeos/components/tether/active_host.h"
+
+namespace chromeos {
+
+class NetworkStateHandler;
+
+namespace tether {
+
+class HostScanCache;
+
+// Restores Tether state after a browser crash.
+class CrashRecoveryManager {
+ public:
+  CrashRecoveryManager(NetworkStateHandler* network_state_handler,
+                       ActiveHost* active_host,
+                       HostScanCache* host_scan_cache);
+  virtual ~CrashRecoveryManager();
+
+  // Restores state which was lost by a browser crash. If a crash did not occur
+  // the last time that the Tether component was active, this function is a
+  // no-op. If there was an active Tether connection and the browser crashed,
+  // this function restores the Tether connection.
+  //
+  // This function should only be called during the initialization of the Tether
+  // component.
+  void RestorePreCrashStateIfNecessary(
+      const base::Closure& on_restoration_finished);
+
+ private:
+  void RestoreConnectedState(const base::Closure& on_restoration_finished,
+                             const std::string& active_host_device_id,
+                             const std::string& tether_network_guid,
+                             const std::string& wifi_network_guid);
+  void OnActiveHostFetched(const base::Closure& on_restoration_finished,
+                           ActiveHost::ActiveHostStatus active_host_status,
+                           std::shared_ptr<cryptauth::RemoteDevice> active_host,
+                           const std::string& tether_network_guid,
+                           const std::string& wifi_network_guid);
+
+  NetworkStateHandler* network_state_handler_;
+  ActiveHost* active_host_;
+  HostScanCache* host_scan_cache_;
+
+  base::WeakPtrFactory<CrashRecoveryManager> weak_ptr_factory_;
+
+  DISALLOW_COPY_AND_ASSIGN(CrashRecoveryManager);
+};
+
+}  // namespace tether
+
+}  // namespace chromeos
+
+#endif  // CHROMEOS_COMPONENTS_TETHER_CRASH_RECOVERY_MANAGER_H_

chromeos/components/tether/crash_recovery_manager_unittest.cc

+// Copyright 2017 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/tether/crash_recovery_manager.h"
+
+#include <memory>
+#include <sstream>
+
+#include "base/bind.h"
+#include "base/memory/ptr_util.h"
+#include "base/test/scoped_task_environment.h"
+#include "chromeos/components/tether/device_id_tether_network_guid_map.h"
+#include "chromeos/components/tether/fake_active_host.h"
+#include "chromeos/components/tether/fake_host_scan_cache.h"
+#include "chromeos/components/tether/host_scan_cache_entry.h"
+#include "chromeos/dbus/dbus_thread_manager.h"
+#include "chromeos/network/network_state_handler.h"
+#include "chromeos/network/network_state_test.h"
+#include "components/cryptauth/remote_device_test_util.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/cros_system_api/dbus/shill/dbus-constants.h"
+
+namespace chromeos {
+
+namespace tether {
+
+namespace {
+
+constexpr char kWifiNetworkGuid[] = "wifiNetworkGuid";
+
+std::string CreateConfigurationJsonString(bool is_connected) {
+  std::string connection_state =
+      is_connected ? shill::kStateOnline : shill::kStateIdle;
+
+  std::stringstream ss;
+  ss << "{"
+     << "  \"GUID\": \"" << kWifiNetworkGuid << "\","
+     << "  \"Type\": \"" << shill::kTypeWifi << "\","
+     << "  \"State\": \"" << connection_state << "\""
+     << "}";
+  return ss.str();
+}
+
+}  // namespace
+
+class CrashRecoveryManagerTest : public NetworkStateTest {
+ protected:
+  CrashRecoveryManagerTest()
+      : test_device_(cryptauth::GenerateTestRemoteDevices(1u)[0]) {}
+  ~CrashRecoveryManagerTest() override {}
+
+  void SetUp() override {
+    DBusThreadManager::Initialize();
+    NetworkStateTest::SetUp();
+    network_state_handler()->SetTetherTechnologyState(
+        NetworkStateHandler::TECHNOLOGY_ENABLED);
+
+    is_restoration_finished_ = false;
+
+    fake_active_host_ = base::MakeUnique<FakeActiveHost>();
+    fake_host_scan_cache_ = base::MakeUnique<FakeHostScanCache>();
+
+    crash_recovery_manager_ = base::MakeUnique<CrashRecoveryManager>(
+        network_state_handler(), fake_active_host_.get(),
+        fake_host_scan_cache_.get());
+
+    device_id_tether_network_guid_map_ =
+        base::MakeUnique<DeviceIdTetherNetworkGuidMap>();
+  }
+
+  void TearDown() override {
+    ShutdownNetworkState();
+    NetworkStateTest::TearDown();
+    DBusThreadManager::Shutdown();
+  }
+
+  std::string GetTetherNetworkGuid() {
+    return device_id_tether_network_guid_map_->GetTetherNetworkGuidForDeviceId(
+        test_device_.GetDeviceId());
+  }
+
+  void SetConnected() {
+    fake_active_host_->SetActiveHostConnected(
+        test_device_.GetDeviceId(), GetTetherNetworkGuid(), kWifiNetworkGuid);
+  }
+
+  void AddScanEntry() {
+    fake_host_scan_cache_->SetHostScanResult(
+        *HostScanCacheEntry::Builder()
+             .SetTetherNetworkGuid(GetTetherNetworkGuid())
+             .SetDeviceName("deviceName")
+             .SetCarrier("carrier")
+             .SetBatteryPercentage(100)
+             .SetSignalStrength(100)
+             .SetSetupRequired(false)
+             .Build());
+
+    network_state_handler()->AddTetherNetworkState(
+        GetTetherNetworkGuid(), "deviceName", "carrier",
+        100 /* battery_percentage */, 100 /* signal_strength */,
+        false /* has_connected_to_host */);
+  }
+
+  void AddWifiNetwork(bool is_connected) {
+    ConfigureService(CreateConfigurationJsonString(is_connected));
+  }
+
+  void StartRestoration() {
+    crash_recovery_manager_->RestorePreCrashStateIfNecessary(
+        base::Bind(&CrashRecoveryManagerTest::OnRestorationFinished,
+                   base::Unretained(this)));
+  }
+
+  void OnRestorationFinished() { is_restoration_finished_ = true; }
+
+  void VerifyDisconnectedAfterRestoration() {
+    EXPECT_TRUE(is_restoration_finished_);
+    EXPECT_EQ(ActiveHost::ActiveHostStatus::DISCONNECTED,
+              fake_active_host_->GetActiveHostStatus());
+  }
+
+  const base::test::ScopedTaskEnvironment scoped_task_environment_;
+  const cryptauth::RemoteDevice test_device_;
+
+  std::unique_ptr<FakeActiveHost> fake_active_host_;
+  std::unique_ptr<FakeHostScanCache> fake_host_scan_cache_;
+
+  // TODO(hansberry): Use a fake for this when a real mapping scheme is created.
+  std::unique_ptr<DeviceIdTetherNetworkGuidMap>
+      device_id_tether_network_guid_map_;
+
+  bool is_restoration_finished_;
+
+  std::unique_ptr<CrashRecoveryManager> crash_recovery_manager_;
+
+ private:
+  DISALLOW_COPY_AND_ASSIGN(CrashRecoveryManagerTest);
+};
+
+TEST_F(CrashRecoveryManagerTest, ActiveHostDisconnected) {
+  StartRestoration();
+  VerifyDisconnectedAfterRestoration();
+}
+
+TEST_F(CrashRecoveryManagerTest, ActiveHostConnecting) {
+  fake_active_host_->SetActiveHostConnecting(test_device_.GetDeviceId(),
+                                             GetTetherNetworkGuid());
+
+  StartRestoration();
+  VerifyDisconnectedAfterRestoration();
+}
+
+TEST_F(CrashRecoveryManagerTest, ActiveHostConnected_EntryNotInCache) {
+  SetConnected();
+
+  StartRestoration();
+  VerifyDisconnectedAfterRestoration();
+}
+
+TEST_F(CrashRecoveryManagerTest, ActiveHostConnected_WifiNetworkMissing) {
+  AddScanEntry();
+  SetConnected();
+
+  StartRestoration();
+  VerifyDisconnectedAfterRestoration();
+}
+
+TEST_F(CrashRecoveryManagerTest, ActiveHostConnected_WifiNetworkDisconnected) {
+  AddScanEntry();
+  AddWifiNetwork(false /* is_connected */);
+  SetConnected();
+
+  StartRestoration();
+  VerifyDisconnectedAfterRestoration();
+}
+
+TEST_F(CrashRecoveryManagerTest, ActiveHostConnected_RestoreSuccessful) {
+  AddScanEntry();
+  AddWifiNetwork(true /* is_connected */);
+  SetConnected();
+
+  StartRestoration();
+  EXPECT_TRUE(is_restoration_finished_);
+  EXPECT_EQ(ActiveHost::ActiveHostStatus::CONNECTED,
+            fake_active_host_->GetActiveHostStatus());
+}
+
+}  // namespace tether
+
+}  // namespace chromeos

chromeos/components/tether/initializer.cc

@@ -8,6 +8,7 @@
 #include "chromeos/components/tether/active_host.h"
 #include "chromeos/components/tether/active_host_network_state_updater.h"
 #include "chromeos/components/tether/ble_connection_manager.h"
+#include "chromeos/components/tether/crash_recovery_manager.h"
 #include "chromeos/components/tether/device_id_tether_network_guid_map.h"
 #include "chromeos/components/tether/host_connection_metrics_logger.h"
 #include "chromeos/components/tether/host_scan_device_prioritizer_impl.h"
@@ -248,8 +249,18 @@ void Initializer::OnBluetoothAdapterAdvertisingIntervalSet(
           network_connection_handler_, tether_connector_.get(),
           tether_disconnector_.get());
 
-  // Because Initializer is created on each user log in, it's appropriate to
-  // call this method now.
+  crash_recovery_manager_ = base::MakeUnique<CrashRecoveryManager>(
+      network_state_handler_, active_host_.get(),
+      master_host_scan_cache_.get());
+  crash_recovery_manager_->RestorePreCrashStateIfNecessary(base::Bind(
+      &Initializer::OnPreCrashStateRestored, weak_ptr_factory_.GetWeakPtr()));
+}
+
+void Initializer::OnPreCrashStateRestored() {
+  // |crash_recovery_manager_| is no longer needed since it has completed.
+  crash_recovery_manager_.reset();
+
+  // Start a scan now that the Tether module has started up.
   host_scan_scheduler_->UserLoggedIn();
 }
 

chromeos/components/tether/initializer.h

@@ -37,6 +37,7 @@ namespace tether {
 class ActiveHost;
 class ActiveHostNetworkStateUpdater;
 class BleConnectionManager;
+class CrashRecoveryManager;
 class NetworkConnectionHandlerTetherDelegate;
 class DeviceIdTetherNetworkGuidMap;
 class HostScanner;
@@ -103,6 +104,7 @@ class Initializer : public OAuth2TokenService::Observer {
       scoped_refptr<device::BluetoothAdapter> adapter);
   void OnBluetoothAdapterAdvertisingIntervalError(
       device::BluetoothAdvertisement::ErrorCode status);
+  void OnPreCrashStateRestored();
 
   cryptauth::CryptAuthService* cryptauth_service_;
   std::unique_ptr<NotificationPresenter> notification_presenter_;
@@ -145,6 +147,7 @@ class Initializer : public OAuth2TokenService::Observer {
       network_connection_handler_tether_delegate_;
   std::unique_ptr<TetherNetworkDisconnectionHandler>
       tether_network_disconnection_handler_;
+  std::unique_ptr<CrashRecoveryManager> crash_recovery_manager_;
 
   base::WeakPtrFactory<Initializer> weak_ptr_factory_;