Do not start a scan when the Default network changes unless both the Default...

Do not start a scan when the Default network changes unless both the Default network and the Tether network are disconnected.

This is done to prevent a scan from starting when Ethernet and Tether are both connected, and then Ethernet is disconnected (but Tether remains connected).

This CL also ensures a scan on login, regardless of whether or not the Default network is connected.

Bug: 672263, 738542
Change-Id: I062fdc3dc825df6a6e986db80964fa92732774f7
Reviewed-on: https://chromium-review.googlesource.com/598940
Commit-Queue: Leslie Watkins <lesliewatkins@chromium.org>
Reviewed-by: Kyle Horimoto <khorimoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#491834}

chromeos/components/tether/host_scan_scheduler.cc

@@ -33,14 +33,15 @@ HostScanScheduler::~HostScanScheduler() {
   host_scanner_->RemoveObserver(this);
 }
 
-void HostScanScheduler::UserLoggedIn() {
-  if (!IsNetworkConnectingOrConnected(network_state_handler_->DefaultNetwork()))
+void HostScanScheduler::ScheduleScan() {
   EnsureScan();
 }
 
 void HostScanScheduler::DefaultNetworkChanged(const NetworkState* network) {
-  if (!IsNetworkConnectingOrConnected(network))
+  if (!IsNetworkConnectingOrConnected(network) &&
+      !IsTetherNetworkConnectingOrConnected()) {
     EnsureScan();
+  }
 }
 
 void HostScanScheduler::ScanRequested() {
@@ -65,6 +66,13 @@ bool HostScanScheduler::IsNetworkConnectingOrConnected(
          (network->IsConnectingState() || network->IsConnectedState());
 }
 
+bool HostScanScheduler::IsTetherNetworkConnectingOrConnected() {
+  return network_state_handler_->ConnectingNetworkByType(
+             NetworkTypePattern::Tether()) ||
+         network_state_handler_->ConnectedNetworkByType(
+             NetworkTypePattern::Tether());
+}
+
 }  // namespace tether
 
 }  // namespace chromeos

chromeos/components/tether/host_scan_scheduler.h

@@ -29,7 +29,7 @@ class HostScanScheduler : public NetworkStateHandlerObserver,
                     HostScanner* host_scanner);
   ~HostScanScheduler() override;
 
-  void UserLoggedIn();
+  void ScheduleScan();
 
   // NetworkStateHandlerObserver:
   void DefaultNetworkChanged(const NetworkState* network) override;
@@ -43,6 +43,7 @@ class HostScanScheduler : public NetworkStateHandlerObserver,
 
   void EnsureScan();
   bool IsNetworkConnectingOrConnected(const NetworkState* network);
+  bool IsTetherNetworkConnectingOrConnected();
 
   NetworkStateHandler* network_state_handler_;
   HostScanner* host_scanner_;

chromeos/components/tether/host_scan_scheduler_unittest.cc

@@ -46,7 +46,10 @@ class FakeHostScanner : public HostScanner {
     num_scans_started_++;
   }
 
-  void StopScan() { is_scan_active_ = false; }
+  void StopScan() {
+    is_scan_active_ = false;
+    NotifyScanFinished();
+  }
 
   bool IsScanActive() override { return is_scan_active_; }
 
@@ -57,13 +60,16 @@ class FakeHostScanner : public HostScanner {
   int num_scans_started_ = 0;
 };
 
+const char kEthernetServiceGuid[] = "ethernetServiceGuid";
 const char kWifiServiceGuid[] = "wifiServiceGuid";
+const char kTetherGuid[] = "tetherGuid";
 
-std::string CreateConfigurationJsonString() {
+std::string CreateConfigurationJsonString(const std::string& guid,
+                                          const std::string& type) {
   std::stringstream ss;
   ss << "{"
-     << "  \"GUID\": \"" << kWifiServiceGuid << "\","
-     << "  \"Type\": \"" << shill::kTypeWifi << "\","
+     << "  \"GUID\": \"" << guid << "\","
+     << "  \"Type\": \"" << type << "\","
      << "  \"State\": \"" << shill::kStateReady << "\""
      << "}";
   return ss.str();
@@ -77,9 +83,10 @@ class HostScanSchedulerTest : public NetworkStateTest {
     DBusThreadManager::Initialize();
     NetworkStateTest::SetUp();
 
-    wifi_service_path_ = ConfigureService(CreateConfigurationJsonString());
-    test_manager_client()->SetManagerProperty(shill::kDefaultServiceProperty,
-                                              base::Value(wifi_service_path_));
+    ethernet_service_path_ = ConfigureService(CreateConfigurationJsonString(
+        kEthernetServiceGuid, shill::kTypeEthernet));
+    test_manager_client()->SetManagerProperty(
+        shill::kDefaultServiceProperty, base::Value(ethernet_service_path_));
 
     network_state_handler()->SetTetherTechnologyState(
         NetworkStateHandler::TECHNOLOGY_ENABLED);
@@ -98,42 +105,65 @@ class HostScanSchedulerTest : public NetworkStateTest {
     DBusThreadManager::Shutdown();
   }
 
-  void SetDefaultNetworkDisconnected() {
-    SetServiceProperty(wifi_service_path_, std::string(shill::kStateProperty),
+  // Disconnects the Ethernet network and manually sets the default network to
+  // |new_default_service_path|. If |new_default_service_path| is empty then no
+  // default network is set.
+  void SetEthernetNetworkDisconnected(
+      const std::string& new_default_service_path) {
+    SetServiceProperty(ethernet_service_path_,
+                       std::string(shill::kStateProperty),
                        base::Value(shill::kStateIdle));
+    if (new_default_service_path.empty())
+      return;
+
+    test_manager_client()->SetManagerProperty(
+        shill::kDefaultServiceProperty, base::Value(new_default_service_path));
   }
 
-  void SetDefaultNetworkConnecting() {
-    SetServiceProperty(wifi_service_path_, std::string(shill::kStateProperty),
+  void SetEthernetNetworkConnecting() {
+    SetServiceProperty(ethernet_service_path_,
+                       std::string(shill::kStateProperty),
                        base::Value(shill::kStateAssociation));
+    test_manager_client()->SetManagerProperty(
+        shill::kDefaultServiceProperty, base::Value(ethernet_service_path_));
   }
 
-  void SetDefaultNetworkConnected() {
-    SetServiceProperty(wifi_service_path_, std::string(shill::kStateProperty),
+  void SetEthernetNetworkConnected() {
+    SetServiceProperty(ethernet_service_path_,
+                       std::string(shill::kStateProperty),
                        base::Value(shill::kStateReady));
+    test_manager_client()->SetManagerProperty(
+        shill::kDefaultServiceProperty, base::Value(ethernet_service_path_));
+  }
+
+  // Adds a Tether network state, adds a Wifi network to be used as the Wifi
+  // hotspot, and associates the two networks. Returns the service path of the
+  // Wifi network.
+  std::string AddTetherNetworkState() {
+    network_state_handler()->AddTetherNetworkState(
+        kTetherGuid, "name", "carrier", 100 /* battery_percentage */,
+        100 /* signal strength */, false /* has_connected_to_host */);
+    std::string wifi_service_path = ConfigureService(
+        CreateConfigurationJsonString(kWifiServiceGuid, shill::kTypeWifi));
+    network_state_handler()->AssociateTetherNetworkStateWithWifiNetwork(
+        kTetherGuid, kWifiServiceGuid);
+    return wifi_service_path;
   }
 
   base::test::ScopedTaskEnvironment scoped_task_environment_;
-  std::string wifi_service_path_;
+  std::string ethernet_service_path_;
 
   std::unique_ptr<FakeHostScanner> fake_host_scanner_;
 
   std::unique_ptr<HostScanScheduler> host_scan_scheduler_;
 };
 
-TEST_F(HostScanSchedulerTest, UserLoggedIn) {
-  EXPECT_EQ(0, fake_host_scanner_->num_scans_started());
-  EXPECT_FALSE(
-      network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
-
-  host_scan_scheduler_->UserLoggedIn();
-
+TEST_F(HostScanSchedulerTest, ScheduleScan) {
   EXPECT_EQ(0, fake_host_scanner_->num_scans_started());
   EXPECT_FALSE(
       network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
 
-  SetDefaultNetworkDisconnected();
-  host_scan_scheduler_->UserLoggedIn();
+  host_scan_scheduler_->ScheduleScan();
 
   EXPECT_EQ(1, fake_host_scanner_->num_scans_started());
   EXPECT_TRUE(
@@ -158,7 +188,6 @@ TEST_F(HostScanSchedulerTest, ScanRequested) {
       network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
 
   fake_host_scanner_->StopScan();
-  host_scan_scheduler_->ScanFinished();
   EXPECT_EQ(1, fake_host_scanner_->num_scans_started());
   EXPECT_FALSE(
       network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
@@ -171,33 +200,102 @@ TEST_F(HostScanSchedulerTest, ScanRequested) {
 }
 
 TEST_F(HostScanSchedulerTest, DefaultNetworkChanged) {
+  // When no Tether network is present, a scan should start when the default
+  // network is disconnected.
   EXPECT_EQ(0, fake_host_scanner_->num_scans_started());
   EXPECT_FALSE(
       network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
 
-  SetDefaultNetworkConnecting();
-
+  SetEthernetNetworkConnecting();
   EXPECT_EQ(0, fake_host_scanner_->num_scans_started());
   EXPECT_FALSE(
       network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
 
-  SetDefaultNetworkDisconnected();
+  SetEthernetNetworkConnected();
+  EXPECT_EQ(0, fake_host_scanner_->num_scans_started());
+  EXPECT_FALSE(
+      network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
 
+  SetEthernetNetworkDisconnected(std::string() /* default_service_path */);
   EXPECT_EQ(1, fake_host_scanner_->num_scans_started());
   EXPECT_TRUE(
       network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
 
-  SetDefaultNetworkConnecting();
+  fake_host_scanner_->StopScan();
+
+  // When Tether is present but disconnected, a scan should start when the
+  // default network is disconnected.
+  std::string tether_service_path = AddTetherNetworkState();
 
   EXPECT_EQ(1, fake_host_scanner_->num_scans_started());
-  EXPECT_TRUE(
+  EXPECT_FALSE(
       network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
 
-  SetDefaultNetworkDisconnected();
+  SetEthernetNetworkConnecting();
+  EXPECT_EQ(1, fake_host_scanner_->num_scans_started());
+  EXPECT_FALSE(
+      network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
 
+  SetEthernetNetworkConnected();
   EXPECT_EQ(1, fake_host_scanner_->num_scans_started());
+  EXPECT_FALSE(
+      network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
+
+  SetEthernetNetworkDisconnected(std::string() /* default_service_path */);
+  EXPECT_EQ(2, fake_host_scanner_->num_scans_started());
   EXPECT_TRUE(
       network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
+
+  fake_host_scanner_->StopScan();
+
+  // When Tether is present and connecting, no scan should start when an
+  // Ethernet network becomes the default network and then disconnects.
+  network_state_handler()->SetTetherNetworkStateConnecting(kTetherGuid);
+
+  EXPECT_EQ(2, fake_host_scanner_->num_scans_started());
+  EXPECT_FALSE(
+      network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
+
+  SetEthernetNetworkConnecting();
+  EXPECT_EQ(2, fake_host_scanner_->num_scans_started());
+  EXPECT_FALSE(
+      network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
+
+  SetEthernetNetworkConnected();
+  EXPECT_EQ(2, fake_host_scanner_->num_scans_started());
+  EXPECT_FALSE(
+      network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
+
+  SetEthernetNetworkDisconnected(tether_service_path);
+  EXPECT_EQ(2, fake_host_scanner_->num_scans_started());
+  EXPECT_FALSE(
+      network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
+
+  fake_host_scanner_->StopScan();
+
+  // When Tether is present and connected, no scan should start when an Ethernet
+  // network becomes the default network and then disconnects.
+  base::RunLoop().RunUntilIdle();
+  network_state_handler()->SetTetherNetworkStateConnected(kTetherGuid);
+
+  EXPECT_EQ(2, fake_host_scanner_->num_scans_started());
+  EXPECT_FALSE(
+      network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
+
+  SetEthernetNetworkConnecting();
+  EXPECT_EQ(2, fake_host_scanner_->num_scans_started());
+  EXPECT_FALSE(
+      network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
+
+  SetEthernetNetworkConnected();
+  EXPECT_EQ(2, fake_host_scanner_->num_scans_started());
+  EXPECT_FALSE(
+      network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
+
+  SetEthernetNetworkDisconnected(tether_service_path);
+  EXPECT_EQ(2, fake_host_scanner_->num_scans_started());
+  EXPECT_FALSE(
+      network_state_handler()->GetScanningByType(NetworkTypePattern::Tether()));
 }
 
 }  // namespace tether

chromeos/components/tether/host_scanner.h

@@ -60,6 +60,8 @@ class HostScanner : public HostScannerOperation::Observer {
   // function is a no-op.
   virtual void StartScan();
 
+  void NotifyScanFinished();
+
   // HostScannerOperation::Observer:
   void OnTetherAvailabilityResponse(
       std::vector<HostScannerOperation::ScannedDeviceInfo>&
@@ -81,8 +83,6 @@ class HostScanner : public HostScannerOperation::Observer {
     HOST_SCAN_RESULT_MAX
   };
 
-  void NotifyScanFinished();
-
   void OnTetherHostsFetched(const cryptauth::RemoteDeviceList& tether_hosts);
   void SetCacheEntry(
       const HostScannerOperation::ScannedDeviceInfo& scanned_device_info);

chromeos/components/tether/initializer.cc

@@ -266,7 +266,7 @@ void Initializer::OnPreCrashStateRestored() {
   crash_recovery_manager_.reset();
 
   // Start a scan now that the Tether module has started up.
-  host_scan_scheduler_->UserLoggedIn();
+  host_scan_scheduler_->ScheduleScan();
 }
 
 void Initializer::OnBluetoothAdapterAdvertisingIntervalError(