Fix starting network location data provider thread issue.

Have a reference count in each network location data provider and check the reference count in StartProvider to see if the network location provider thread has already been started. Before it would just Start the thread again if StartProvider was called more than once.

BUG=56245
TEST= unit test --gtest_filter=*Geolo*Netw*

Review URL: http://codereview.chromium.org/3405017

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@60036 0039d316-1c4b-4281-b951-d872f2087c98

4 files changed, 50 insertions, 6 deletions

diff --git a/chrome/browser/geolocation/device_data_provider.h b/chrome/browser/geolocation/device_data_provider.h
index 6e659a2..9cd4390 100644
--- a/chrome/browser/geolocation/device_data_provider.h
+++ b/chrome/browser/geolocation/device_data_provider.h
@@ -350,16 +350,20 @@ class DeviceDataProvider : public NonThreadSafe {
   // Adds a listener, which will be called back with DeviceDataUpdateAvailable
   // whenever new data is available. Returns the singleton instance.
   static DeviceDataProvider* Register(ListenerInterface* listener) {
+    bool need_to_start_thread = false;
     if (!instance_) {
       instance_ = new DeviceDataProvider();
+      need_to_start_thread = true;
     }
     DCHECK(instance_);
     DCHECK(instance_->CalledOnValidThread());
     instance_->AddListener(listener);
     // Start the provider after adding the listener, to avoid any race in
     // it receiving an early callback.
-    bool started = instance_->StartDataProvider();
-    DCHECK(started);
+    if (need_to_start_thread) {
+      bool started = instance_->StartDataProvider();
+      DCHECK(started);
+    }
     return instance_;
   }
 
diff --git a/chrome/browser/geolocation/gateway_data_provider_common.cc b/chrome/browser/geolocation/gateway_data_provider_common.cc
index 5ad7d04..cd9d189 100644
--- a/chrome/browser/geolocation/gateway_data_provider_common.cc
+++ b/chrome/browser/geolocation/gateway_data_provider_common.cc
@@ -15,11 +15,12 @@ GatewayDataProviderCommon::GatewayDataProviderCommon()
 GatewayDataProviderCommon::~GatewayDataProviderCommon() {
   // Thread must be stopped before entering destructor chain to avoid race
   // conditions; see comment in DeviceDataProvider::Unregister.
-  DCHECK(!IsRunning()) << "Must call StopDataProvider before destroying me";
+  DCHECK(!IsRunning()); // Must call StopDataProvider before destroying me.
 }
 
 bool GatewayDataProviderCommon::StartDataProvider() {
   DCHECK(CalledOnClientThread());
+  DCHECK(!IsRunning());  // StartDataProvider must only be called once.
   return Start();
 }
 
diff --git a/chrome/browser/geolocation/network_location_provider_unittest.cc b/chrome/browser/geolocation/network_location_provider_unittest.cc
index da4edb1..7cd403d 100644
--- a/chrome/browser/geolocation/network_location_provider_unittest.cc
+++ b/chrome/browser/geolocation/network_location_provider_unittest.cc
@@ -58,7 +58,8 @@ class MockDeviceDataProviderImpl
     return instance_;
   }
 
-  MockDeviceDataProviderImpl() : got_data_(true) {
+  MockDeviceDataProviderImpl() : start_calls_(0), stop_calls_(0),
+      got_data_(true) {
   }
 
   virtual ~MockDeviceDataProviderImpl() {
@@ -68,9 +69,12 @@ class MockDeviceDataProviderImpl
 
   // DeviceDataProviderImplBase implementation.
   virtual bool StartDataProvider() {
+    start_calls_++;
     return true;
   }
-  virtual void StopDataProvider() {}
+  virtual void StopDataProvider() {
+    stop_calls_++;
+  }
   virtual bool GetData(DataType* data_out) {
     CHECK(data_out);
     *data_out = data_;
@@ -86,6 +90,8 @@ class MockDeviceDataProviderImpl
   }
 
   void set_got_data(bool got_data) { got_data_ = got_data; }
+  int start_calls_;
+  int stop_calls_;
 
  private:
   static MockDeviceDataProviderImpl<DataType>* instance_;
@@ -345,6 +351,38 @@ TEST_F(GeolocationNetworkProviderTest, StartProvider) {
   CheckEmptyRequestIsValid(fetcher->upload_data());
 }
 
+TEST_F(GeolocationNetworkProviderTest, MultipleStartProvider) {
+  scoped_ptr<LocationProviderBase> provider_1(CreateProvider(true));
+  scoped_ptr<LocationProviderBase> provider_2(CreateProvider(true));
+  EXPECT_EQ(0, gateway_data_provider_->start_calls_);
+  EXPECT_EQ(0, radio_data_provider_->start_calls_);
+  EXPECT_EQ(0, wifi_data_provider_->start_calls_);
+  EXPECT_EQ(0, gateway_data_provider_->stop_calls_);
+  EXPECT_EQ(0, radio_data_provider_->stop_calls_);
+  EXPECT_EQ(0, wifi_data_provider_->stop_calls_);
+  // Start first provider.
+  EXPECT_TRUE(provider_1->StartProvider(false));
+  EXPECT_EQ(1, gateway_data_provider_->start_calls_);
+  EXPECT_EQ(1, radio_data_provider_->start_calls_);
+  EXPECT_EQ(1, wifi_data_provider_->start_calls_);
+  // Start second provider.
+  EXPECT_TRUE(provider_2->StartProvider(false));
+  EXPECT_EQ(1, gateway_data_provider_->start_calls_);
+  EXPECT_EQ(1, radio_data_provider_->start_calls_);
+  EXPECT_EQ(1, wifi_data_provider_->start_calls_);
+
+  // Stop first provider.
+  provider_1->StopProvider();
+  EXPECT_EQ(0, gateway_data_provider_->stop_calls_);
+  EXPECT_EQ(0, radio_data_provider_->stop_calls_);
+  EXPECT_EQ(0, wifi_data_provider_->stop_calls_);
+  // Stop second provider.
+  provider_2->StopProvider();
+  EXPECT_EQ(1, gateway_data_provider_->stop_calls_);
+  EXPECT_EQ(1, radio_data_provider_->stop_calls_);
+  EXPECT_EQ(1, wifi_data_provider_->stop_calls_);
+}
+
 TEST_F(GeolocationNetworkProviderTest, MultiRegistrations) {
   // TODO(joth): Strictly belongs in a base-class unit test file.
   MessageLoopQuitListener listener;
diff --git a/chrome/browser/geolocation/wifi_data_provider_common.cc b/chrome/browser/geolocation/wifi_data_provider_common.cc
index 39df876..bfa1c5e 100644
--- a/chrome/browser/geolocation/wifi_data_provider_common.cc
+++ b/chrome/browser/geolocation/wifi_data_provider_common.cc
@@ -25,11 +25,12 @@ WifiDataProviderCommon::WifiDataProviderCommon()
 WifiDataProviderCommon::~WifiDataProviderCommon() {
   // Thread must be stopped before entering destructor chain to avoid race
   // conditions; see comment in DeviceDataProvider::Unregister.
-  DCHECK(!IsRunning()) << "Must call StopDataProvider before destroying me";
+  DCHECK(!IsRunning());  // Must call StopDataProvider before destroying me.
 }
 
 bool WifiDataProviderCommon::StartDataProvider() {
   DCHECK(CalledOnClientThread());
+  DCHECK(!IsRunning());  // StartDataProvider must only be called once.
   return Start();
 }