Simplify (honest!) the threading design for data providers: the API is now single threaded, and worker

threads are encapsulated within the data provider implementation.

BUG=None
TEST=unit_tests.exe --gtest_filter=Geol* --gtest_repeat=10000 --gtest_break_on_failure

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

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

9 files changed, 205 insertions, 185 deletions

diff --git a/chrome/browser/geolocation/device_data_provider.h b/chrome/browser/geolocation/device_data_provider.h
index a03011b..c70a777 100644
--- a/chrome/browser/geolocation/device_data_provider.h
+++ b/chrome/browser/geolocation/device_data_provider.h
@@ -29,10 +29,12 @@
 #include <vector>
 
 #include "base/basictypes.h"
-#include "base/lock.h"
+#include "base/message_loop.h"
+#include "base/non_thread_safe.h"
 #include "base/string16.h"
 #include "base/string_util.h"
 #include "base/scoped_ptr.h"
+#include "base/task.h"
 
 // The following data structures are used to store cell radio data and wifi
 // data. See the Geolocation API design document at
@@ -181,65 +183,94 @@ struct WifiData {
 template<typename DataType>
 class DeviceDataProvider;
 
+// This class just exists to work-around MSVC2005 not being able to have a
+// template class implement RefCountedThreadSafe
+class DeviceDataProviderImplBaseHack
+    : public base::RefCountedThreadSafe<DeviceDataProviderImplBaseHack> {
+ protected:
+  friend class base::RefCountedThreadSafe<DeviceDataProviderImplBaseHack>;
+  virtual ~DeviceDataProviderImplBaseHack() {}
+};
+
+// See class DeviceDataProvider for the public client API.
 // DeviceDataProvider uses containment to hide platform-specific implementation
 // details from common code. This class provides common functionality for these
-// contained implementation classes.
+// contained implementation classes. This is a modified pimpl pattern: this
+// class needs to be in the public header due to use of templating.
 template<typename DataType>
-class DeviceDataProviderImplBase {
+class DeviceDataProviderImplBase : public DeviceDataProviderImplBaseHack {
  public:
-  DeviceDataProviderImplBase() : container_(NULL) {}
-  virtual ~DeviceDataProviderImplBase() {}
+  DeviceDataProviderImplBase()
+      : container_(NULL), client_loop_(MessageLoop::current()) {
+    DCHECK(client_loop_);
+  }
 
   virtual bool StartDataProvider() = 0;
-
+  virtual void StopDataProvider() = 0;
   virtual bool GetData(DataType* data) = 0;
 
   // Sets the container of this class, which is of type DeviceDataProvider.
   // This is required to pass as a parameter when making the callback to
   // listeners.
   void SetContainer(DeviceDataProvider<DataType>* container) {
+    DCHECK(CalledOnClientThread());
     container_ = container;
   }
 
   typedef typename DeviceDataProvider<DataType>::ListenerInterface
           ListenerInterface;
   void AddListener(ListenerInterface* listener) {
-    AutoLock mutex(listeners_mutex_);
+    DCHECK(CalledOnClientThread());
     listeners_.insert(listener);
   }
   bool RemoveListener(ListenerInterface* listener) {
-    AutoLock mutex(listeners_mutex_);
-    typename ListenersSet::iterator iter = find(listeners_.begin(),
-                                                listeners_.end(),
-                                                listener);
-    if (iter == listeners_.end()) {
-      return false;
-    }
-    listeners_.erase(iter);
-    return true;
+    DCHECK(CalledOnClientThread());
+    return listeners_.erase(listener) == 1;
+  }
+
+  bool has_listeners() const {
+    DCHECK(CalledOnClientThread());
+    return !listeners_.empty();
   }
 
  protected:
+  virtual ~DeviceDataProviderImplBase() {
+    DCHECK(CalledOnClientThread());
+  }
+
   // Calls DeviceDataUpdateAvailable() on all registered listeners.
   typedef std::set<ListenerInterface*> ListenersSet;
   void NotifyListeners() {
-    AutoLock lock(listeners_mutex_);
-    for (typename ListenersSet::const_iterator iter = listeners_.begin();
-         iter != listeners_.end();
-         ++iter) {
-      (*iter)->DeviceDataUpdateAvailable(container_);
-    }
+    // Always make the nitofy callback via a posted task, se we can unwind
+    // callstack here and make callback without causing client re-entrancy.
+    client_loop_->PostTask(FROM_HERE, NewRunnableMethod(this,
+        &DeviceDataProviderImplBase<DataType>::NotifyListenersInClientLoop));
+  }
+
+  bool CalledOnClientThread() const {
+    return MessageLoop::current() == this->client_loop_;
   }
 
  private:
+  void NotifyListenersInClientLoop() {
+    DCHECK(CalledOnClientThread());
+    // It's possible that all the listeners (and the container) went away
+    // whilst this task was pending. This is fine; the loop will be a no-op.
+    typename ListenersSet::const_iterator iter = listeners_.begin();
+    while (iter != listeners_.end()) {
+      ListenerInterface* listener = *iter;
+      ++iter;  // Advance iter before callback, in case listener unregisters.
+      listener->DeviceDataUpdateAvailable(container_);
+    }
+  }
+
   DeviceDataProvider<DataType>* container_;
 
-  // The listeners to this class and their mutex.
-  // TODO(joth): Once we've established the client is always single threaded,
-  // remove mutex and instead capture client's MessageLoop to stage the
-  // NotifyListeners callback via.
+  // Reference to the client's message loop, all callbacks and access to
+  // the listeners_ member should happen in this context.
+  MessageLoop* client_loop_;
+
   ListenersSet listeners_;
-  Lock listeners_mutex_;
 
   DISALLOW_COPY_AND_ASSIGN(DeviceDataProviderImplBase);
 };
@@ -253,13 +284,12 @@ typedef DeviceDataProviderImplBase<WifiData> WifiDataProviderImplBase;
 // location providers. These location providers access the instance through the
 // Register and Unregister methods.
 template<typename DataType>
-class DeviceDataProvider {
+class DeviceDataProvider : public NonThreadSafe {
  public:
   // Interface to be implemented by listeners to a device data provider.
   class ListenerInterface {
    public:
-    // NOTE this may be called back in the context of the implementation private
-    // worker thread. (TODO Is there a naming convention to use for this?)
+    // Will be called in the context of the thread that called Register().
     virtual void DeviceDataUpdateAvailable(
         DeviceDataProvider<DataType>* provider) = 0;
     virtual ~ListenerInterface() {}
@@ -281,24 +311,15 @@ class DeviceDataProvider {
   // Adds a listener, which will be called back with DeviceDataUpdateAvailable
   // whenever new data is available. Returns the singleton instance.
   static DeviceDataProvider* Register(ListenerInterface* listener) {
-    // TODO(joth): The following comment applied when this was used in Gears;
-    // revisit if this is still needed once usage is established in Chromium.
-    // We protect against Register and Unregister being called asynchronously
-    // from different threads. This is the case when a device data provider is
-    // used by a NetworkLocationProvider object. Register is always called from
-    // the JavaScript thread. Unregister is called when NetworkLocationProvider
-    // objects are destructed, which happens asynchronously once the
-    // NetworkLocationProvider HTTP request has completed.
-    AutoLock mutex(instance_mutex_);
     if (!instance_) {
       instance_ = new DeviceDataProvider();
     }
     DCHECK(instance_);
-    instance_->Ref();
+    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_->impl_->StartDataProvider();
+    bool started = instance_->StartDataProvider();
     DCHECK(started);
     return instance_;
   }
@@ -306,11 +327,17 @@ class DeviceDataProvider {
   // Removes a listener. If this is the last listener, deletes the singleton
   // instance. Return value indicates success.
   static bool Unregister(ListenerInterface* listener) {
-    AutoLock mutex(instance_mutex_);
+    DCHECK(instance_);
+    DCHECK(instance_->CalledOnValidThread());
+    DCHECK(instance_->has_listeners());
     if (!instance_->RemoveListener(listener)) {
       return false;
     }
-    if (instance_->Unref()) {
+    if (!instance_->has_listeners()) {
+      // Must stop the provider (and any implementation threads) before
+      // destroying to avoid any race conditions in access to the provider in
+      // the destructor chain.
+      instance_->StopDataProvider();
       delete instance_;
       instance_ = NULL;
     }
@@ -321,26 +348,22 @@ class DeviceDataProvider {
   // value indicates whether this is all the data the provider could ever
   // obtain.
   bool GetData(DataType* data) {
+    DCHECK(this->CalledOnValidThread());
     return impl_->GetData(data);
   }
 
  private:
   // Private constructor and destructor, callers access singleton through
   // Register and Unregister.
-  DeviceDataProvider() : count_(0) {
+  DeviceDataProvider() {
     DCHECK(factory_function_);
-    impl_.reset((*factory_function_)());
+    impl_ = (*factory_function_)();
+    DCHECK(impl_);
     impl_->SetContainer(this);
   }
-  virtual ~DeviceDataProvider() {}
-
-  void Ref() {
-    ++count_;
-  }
-  // Returns true when the ref count transitions from 1 to 0.
-  bool Unref() {
-    --count_;
-    return count_ == 0;
+  virtual ~DeviceDataProvider() {
+    DCHECK(impl_);
+    impl_->SetContainer(NULL);
   }
 
   void AddListener(ListenerInterface* listener) {
@@ -351,29 +374,36 @@ class DeviceDataProvider {
     return impl_->RemoveListener(listener);
   }
 
+  bool has_listeners() const {
+    return impl_->has_listeners();
+  }
+
+  bool StartDataProvider() {
+    return impl_->StartDataProvider();
+  }
+
+  void StopDataProvider() {
+    impl_->StopDataProvider();
+  }
+
   static DeviceDataProviderImplBase<DataType>* DefaultFactoryFunction();
 
-  // The singleton instance of this class and its mutex.
+  // The singleton-like instance of this class. (Not 'true' singleton, as it
+  // may go through multiple create/destroy/create cycles per process instance,
+  // e.g. when under test).
   static DeviceDataProvider* instance_;
-  static Lock instance_mutex_;
 
   // The factory function used to create the singleton instance.
   static ImplFactoryFunction factory_function_;
 
   // The internal implementation.
-  scoped_ptr<DeviceDataProviderImplBase<DataType> > impl_;
-
-  int count_;
+  scoped_refptr<DeviceDataProviderImplBase<DataType> > impl_;
 
   DISALLOW_EVIL_CONSTRUCTORS(DeviceDataProvider);
 };
 
 // static
 template<typename DataType>
-Lock DeviceDataProvider<DataType>::instance_mutex_;
-
-// static
-template<typename DataType>
 DeviceDataProvider<DataType>* DeviceDataProvider<DataType>::instance_ = NULL;
 
 // static
diff --git a/chrome/browser/geolocation/empty_device_data_provider.h b/chrome/browser/geolocation/empty_device_data_provider.h
index 1c932fd..ac68473 100644
--- a/chrome/browser/geolocation/empty_device_data_provider.h
+++ b/chrome/browser/geolocation/empty_device_data_provider.h
@@ -18,6 +18,7 @@ class EmptyDeviceDataProvider : public DeviceDataProviderImplBase<DataType> {
 
   // DeviceDataProviderImplBase implementation
   virtual bool StartDataProvider() { return true; }
+  virtual void StopDataProvider() { }
   virtual bool GetData(DataType *data) {
     DCHECK(data);
     // This is all the data we can get - nothing.
diff --git a/chrome/browser/geolocation/location_provider.cc b/chrome/browser/geolocation/location_provider.cc
index 34d1084..c2135b5 100644
--- a/chrome/browser/geolocation/location_provider.cc
+++ b/chrome/browser/geolocation/location_provider.cc
@@ -8,35 +8,37 @@
 #include "chrome/browser/geolocation/location_provider.h"
 
 #include <assert.h>
+#include "base/logging.h"
 
-LocationProviderBase::LocationProviderBase()
-    : client_loop_(MessageLoop::current()) {
+LocationProviderBase::LocationProviderBase() {
 }
 
 LocationProviderBase::~LocationProviderBase() {
-  DCHECK_EQ(client_loop_, MessageLoop::current());
+  DCHECK(CalledOnValidThread());
 }
 
 void LocationProviderBase::RegisterListener(ListenerInterface* listener) {
+  DCHECK(CalledOnValidThread());
   DCHECK(listener);
-  if (RunInClientThread(FROM_HERE,
-                        &LocationProviderBase::RegisterListener, listener))
-    return;
-  ListenerMap::iterator iter = listeners_.find(listener);
-  if (iter == listeners_.end()) {
-    std::pair<ListenerMap::iterator, bool> result =
-        listeners_.insert(std::make_pair(listener, 0));
-    DCHECK(result.second);
-    iter = result.first;
+  std::pair<ListenerMap::iterator, bool> result =
+      listeners_.insert(std::make_pair(listener, 0));
+  DCHECK(result.first != listeners_.end());
+
+  int& ref_count = result.first->second;
+  const bool& is_new = result.second;
+  ++ref_count;
+
+  // Check the post condition...
+  if (is_new) {
+    DCHECK(ref_count == 1);
+  } else {
+    DCHECK(ref_count > 1);
   }
-  ++iter->second;
 }
 
 void LocationProviderBase::UnregisterListener(ListenerInterface *listener) {
+  DCHECK(CalledOnValidThread());
   DCHECK(listener);
-  if (RunInClientThread(FROM_HERE,
-                        &LocationProviderBase::UnregisterListener, listener))
-    return;
   ListenerMap::iterator iter = listeners_.find(listener);
   if (iter != listeners_.end()) {
     if (--iter->second == 0) {
@@ -45,10 +47,12 @@ void LocationProviderBase::UnregisterListener(ListenerInterface *listener) {
   }
 }
 
+bool LocationProviderBase::has_listeners() const {
+  return !listeners_.empty();
+}
+
 void LocationProviderBase::UpdateListeners() {
-  // Currently we required location provider implementations to make
-  // notifications from the client thread. This could be relaxed if needed.
-  CheckRunningInClientLoop();
+  DCHECK(CalledOnValidThread());
   for (ListenerMap::const_iterator iter = listeners_.begin();
        iter != listeners_.end();
        ++iter) {
@@ -57,9 +61,7 @@ void LocationProviderBase::UpdateListeners() {
 }
 
 void LocationProviderBase::InformListenersOfMovement() {
-  // Currently we required location provider implementations to make
-  // notifications from the client thread. This could be relaxed if needed.
-  CheckRunningInClientLoop();
+  DCHECK(CalledOnValidThread());
   for (ListenerMap::const_iterator iter = listeners_.begin();
        iter != listeners_.end();
        ++iter) {
@@ -67,10 +69,6 @@ void LocationProviderBase::InformListenersOfMovement() {
   }
 }
 
-void LocationProviderBase::CheckRunningInClientLoop() {
-  DCHECK_EQ(MessageLoop::current(), client_loop());
-}
-
 // Currently no platforms have a GPS location provider.
 LocationProviderBase* NewGpsLocationProvider() {
   return NULL;
diff --git a/chrome/browser/geolocation/location_provider.h b/chrome/browser/geolocation/location_provider.h
index a9960805..eca5b57 100644
--- a/chrome/browser/geolocation/location_provider.h
+++ b/chrome/browser/geolocation/location_provider.h
@@ -14,19 +14,15 @@
 #define CHROME_BROWSER_GEOLOCATION_LOCATION_PROVIDER_H_
 
 #include <map>
-#include "base/lock.h"
-#include "base/message_loop.h"
-#include "base/ref_counted.h"
+#include "base/non_thread_safe.h"
 #include "base/string16.h"
-#include "base/task.h"
 
 class GURL;
 struct Position;
 class URLRequestContextGetter;
 
 // The base class used by all location providers.
-class LocationProviderBase
-    : public base::RefCountedThreadSafe<LocationProviderBase> {
+class LocationProviderBase : public NonThreadSafe {
  public:
   // Provides storage for the access token used in the network request.
   // Normally the client (i.e. geolocation controller) implements this, but
@@ -62,18 +58,19 @@ class LocationProviderBase
     virtual ~ListenerInterface() {}
   };
 
-  // TODO(joth): Make register / unregister non-virtual.
+  virtual ~LocationProviderBase();
+
   // Registers a listener, which will be called back on
   // ListenerInterface::LocationUpdateAvailable as soon as a position is
   // available and again whenever a new position is available. Ref counts the
   // listener to handle multiple calls to this method.
-  virtual void RegisterListener(ListenerInterface* listener);
+  void RegisterListener(ListenerInterface* listener);
   // Unregisters a listener. Unrefs the listener to handle multiple calls to
   // this method. Once the ref count reaches zero, the listener is removed and
   // once this method returns, no further calls to
   // ListenerInterface::LocationUpdateAvailable will be made for this listener.
   // It may block if a callback is in progress.
-  virtual void UnregisterListener(ListenerInterface* listener);
+  void UnregisterListener(ListenerInterface* listener);
 
   // Interface methods
   // Returns false if the provider failed to start.
@@ -84,12 +81,10 @@ class LocationProviderBase
   // as possible. Default implementation does nothing.
   virtual void UpdatePosition() {}
 
+  bool has_listeners() const;
+
  protected:
-  // Instances should only be destroyed via the thread safe ref count; derived
-  // classes should not have a public destructor.
-  friend class base::RefCountedThreadSafe<LocationProviderBase>;
   LocationProviderBase();
-  virtual ~LocationProviderBase();
 
   // Inform listeners that a new position or error is available, using
   // LocationUpdateAvailable.
@@ -97,38 +92,11 @@ class LocationProviderBase
   // Inform listeners that movement has been detected, using MovementDetected.
   virtual void InformListenersOfMovement();
 
-  MessageLoop* client_loop() { return client_loop_; }
-  void CheckRunningInClientLoop();
-
  private:
-  // Utility methods to delegate method calls into the client thread
-  template <class Method>
-  bool RunInClientThread(const tracked_objects::Location& from_here,
-                         Method method) {
-    if (MessageLoop::current() == client_loop_) {
-      return false;
-    }
-    client_loop_->PostTask(from_here, NewRunnableMethod(this, method));
-    return true;
-  }
-  template <class Method, class A>
-  bool RunInClientThread(const tracked_objects::Location& from_here,
-                         Method method, const A& a) {
-    if (MessageLoop::current() == client_loop_) {
-      return false;
-    }
-    client_loop_->PostTask(from_here, NewRunnableMethod(this, method, a));
-    return true;
-  }
-
   // The listeners registered to this provider. For each listener, we store a
   // ref count.
   typedef std::map<ListenerInterface*, int> ListenerMap;
   ListenerMap listeners_;
-
-  // Reference to the client's message loop, all callbacks and access to
-  // the listeners_ member should happen in this context.
-  MessageLoop* client_loop_;
 };
 
 // Factory functions for the various types of location provider to abstract over
diff --git a/chrome/browser/geolocation/network_location_provider.cc b/chrome/browser/geolocation/network_location_provider.cc
index 0fb5fe1..f4d0455 100644
--- a/chrome/browser/geolocation/network_location_provider.cc
+++ b/chrome/browser/geolocation/network_location_provider.cc
@@ -178,7 +178,7 @@ void NetworkLocationProvider::LocationResponseAvailable(
     const Position& position,
     bool server_error,
     const string16& access_token) {
-  CheckRunningInClientLoop();
+  DCHECK(CalledOnValidThread());
   // Record the position and update our cache.
   {
     AutoLock position_lock(position_mutex_);
@@ -202,7 +202,7 @@ void NetworkLocationProvider::LocationResponseAvailable(
 }
 
 bool NetworkLocationProvider::StartProvider() {
-  CheckRunningInClientLoop();
+  DCHECK(CalledOnValidThread());
   DCHECK(radio_data_provider_ == NULL);
   DCHECK(wifi_data_provider_ == NULL);
   if (!request_->url().is_valid()) {
@@ -234,7 +234,7 @@ bool NetworkLocationProvider::StartProvider() {
 
 // Other methods
 void NetworkLocationProvider::RequestPosition() {
-  CheckRunningInClientLoop();
+  DCHECK(CalledOnValidThread());
 
   delayed_start_task_.RevokeAll();
   const Position* cached_position;
@@ -278,11 +278,7 @@ void NetworkLocationProvider::RequestPosition() {
 }
 
 void NetworkLocationProvider::OnDeviceDataUpdated() {
-  if (MessageLoop::current() != client_loop()) {
-    client_loop()->PostTask(FROM_HERE,
-        NewRunnableMethod(this, &NetworkLocationProvider::OnDeviceDataUpdated));
-    return;
-  }
+  DCHECK(CalledOnValidThread());
   device_data_updated_timestamp_ = GetCurrentTimeMillis();
 
   is_new_data_available_ = is_radio_data_complete_ || is_radio_data_complete_;
diff --git a/chrome/browser/geolocation/network_location_provider_unittest.cc b/chrome/browser/geolocation/network_location_provider_unittest.cc
index 51593c0..2a622e4 100644
--- a/chrome/browser/geolocation/network_location_provider_unittest.cc
+++ b/chrome/browser/geolocation/network_location_provider_unittest.cc
@@ -27,7 +27,7 @@ class MessageLoopQuitListener
       : client_message_loop_(MessageLoop::current()),
         updated_provider_(NULL),
         movement_provider_(NULL) {
-    DCHECK(client_message_loop_);
+    CHECK(client_message_loop_);
   }
   // ListenerInterface
   virtual void LocationUpdateAvailable(LocationProviderBase* provider) {
@@ -96,6 +96,7 @@ class MockDeviceDataProviderImpl
   virtual bool StartDataProvider() {
     return true;
   }
+  virtual void StopDataProvider() {}
   virtual bool GetData(DataType* data_out) {
     CHECK(data_out);
     AutoLock lock(data_mutex_);
@@ -127,7 +128,7 @@ MockDeviceDataProviderImpl<DataType>*
 MockDeviceDataProviderImpl<DataType>::instance_ = NULL;
 
 // Main test fixture
-class NetworkLocationProviderTest : public testing::Test {
+class GeolocationNetworkProviderTest : public testing::Test {
  public:
   virtual void SetUp() {
     URLFetcher::set_factory(&url_fetcher_factory_);
@@ -149,7 +150,7 @@ class NetworkLocationProviderTest : public testing::Test {
   }
 
  protected:
-  NetworkLocationProviderTest() : test_server_url_(kTestServerUrl) {
+  GeolocationNetworkProviderTest() : test_server_url_(kTestServerUrl) {
     // TODO(joth): Really these should be in SetUp, not here, but they take no
     // effect on Mac OS Release builds if done there. I kid not. Figure out why.
     RadioDataProvider::SetFactory(
@@ -257,17 +258,17 @@ class NetworkLocationProviderTest : public testing::Test {
 };
 
 
-TEST_F(NetworkLocationProviderTest, CreateDestroy) {
+TEST_F(GeolocationNetworkProviderTest, CreateDestroy) {
   // Test fixture members were SetUp correctly.
   EXPECT_EQ(&main_message_loop_, MessageLoop::current());
-  scoped_refptr<LocationProviderBase> provider(CreateProvider());
+  scoped_ptr<LocationProviderBase> provider(CreateProvider());
   EXPECT_TRUE(NULL != provider.get());
-  provider = NULL;
+  provider.reset();
   SUCCEED();
 }
 
-TEST_F(NetworkLocationProviderTest, StartProvider) {
-  scoped_refptr<LocationProviderBase> provider(CreateProvider());
+TEST_F(GeolocationNetworkProviderTest, StartProvider) {
+  scoped_ptr<LocationProviderBase> provider(CreateProvider());
   EXPECT_TRUE(provider->StartProvider());
   TestURLFetcher* fetcher = url_fetcher_factory_.GetFetcherByID(0);
   ASSERT_TRUE(fetcher != NULL);
@@ -278,8 +279,24 @@ TEST_F(NetworkLocationProviderTest, StartProvider) {
   CheckEmptyRequestIsValid(fetcher->upload_data());
 }
 
-TEST_F(NetworkLocationProviderTest, MultipleWifiScansComplete) {
-  scoped_refptr<LocationProviderBase> provider(CreateProvider());
+TEST_F(GeolocationNetworkProviderTest, MultiRegistrations) {
+  // TODO(joth): Strictly belongs in a base-class unit test file.
+  MessageLoopQuitListener listener;
+  scoped_ptr<LocationProviderBase> provider(CreateProvider());
+  EXPECT_FALSE(provider->has_listeners());
+  provider->RegisterListener(&listener);
+  EXPECT_TRUE(provider->has_listeners());
+  provider->RegisterListener(&listener);
+  EXPECT_TRUE(provider->has_listeners());
+
+  provider->UnregisterListener(&listener);
+  EXPECT_TRUE(provider->has_listeners());
+  provider->UnregisterListener(&listener);
+  EXPECT_FALSE(provider->has_listeners());
+}
+
+TEST_F(GeolocationNetworkProviderTest, MultipleWifiScansComplete) {
+  scoped_ptr<LocationProviderBase> provider(CreateProvider());
   EXPECT_TRUE(provider->StartProvider());
 
   TestURLFetcher* fetcher = url_fetcher_factory_.GetFetcherByID(0);
@@ -311,6 +328,7 @@ TEST_F(NetworkLocationProviderTest, MultipleWifiScansComplete) {
   const int kFirstScanAps = 6;
   MockDeviceDataProviderImpl<WifiData>::instance()->SetData(
       CreateReferenceWifiScanData(kFirstScanAps));  // Will notify listeners
+  main_message_loop_.RunAllPending();
   fetcher = url_fetcher_factory_.GetFetcherByID(kFirstScanAps);
   ASSERT_TRUE(fetcher != NULL);
   // The request should have access token (set previously) and the wifi data.
@@ -352,6 +370,7 @@ TEST_F(NetworkLocationProviderTest, MultipleWifiScansComplete) {
   const int kSecondScanAps = kFirstScanAps - 1;
   MockDeviceDataProviderImpl<WifiData>::instance()->SetData(
       CreateReferenceWifiScanData(kSecondScanAps));
+  main_message_loop_.RunAllPending();
   fetcher = url_fetcher_factory_.GetFetcherByID(kSecondScanAps);
   EXPECT_FALSE(fetcher);
 
@@ -364,6 +383,7 @@ TEST_F(NetworkLocationProviderTest, MultipleWifiScansComplete) {
   const int kThirdScanAps = kFirstScanAps * 2 + 1;
   MockDeviceDataProviderImpl<WifiData>::instance()->SetData(
       CreateReferenceWifiScanData(kThirdScanAps));
+  main_message_loop_.RunAllPending();
   fetcher = url_fetcher_factory_.GetFetcherByID(kThirdScanAps);
   EXPECT_TRUE(fetcher);
   // ...reply with a network error.
@@ -383,6 +403,7 @@ TEST_F(NetworkLocationProviderTest, MultipleWifiScansComplete) {
       url_fetcher_factory_.GetFetcherByID(kFirstScanAps);
   MockDeviceDataProviderImpl<WifiData>::instance()->SetData(
       CreateReferenceWifiScanData(kFirstScanAps));
+  main_message_loop_.RunAllPending();
   fetcher = url_fetcher_factory_.GetFetcherByID(kFirstScanAps);
   EXPECT_EQ(orig_fetcher, fetcher);  // No new request created.
 
diff --git a/chrome/browser/geolocation/wifi_data_provider_unittest_win.cc b/chrome/browser/geolocation/wifi_data_provider_unittest_win.cc
index c6a3b60..90a7e36 100644
--- a/chrome/browser/geolocation/wifi_data_provider_unittest_win.cc
+++ b/chrome/browser/geolocation/wifi_data_provider_unittest_win.cc
@@ -41,67 +41,65 @@ class MessageLoopQuitListener
  public:
   explicit MessageLoopQuitListener(MessageLoop* message_loop)
       : message_loop_to_quit_(message_loop) {
-    DCHECK(message_loop_to_quit_ != NULL);
+    CHECK(message_loop_to_quit_ != NULL);
   }
   // ListenerInterface
   virtual void DeviceDataUpdateAvailable(
       DeviceDataProvider<WifiData>* provider) {
-    // We expect the callback to come from the provider's internal worker
-    // thread. This is not a strict requirement, just a lot of the complexity
-    // here is predicated on the need to cope with this scenario!
-    EXPECT_NE(MessageLoop::current(), message_loop_to_quit_);
+    // Provider should call back on client's thread.
+    EXPECT_EQ(MessageLoop::current(), message_loop_to_quit_);
     provider_ = provider;
-    // Can't call Quit() directly on another thread's message loop.
-    message_loop_to_quit_->PostTask(FROM_HERE, new MessageLoop::QuitTask);
+    message_loop_to_quit_->Quit();
   }
   MessageLoop* message_loop_to_quit_;
   DeviceDataProvider<WifiData>* provider_;
 };
 
+Win32WifiDataProvider* CreateWin32WifiDataProvider(MockWlanApi** wlan_api_out) {
+  Win32WifiDataProvider* provider = new Win32WifiDataProvider;
+  *wlan_api_out = new MockWlanApi;
+  provider->inject_mock_wlan_api(*wlan_api_out);  // Takes ownership.
+  provider->inject_mock_polling_policy(new MockPollingPolicy);  // ditto
+  return provider;
+}
+
+WifiDataProviderImplBase* CreateWin32WifiDataProvider() {
+  MockWlanApi* wlan_api;
+  return CreateWin32WifiDataProvider(&wlan_api);
+}
+}  // namespace
+
 // Main test fixture
-class Win32WifiDataProviderTest : public testing::Test {
+class GeolocationWin32WifiDataProviderTest : public testing::Test {
  public:
-  static Win32WifiDataProvider* CreateWin32WifiDataProvider(
-      MockWlanApi** wlan_api_out) {
-    Win32WifiDataProvider* provider = new Win32WifiDataProvider;
-    *wlan_api_out = new MockWlanApi;
-    provider->inject_mock_wlan_api(*wlan_api_out);  // Takes ownership.
-    provider->inject_mock_polling_policy(new MockPollingPolicy);  // ditto
-    return provider;
-  }
   virtual void SetUp() {
-    provider_.reset(CreateWin32WifiDataProvider(&wlan_api_));
+    provider_ = CreateWin32WifiDataProvider(&wlan_api_);
   }
   virtual void TearDown() {
-    provider_.reset();
+    provider_->StopDataProvider();
+    provider_ = NULL;
   }
 
  protected:
   MessageLoop main_message_loop_;
-  scoped_ptr<Win32WifiDataProvider> provider_;
+  scoped_refptr<Win32WifiDataProvider> provider_;
   MockWlanApi* wlan_api_;
 };
 
-WifiDataProviderImplBase* CreateWin32WifiDataProviderStatic() {
-  MockWlanApi* wlan_api;
-  return Win32WifiDataProviderTest::CreateWin32WifiDataProvider(&wlan_api);
-}
-}  // namespace
-
-TEST_F(Win32WifiDataProviderTest, CreateDestroy) {
+TEST_F(GeolocationWin32WifiDataProviderTest, CreateDestroy) {
   // Test fixture members were SetUp correctly.
   EXPECT_EQ(&main_message_loop_, MessageLoop::current());
   EXPECT_TRUE(NULL != provider_.get());
   EXPECT_TRUE(NULL != wlan_api_);
 }
 
-TEST_F(Win32WifiDataProviderTest, StartThread) {
+TEST_F(GeolocationWin32WifiDataProviderTest, StartThread) {
   EXPECT_TRUE(provider_->StartDataProvider());
-  provider_.reset();  // Stop()s the thread.
+  provider_->StopDataProvider();
   SUCCEED();
 }
 
-TEST_F(Win32WifiDataProviderTest, DoAnEmptyScan) {
+TEST_F(GeolocationWin32WifiDataProviderTest, DoAnEmptyScan) {
   MessageLoopQuitListener quit_listener(&main_message_loop_);
   provider_->AddListener(&quit_listener);
   EXPECT_TRUE(provider_->StartDataProvider());
@@ -115,7 +113,7 @@ TEST_F(Win32WifiDataProviderTest, DoAnEmptyScan) {
   provider_->RemoveListener(&quit_listener);
 }
 
-TEST_F(Win32WifiDataProviderTest, DoScanWithResults) {
+TEST_F(GeolocationWin32WifiDataProviderTest, DoScanWithResults) {
   MessageLoopQuitListener quit_listener(&main_message_loop_);
   provider_->AddListener(&quit_listener);
   AccessPointData single_access_point;
@@ -138,9 +136,9 @@ TEST_F(Win32WifiDataProviderTest, DoScanWithResults) {
   provider_->RemoveListener(&quit_listener);
 }
 
-TEST_F(Win32WifiDataProviderTest, StartThreadViaDeviceDataProvider) {
+TEST_F(GeolocationWin32WifiDataProviderTest, StartThreadViaDeviceDataProvider) {
   MessageLoopQuitListener quit_listener(&main_message_loop_);
-  WifiDataProvider::SetFactory(CreateWin32WifiDataProviderStatic);
+  WifiDataProvider::SetFactory(CreateWin32WifiDataProvider);
   DeviceDataProvider<WifiData>::Register(&quit_listener);
   main_message_loop_.Run();
   DeviceDataProvider<WifiData>::Unregister(&quit_listener);
diff --git a/chrome/browser/geolocation/wifi_data_provider_win.cc b/chrome/browser/geolocation/wifi_data_provider_win.cc
index 60a1e9b..f103036 100644
--- a/chrome/browser/geolocation/wifi_data_provider_win.cc
+++ b/chrome/browser/geolocation/wifi_data_provider_win.cc
@@ -166,9 +166,9 @@ Win32WifiDataProvider::Win32WifiDataProvider()
 }
 
 Win32WifiDataProvider::~Win32WifiDataProvider() {
-  // Base class auto-stops the thread, however we need to do it here so our
-  // override of CleanUp still exists whilst the thread is shutting down.
-  Stop();
+  // 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";
 }
 
 void Win32WifiDataProvider::inject_mock_wlan_api(WlanApiInterface* wlan_api) {
@@ -185,10 +185,17 @@ void Win32WifiDataProvider::inject_mock_polling_policy(
 }
 
 bool Win32WifiDataProvider::StartDataProvider() {
-  return base::Thread::Start();
+  DCHECK(CalledOnClientThread());
+  return Start();
+}
+
+void Win32WifiDataProvider::StopDataProvider() {
+  DCHECK(CalledOnClientThread());
+  Stop();
 }
 
 bool Win32WifiDataProvider::GetData(WifiData *data) {
+  DCHECK(CalledOnClientThread());
   DCHECK(data);
   AutoLock lock(data_mutex_);
   *data = wifi_data_;
diff --git a/chrome/browser/geolocation/wifi_data_provider_win.h b/chrome/browser/geolocation/wifi_data_provider_win.h
index d5f28d8..62d0766 100644
--- a/chrome/browser/geolocation/wifi_data_provider_win.h
+++ b/chrome/browser/geolocation/wifi_data_provider_win.h
@@ -25,7 +25,6 @@ class Win32WifiDataProvider
   };
 
   Win32WifiDataProvider();
-  virtual ~Win32WifiDataProvider();
 
   // Takes ownership of wlan_api. Must be called before Start().
   void inject_mock_wlan_api(WlanApiInterface* wlan_api);
@@ -35,12 +34,14 @@ class Win32WifiDataProvider
 
   // WifiDataProviderImplBase implementation
   virtual bool StartDataProvider();
+  virtual void StopDataProvider();
   virtual bool GetData(WifiData *data);
 
  private:
+  virtual ~Win32WifiDataProvider();
+
   // Thread implementation
   virtual void Init();
-  // Called just after the message loop ends
   virtual void CleanUp();
 
   // Task which run in the child thread.