Revert "Move WifiPollingPolicy to a global instance"

This reverts commit 29212f31.

Reason for revert: Crashing on Canary builds, see crbug.com/798987

Original change's description:
> Move WifiPollingPolicy to a global instance
> 
> The WifiPollingPolicy controls how frequently the network location
> provider will initiate Wi-Fi scans. Previously, the policy was attached
> to the WifiDataProvider, which is destroyed when there are no active
> geolocation API calls. Destruction of the provider causes us to lose track
> of our polling policy.
> 
> The policy allows the first scan to be performed immediately and schedules
> subsequent scans at regular intervals. When the provider is destroyed and
> recreated rapidly it considers each scan to be "first", which allows many
> Wi-Fi scans to be performed in a short window.
> 
> This change moves the policy state to a global instance so it can be saved
> when the provider is destroyed and recreated. The new policy allows the
> first-ever scan to be performed immediately, and subsequent scans may also
> be performed immediately if enough time has lapsed since the previous scan.
> In all other situations, the configured polling interval is enforced.
> 
> BUG=764954
> 
> Change-Id: I00649dde2a707127e53e0330511172fef4a43471
> Reviewed-on: https://chromium-review.googlesource.com/834988
> Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
> Reviewed-by: Reilly Grant <reillyg@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#526782}

TBR=reillyg@chromium.org,mattreynolds@chromium.org

Change-Id: I25d5596418df55e9de9e0f09e6ba7546b54551a4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 764954
Reviewed-on: https://chromium-review.googlesource.com/851132Reviewed-by: Matt Reynolds <mattreynolds@chromium.org>
Commit-Queue: Matt Reynolds <mattreynolds@chromium.org>
Cr-Commit-Position: refs/heads/master@{#527058}

device/geolocation/BUILD.gn

@@ -53,7 +53,6 @@ component("geolocation") {
     "wifi_data_provider_manager.h",
     "wifi_data_provider_win.cc",
     "wifi_data_provider_win.h",
-    "wifi_polling_policy.cc",
     "wifi_polling_policy.h",
   ]
 

device/geolocation/location_arbitrator.cc

@@ -14,7 +14,6 @@
 #include "build/build_config.h"
 #include "device/geolocation/network_location_provider.h"
 #include "device/geolocation/public/cpp/geoposition.h"
-#include "device/geolocation/wifi_polling_policy.h"
 
 namespace device {
 
@@ -34,12 +33,7 @@ LocationArbitrator::LocationArbitrator(
       is_permission_granted_(false),
       is_running_(false) {}
 
-LocationArbitrator::~LocationArbitrator() {
-  // Destroy the global WifiPollingPolicy. The policy is created and used by the
-  // network location provider but should be retained across network provider
-  // restarts to ensure the time of the most recent WiFi scan is not lost.
-  WifiPollingPolicy::Shutdown();
-}
+LocationArbitrator::~LocationArbitrator() = default;
 
 bool LocationArbitrator::HasPermissionBeenGrantedForTest() const {
   return is_permission_granted_;

device/geolocation/wifi_data_provider_chromeos.cc

@@ -36,16 +36,20 @@ WifiDataProviderChromeOs::~WifiDataProviderChromeOs() = default;
 void WifiDataProviderChromeOs::StartDataProvider() {
   DCHECK(CalledOnClientThread());
 
-  if (!WifiPollingPolicy::IsInitialized())
-    WifiPollingPolicy::Initialize(CreatePollingPolicy());
-  DCHECK(WifiPollingPolicy::IsInitialized());
+  DCHECK(polling_policy_ == nullptr);
+  polling_policy_.reset(
+      new GenericWifiPollingPolicy<kDefaultPollingIntervalMilliseconds,
+                                   kNoChangePollingIntervalMilliseconds,
+                                   kTwoNoChangePollingIntervalMilliseconds,
+                                   kNoWifiPollingIntervalMilliseconds>);
 
-  ScheduleStart(WifiPollingPolicy::Get()->PollingInterval());
+  ScheduleStart();
 }
 
 void WifiDataProviderChromeOs::StopDataProvider() {
   DCHECK(CalledOnClientThread());
 
+  polling_policy_.reset();
   ScheduleStop();
 }
 
@@ -79,7 +83,7 @@ void WifiDataProviderChromeOs::DidWifiScanTaskNoResults() {
   // Schedule next scan if started (StopDataProvider could have been called
   // in between DoWifiScanTaskOnNetworkHandlerThread and this method).
   if (started_)
-    ScheduleNextScan(WifiPollingPolicy::Get()->NoWifiInterval());
+    ScheduleNextScan(polling_policy_->NoWifiInterval());
 }
 
 void WifiDataProviderChromeOs::DidWifiScanTask(const WifiData& new_data) {
@@ -89,8 +93,8 @@ void WifiDataProviderChromeOs::DidWifiScanTask(const WifiData& new_data) {
   // Schedule next scan if started (StopDataProvider could have been called
   // in between DoWifiScanTaskOnNetworkHandlerThread and this method).
   if (started_) {
-    WifiPollingPolicy::Get()->UpdatePollingInterval(update_available);
-    ScheduleNextScan(WifiPollingPolicy::Get()->PollingInterval());
+    polling_policy_->UpdatePollingInterval(update_available);
+    ScheduleNextScan(polling_policy_->PollingInterval());
   }
 
   if (update_available || !is_first_scan_complete_) {
@@ -120,7 +124,7 @@ void WifiDataProviderChromeOs::ScheduleStop() {
   started_ = false;
 }
 
-void WifiDataProviderChromeOs::ScheduleStart(int interval) {
+void WifiDataProviderChromeOs::ScheduleStart() {
   DCHECK(CalledOnClientThread());
   DCHECK(!started_);
   if (!NetworkHandler::IsInitialized()) {
@@ -128,12 +132,13 @@ void WifiDataProviderChromeOs::ScheduleStart(int interval) {
     return;
   }
   started_ = true;
-  NetworkHandler::Get()->task_runner()->PostDelayedTask(
+  // Perform first scan ASAP regardless of the polling policy. If this scan
+  // fails we'll retry at a rate in line with the polling policy.
+  NetworkHandler::Get()->task_runner()->PostTask(
       FROM_HERE,
       base::Bind(
           &WifiDataProviderChromeOs::DoWifiScanTaskOnNetworkHandlerThread,
-          this),
-      base::TimeDelta::FromMilliseconds(interval));
+          this));
 }
 
 bool WifiDataProviderChromeOs::GetAccessPointData(
@@ -171,14 +176,6 @@ bool WifiDataProviderChromeOs::GetAccessPointData(
   return true;
 }
 
-std::unique_ptr<WifiPollingPolicy>
-WifiDataProviderChromeOs::CreatePollingPolicy() {
-  return std::make_unique<GenericWifiPollingPolicy<
-      kDefaultPollingIntervalMilliseconds, kNoChangePollingIntervalMilliseconds,
-      kTwoNoChangePollingIntervalMilliseconds,
-      kNoWifiPollingIntervalMilliseconds>>();
-}
-
 // static
 WifiDataProvider* WifiDataProviderManager::DefaultFactoryFunction() {
   return new WifiDataProviderChromeOs();

device/geolocation/wifi_data_provider_chromeos.h

@@ -39,7 +39,7 @@ class DEVICE_GEOLOCATION_EXPORT WifiDataProviderChromeOs
   void ScheduleNextScan(int interval);
 
   // Will schedule starting of the scanning process.
-  void ScheduleStart(int interval);
+  void ScheduleStart();
 
   // Will schedule stopping of the scanning process.
   void ScheduleStop();
@@ -47,8 +47,8 @@ class DEVICE_GEOLOCATION_EXPORT WifiDataProviderChromeOs
   // Get access point data from chromeos.
   bool GetAccessPointData(WifiData::AccessPointDataSet* data);
 
-  // Create the policy for controlling the polling update interval.
-  std::unique_ptr<WifiPollingPolicy> CreatePollingPolicy();
+  // Controls the polling update interval. (client thread)
+  std::unique_ptr<WifiPollingPolicy> polling_policy_;
 
   // The latest wifi data. (client thread)
   WifiData wifi_data_;

device/geolocation/wifi_data_provider_common.cc

@@ -35,15 +35,18 @@ void WifiDataProviderCommon::StartDataProvider() {
     return;
   }
 
-  if (!WifiPollingPolicy::IsInitialized())
-    WifiPollingPolicy::Initialize(CreatePollingPolicy());
-  DCHECK(WifiPollingPolicy::IsInitialized());
+  DCHECK(!polling_policy_);
+  polling_policy_ = CreatePollingPolicy();
+  DCHECK(polling_policy_);
 
-  ScheduleNextScan(WifiPollingPolicy::Get()->PollingInterval());
+  // Perform first scan ASAP regardless of the polling policy. If this scan
+  // fails we'll retry at a rate in line with the polling policy.
+  ScheduleNextScan(0);
 }
 
 void WifiDataProviderCommon::StopDataProvider() {
   wlan_api_.reset();
+  polling_policy_.reset();
 }
 
 bool WifiDataProviderCommon::GetData(WifiData* data) {
@@ -57,12 +60,12 @@ void WifiDataProviderCommon::DoWifiScanTask() {
   bool update_available = false;
   WifiData new_data;
   if (!wlan_api_->GetAccessPointData(&new_data.access_point_data)) {
-    ScheduleNextScan(WifiPollingPolicy::Get()->NoWifiInterval());
+    ScheduleNextScan(polling_policy_->NoWifiInterval());
   } else {
     update_available = wifi_data_.DiffersSignificantly(new_data);
     wifi_data_ = new_data;
-    WifiPollingPolicy::Get()->UpdatePollingInterval(update_available);
-    ScheduleNextScan(WifiPollingPolicy::Get()->PollingInterval());
+    polling_policy_->UpdatePollingInterval(update_available);
+    ScheduleNextScan(polling_policy_->PollingInterval());
   }
   if (update_available || !is_first_scan_complete_) {
     is_first_scan_complete_ = true;

device/geolocation/wifi_data_provider_common.h

@@ -72,6 +72,9 @@ class DEVICE_GEOLOCATION_EXPORT WifiDataProviderCommon
   // Underlying OS wifi API.
   std::unique_ptr<WlanApiInterface> wlan_api_;
 
+  // Controls the polling update interval.
+  std::unique_ptr<WifiPollingPolicy> polling_policy_;
+
   // Holder for delayed tasks; takes care of cleanup.
   base::WeakPtrFactory<WifiDataProviderCommon> weak_factory_;
 

device/geolocation/wifi_data_provider_common_unittest.cc

@@ -14,7 +14,6 @@
 #include "base/third_party/dynamic_annotations/dynamic_annotations.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "device/geolocation/wifi_data_provider_manager.h"
-#include "device/geolocation/wifi_polling_policy.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
@@ -61,8 +60,7 @@ class MockPollingPolicy : public WifiPollingPolicy {
 class WifiDataProviderCommonWithMock : public WifiDataProviderCommon {
  public:
   WifiDataProviderCommonWithMock()
-      : wlan_api_(new MockWlanApi),
-        polling_policy_(std::make_unique<MockPollingPolicy>()) {}
+      : wlan_api_(new MockWlanApi), polling_policy_(new MockPollingPolicy) {}
 
   // WifiDataProviderCommon
   std::unique_ptr<WlanApiInterface> CreateWlanApi() override {
@@ -99,7 +97,6 @@ class GeolocationWifiDataProviderCommonTest : public testing::Test {
   void TearDown() override {
     provider_->RemoveCallback(&wifi_data_callback_);
     provider_->StopDataProvider();
-    WifiPollingPolicy::Shutdown();
   }
 
  protected:

device/geolocation/wifi_polling_policy.cc

-// Copyright 2018 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 "device/geolocation/wifi_polling_policy.h"
-
-namespace device {
-
-namespace {
-WifiPollingPolicy* g_wifi_polling_policy;
-}  // namespace
-
-// static
-void WifiPollingPolicy::Initialize(std::unique_ptr<WifiPollingPolicy> policy) {
-  g_wifi_polling_policy = policy.release();
-}
-
-// static
-void WifiPollingPolicy::Shutdown() {
-  if (g_wifi_polling_policy)
-    delete g_wifi_polling_policy;
-  g_wifi_polling_policy = nullptr;
-}
-
-// static
-WifiPollingPolicy* WifiPollingPolicy::Get() {
-  DCHECK(g_wifi_polling_policy);
-  return g_wifi_polling_policy;
-}
-
-// static
-bool WifiPollingPolicy::IsInitialized() {
-  return g_wifi_polling_policy != nullptr;
-}
-
-}  // namespace device

device/geolocation/wifi_polling_policy.h

@@ -5,36 +5,21 @@
 #ifndef DEVICE_GEOLOCATION_WIFI_POLLING_POLICY_H_
 #define DEVICE_GEOLOCATION_WIFI_POLLING_POLICY_H_
 
-#include <memory>
-
 #include "base/macros.h"
-#include "base/time/time.h"
-#include "device/geolocation/geolocation_export.h"
 
 namespace device {
 
 // Allows sharing and mocking of the update polling policy function.
-class DEVICE_GEOLOCATION_EXPORT WifiPollingPolicy {
+class WifiPollingPolicy {
  public:
-  virtual ~WifiPollingPolicy() = default;
-
-  // Methods for managing the single instance of WifiPollingPolicy. The WiFi
-  // policy is global so it can outlive the WifiDataProvider instance, which is
-  // shut down and destroyed when no WiFi scanning is active.
-  static void Initialize(std::unique_ptr<WifiPollingPolicy>);
-  static void Shutdown();
-  static WifiPollingPolicy* Get();
-  static bool IsInitialized();
-
+  WifiPollingPolicy() {}
+  virtual ~WifiPollingPolicy() {}
   // Calculates the new polling interval for wiFi scans, given the previous
   // interval and whether the last scan produced new results.
   virtual void UpdatePollingInterval(bool scan_results_differ) = 0;
   virtual int PollingInterval() = 0;
   virtual int NoWifiInterval() = 0;
 
- protected:
-  WifiPollingPolicy() = default;
-
  private:
   DISALLOW_COPY_AND_ASSIGN(WifiPollingPolicy);
 };
@@ -48,7 +33,6 @@ template <int DEFAULT_INTERVAL,
 class GenericWifiPollingPolicy : public WifiPollingPolicy {
  public:
   GenericWifiPollingPolicy() : polling_interval_(DEFAULT_INTERVAL) {}
-
   // WifiPollingPolicy
   void UpdatePollingInterval(bool scan_results_differ) override {
     if (scan_results_differ) {
@@ -61,37 +45,11 @@ class GenericWifiPollingPolicy : public WifiPollingPolicy {
       polling_interval_ = TWO_NO_CHANGE_INTERVAL;
     }
   }
-  int PollingInterval() override { return ComputeInterval(polling_interval_); }
-  int NoWifiInterval() override { return ComputeInterval(NO_WIFI_INTERVAL); }
+  int PollingInterval() override { return polling_interval_; }
+  int NoWifiInterval() override { return NO_WIFI_INTERVAL; }
 
  private:
-  int ComputeInterval(int polling_interval) {
-    base::Time now = base::Time::Now();
-
-    int64_t remaining_millis = 0;
-    if (!next_scan_.is_null()) {
-      // Compute the remaining duration of the current interval. If the interval
-      // is not yet complete, we will schedule a scan to occur once it is.
-      base::TimeDelta remaining = next_scan_ - now;
-      remaining_millis = remaining.InMilliseconds();
-    }
-
-    // If the current interval is complete (or if this is our first scan), scan
-    // now and schedule the next scan to occur at |polling_interval|
-    // milliseconds into the future.
-    if (remaining_millis <= 0) {
-      next_scan_ = now + base::TimeDelta::FromMilliseconds(polling_interval);
-      remaining_millis = 0;
-    }
-
-    return remaining_millis;
-  }
-
   int polling_interval_;
-
-  // The scheduled time of the next scan, or a null value if no scan has
-  // occurred yet.
-  base::Time next_scan_;
 };
 
 }  // namespace device