Fix behavior of bootstrap manager after monitoring timeout was reachedbrillo-m7-releasebrillo-m7-dev

StartMonitoring resets monitor_until_ member always.
ContinueMonitoring resets monitor_until_ only for online state as we
don't have timeout in this state.

Removed check for last_configured_ssid.empty() from
UpdateConnectionState because we should shutdown bootstrapping if device
is connected using other interface.

BUG:25463084

Change-Id: I0afd943f4a3ca797b65a51236103ea3d345828d2
Reviewed-on: https://weave-review.googlesource.com/1473
Reviewed-by: Paul Westbrook <pwestbro@google.com>
(cherry picked from commit 65e1f21f13102b076156e2e6a07486c06f5b25d1)

3 files changed, 95 insertions, 19 deletions

diff --git a/src/privet/wifi_bootstrap_manager.cc b/src/privet/wifi_bootstrap_manager.cc
index 4f46ea4..292622d 100644
--- a/src/privet/wifi_bootstrap_manager.cc
+++ b/src/privet/wifi_bootstrap_manager.cc
@@ -20,7 +20,10 @@ namespace privet {
 
 namespace {
 
+const int kMonitoringWithSsidTimeoutSeconds = 15;
 const int kMonitoringTimeoutSeconds = 120;
+const int kBootstrapTimeoutSeconds = 600;
+const int kConnectingTimeoutSeconds = 180;
 
 const EnumToStringMap<WifiBootstrapManager::State>::Map kWifiSetupStateMap[] = {
     {WifiBootstrapManager::State::kDisabled, "disabled"},
@@ -55,7 +58,8 @@ void WifiBootstrapManager::Init() {
                  lifetime_weak_factory_.GetWeakPtr()));
   if (config_->GetSettings().last_configured_ssid.empty()) {
     // Give implementation some time to figure out state.
-    StartMonitoring(base::TimeDelta::FromSeconds(15));
+    StartMonitoring(
+        base::TimeDelta::FromSeconds(kMonitoringWithSsidTimeoutSeconds));
   } else {
     StartMonitoring(base::TimeDelta::FromSeconds(kMonitoringTimeoutSeconds));
   }
@@ -80,28 +84,30 @@ void WifiBootstrapManager::StartBootstrapping() {
     task_runner_->PostDelayedTask(
         FROM_HERE, base::Bind(&WifiBootstrapManager::OnBootstrapTimeout,
                               tasks_weak_factory_.GetWeakPtr()),
-        base::TimeDelta::FromMinutes(10));
+        base::TimeDelta::FromSeconds(kBootstrapTimeoutSeconds));
   }
   // TODO(vitalybuka): Add SSID probing.
   privet_ssid_ = GenerateSsid();
   CHECK(!privet_ssid_.empty());
+
+  VLOG(1) << "Starting AP with SSID: " << privet_ssid_;
   wifi_->StartAccessPoint(privet_ssid_);
 }
 
 void WifiBootstrapManager::EndBootstrapping() {
+  VLOG(1) << "Stopping AP";
   wifi_->StopAccessPoint();
   privet_ssid_.clear();
 }
 
 void WifiBootstrapManager::StartConnecting(const std::string& ssid,
                                            const std::string& passphrase) {
-  VLOG(1) << "WiFi is attempting to connect. (ssid=" << ssid
-          << ", pass=" << passphrase << ").";
+  VLOG(1) << "Attempting connect to SSID:" << ssid;
   UpdateState(State::kConnecting);
   task_runner_->PostDelayedTask(
       FROM_HERE, base::Bind(&WifiBootstrapManager::OnConnectTimeout,
                             tasks_weak_factory_.GetWeakPtr()),
-      base::TimeDelta::FromMinutes(3));
+      base::TimeDelta::FromSeconds(kConnectingTimeoutSeconds));
   wifi_->Connect(ssid, passphrase,
                  base::Bind(&WifiBootstrapManager::OnConnectDone,
                             tasks_weak_factory_.GetWeakPtr(), ssid));
@@ -110,6 +116,11 @@ void WifiBootstrapManager::StartConnecting(const std::string& ssid,
 void WifiBootstrapManager::EndConnecting() {}
 
 void WifiBootstrapManager::StartMonitoring(const base::TimeDelta& timeout) {
+  monitor_until_ = {};
+  ContinueMonitoring(timeout);
+}
+
+void WifiBootstrapManager::ContinueMonitoring(const base::TimeDelta& timeout) {
   VLOG(1) << "Monitoring connectivity.";
   // We already have a callback in place with |network_| to update our
   // connectivity state.  See OnConnectivityChange().
@@ -134,8 +145,8 @@ void WifiBootstrapManager::StartMonitoring(const base::TimeDelta& timeout) {
 void WifiBootstrapManager::EndMonitoring() {}
 
 void WifiBootstrapManager::UpdateState(State new_state) {
-  VLOG(3) << "Switching state from " << static_cast<int>(state_) << " to "
-          << static_cast<int>(new_state);
+  VLOG(3) << "Switching state from " << EnumToString(state_) << " to "
+          << EnumToString(new_state);
   // Abort irrelevant tasks.
   tasks_weak_factory_.InvalidateWeakPtrs();
 
@@ -227,29 +238,26 @@ void WifiBootstrapManager::OnBootstrapTimeout() {
 }
 
 void WifiBootstrapManager::OnConnectivityChange() {
-  VLOG(3) << "ConnectivityChanged: "
-          << EnumToString(network_->GetConnectionState());
   UpdateConnectionState();
 
-  if (state_ == State::kMonitoring ||  // Reset monitoring timeout.
+  if (state_ == State::kMonitoring ||
       (state_ != State::kDisabled &&
        network_->GetConnectionState() == Network::State::kOnline)) {
-    StartMonitoring(base::TimeDelta::FromSeconds(kMonitoringTimeoutSeconds));
+    ContinueMonitoring(base::TimeDelta::FromSeconds(kMonitoringTimeoutSeconds));
   }
 }
 
 void WifiBootstrapManager::OnMonitorTimeout() {
-  VLOG(1) << "Spent too long offline.  Entering bootstrap mode.";
+  VLOG(1) << "Spent too long offline. Entering bootstrap mode.";
   // TODO(wiley) Retrieve relevant errors from shill.
   StartBootstrapping();
 }
 
 void WifiBootstrapManager::UpdateConnectionState() {
   connection_state_ = ConnectionState{ConnectionState::kUnconfigured};
-  if (config_->GetSettings().last_configured_ssid.empty())
-    return;
 
   Network::State service_state{network_->GetConnectionState()};
+  VLOG(3) << "New network state: " << EnumToString(service_state);
   switch (service_state) {
     case Network::State::kOffline:
       connection_state_ = ConnectionState{ConnectionState::kOffline};
diff --git a/src/privet/wifi_bootstrap_manager.h b/src/privet/wifi_bootstrap_manager.h
index 71dbb49..62a77c2 100644
--- a/src/privet/wifi_bootstrap_manager.h
+++ b/src/privet/wifi_bootstrap_manager.h
@@ -74,6 +74,7 @@ class WifiBootstrapManager : public WifiDelegate {
   void EndConnecting();
 
   void StartMonitoring(const base::TimeDelta& timeout);
+  void ContinueMonitoring(const base::TimeDelta& timeout);
   void EndMonitoring();
 
   // Update the current state, post tasks to notify listeners accordingly to
diff --git a/src/weave_unittest.cc b/src/weave_unittest.cc
index 6ff3f6d..fb00cc9 100644
--- a/src/weave_unittest.cc
+++ b/src/weave_unittest.cc
@@ -21,13 +21,14 @@
 #include "src/bind_lambda.h"
 
 using testing::_;
+using testing::AtLeast;
 using testing::AtMost;
 using testing::HasSubstr;
+using testing::InSequence;
 using testing::Invoke;
 using testing::InvokeWithoutArgs;
 using testing::MatchesRegex;
 using testing::Mock;
-using testing::AtLeast;
 using testing::Return;
 using testing::ReturnRefOfCopy;
 using testing::StartsWith;
@@ -266,10 +267,13 @@ class WeaveTest : public ::testing::Test {
 
   void NotifyNetworkChanged(provider::Network::State state,
                             base::TimeDelta delay) {
-    EXPECT_CALL(network_, GetConnectionState()).WillRepeatedly(Return(state));
-    for (const auto& cb : network_callbacks_) {
-      task_runner_.PostDelayedTask(FROM_HERE, cb, delay);
-    }
+    auto task = [this, state] {
+      EXPECT_CALL(network_, GetConnectionState()).WillRepeatedly(Return(state));
+      for (const auto& cb : network_callbacks_)
+        cb.Run();
+    };
+
+    task_runner_.PostDelayedTask(FROM_HERE, base::Bind(task), delay);
   }
 
   std::map<std::string, provider::HttpServer::RequestHandlerCallback>
@@ -460,4 +464,67 @@ TEST_F(WeaveWiFiSetupTest, StartOfflineWithSsid) {
   StartDevice();
 }
 
+TEST_F(WeaveWiFiSetupTest, OfflineLongTimeWithNoSsid) {
+  EXPECT_CALL(network_, GetConnectionState())
+      .WillRepeatedly(Return(Network::State::kOffline));
+  NotifyNetworkChanged(provider::Network::State::kOnline,
+                       base::TimeDelta::FromHours(15));
+
+  {
+    InSequence s;
+    auto time_stamp = task_runner_.GetClock()->Now();
+
+    EXPECT_CALL(wifi_, StartAccessPoint(MatchesRegex("TEST_NAME.*prv")))
+        .WillOnce(InvokeWithoutArgs([this, &time_stamp]() {
+          EXPECT_LE(task_runner_.GetClock()->Now() - time_stamp,
+                    base::TimeDelta::FromMinutes(1));
+          time_stamp = task_runner_.GetClock()->Now();
+        }));
+
+    EXPECT_CALL(wifi_, StopAccessPoint())
+        .WillOnce(InvokeWithoutArgs([this, &time_stamp]() {
+          EXPECT_GT(task_runner_.GetClock()->Now() - time_stamp,
+                    base::TimeDelta::FromMinutes(5));
+          time_stamp = task_runner_.GetClock()->Now();
+          task_runner_.Break();
+        }));
+  }
+
+  StartDevice();
+}
+
+TEST_F(WeaveWiFiSetupTest, OfflineLongTimeWithSsid) {
+  EXPECT_CALL(config_store_, LoadSettings())
+      .WillRepeatedly(Return(R"({"last_configured_ssid": "TEST_ssid"})"));
+  EXPECT_CALL(network_, GetConnectionState())
+      .WillRepeatedly(Return(Network::State::kOffline));
+  NotifyNetworkChanged(provider::Network::State::kOnline,
+                       base::TimeDelta::FromHours(15));
+
+  {
+    InSequence s;
+    auto time_stamp = task_runner_.GetClock()->Now();
+    for (size_t i = 0; i < 10; ++i) {
+      EXPECT_CALL(wifi_, StartAccessPoint(MatchesRegex("TEST_NAME.*prv")))
+          .WillOnce(InvokeWithoutArgs([this, &time_stamp]() {
+            EXPECT_GT(task_runner_.GetClock()->Now() - time_stamp,
+                      base::TimeDelta::FromMinutes(1));
+            time_stamp = task_runner_.GetClock()->Now();
+          }));
+
+      EXPECT_CALL(wifi_, StopAccessPoint())
+          .WillOnce(InvokeWithoutArgs([this, &time_stamp]() {
+            EXPECT_GT(task_runner_.GetClock()->Now() - time_stamp,
+                      base::TimeDelta::FromMinutes(5));
+            time_stamp = task_runner_.GetClock()->Now();
+          }));
+    }
+
+    EXPECT_CALL(wifi_, StartAccessPoint(MatchesRegex("TEST_NAME.*prv")))
+        .WillOnce(InvokeWithoutArgs([this]() { task_runner_.Break(); }));
+  }
+
+  StartDevice();
+}
+
 }  // namespace weave