Makes use of EngineGeolocationFeature weak_ptr threadsafe.

blimp/engine/feature/geolocation/blimp_location_provider.cc

Index: blimp/engine/feature/geolocation/blimp_location_provider.cc
diff --git a/blimp/engine/feature/geolocation/blimp_location_provider.cc b/blimp/engine/feature/geolocation/blimp_location_provider.cc
index 9fd039774ab36b89f777a437f3f36fdd673ecb34..f8459e9f0493348347df9cd2dc49b7549ec6e904 100644
--- a/blimp/engine/feature/geolocation/blimp_location_provider.cc
+++ b/blimp/engine/feature/geolocation/blimp_location_provider.cc
@@ -7,14 +7,30 @@
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/memory/weak_ptr.h"
+#include "base/task_runner.h"
+#include "base/threading/thread_task_runner_handle.h"
 #include "device/geolocation/geoposition.h"
 
 namespace blimp {
 namespace engine {
+namespace {
+
+void InvokeGeopositionCallback(

[Wez, 2016/09/13 20:19:07]

Ah, I think we're going to need bind this to the lifetime of the
BlimpLocationProvider, after all.  If we don't then we can have:

1. [Thread A] Delegate calls InvokeGeopositionCallback.
2. [Thread B] Object to which |callback| is bound is deleted.
3. [Thread B] PostTask(callback) executes.

At #3 we explode!  This is a question of the expected semantics of
SetUpdateCallback() - does the caller have a reasonable expectation that their
callback won't be invoked after they have deleted the BlimpLocationProvider?  Or
what if they call SetUpdateCallback(null) - is it expected then that the old
callback may still be invoked (which would be the case with your current patch)?

[CJ, 2016/09/13 23:34:02]

|callback| is bound to |this|, the BlimpLocationProvider. I'm not following you.

[Wez, 2016/09/16 00:40:16]

Right, but Bind(callback, this) is just used to supply the LocationProvider to
the LocationUpdateCallback, to satisfy that API contract.

There is a separate issue of whether the calling code expects that if it does:

location_provider->SetUpdateCallback(callback);
delete location_provider;

then is it still possible that |callback| is invoked *after* the caller deleted
the LocationProvider?  With this current approach, using a static function to
bounce across threads, that is possible.

So do you instead need to bind the PostTask of the callback to the lifetime of
the LocationProvider in some way?

[CJ, 2016/09/16 22:55:53]

Done.

+    scoped_refptr<base::TaskRunner> task_runner,
+    BlimpLocationProvider::Delegate::GeopositionReceivedCallback callback,
+    const device::Geoposition& geoposition) {
+  // this calls task_runner->PostTas

[Wez, 2016/09/13 20:19:07]

?

[CJ, 2016/09/13 23:34:02]

Done.

+  task_runner->PostTask(FROM_HERE, base::Bind(callback, geoposition));
+}
+
+}  // namespace
 
 BlimpLocationProvider::BlimpLocationProvider(
-    base::WeakPtr<BlimpLocationProvider::Delegate> delegate)
-    : delegate_(delegate), is_started_(false) {}
+    base::WeakPtr<BlimpLocationProvider::Delegate> delegate,
+    scoped_refptr<base::SingleThreadTaskRunner> delegate_task_runner)
+    : delegate_(delegate),
+      delegate_task_runner_(delegate_task_runner),
+      is_started_(false) {}
 
 BlimpLocationProvider::~BlimpLocationProvider() {
   if (is_started_) {
@@ -23,25 +39,28 @@ BlimpLocationProvider::~BlimpLocationProvider() {
 }
 
 bool BlimpLocationProvider::StartProvider(bool high_accuracy) {
-  if (delegate_) {
-    if (high_accuracy) {
-      delegate_->RequestAccuracy(
-          GeolocationSetInterestLevelMessage::HIGH_ACCURACY);
-    } else {
-      delegate_->RequestAccuracy(
-          GeolocationSetInterestLevelMessage::LOW_ACCURACY);
-    }
-    is_started_ = true;
+  is_started_ = true;
+  if (high_accuracy) {
+    delegate_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&BlimpLocationProvider::Delegate::RequestAccuracy, delegate_,
+                   GeolocationSetInterestLevelMessage::HIGH_ACCURACY));

[Wez, 2016/09/13 20:19:07]

nit: You can merge this into a single PostTask with (high_accuracy ?
HIGH_ACCURACY : LOW_ACCURACY) as the parameter :)

[CJ, 2016/09/13 23:34:02]

Done.

+  } else {
+    delegate_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&BlimpLocationProvider::Delegate::RequestAccuracy, delegate_,
+                   GeolocationSetInterestLevelMessage::LOW_ACCURACY));
   }
   return is_started_;
 }
 
 void BlimpLocationProvider::StopProvider() {
   DCHECK(is_started_);
-  if (delegate_) {
-    delegate_->RequestAccuracy(GeolocationSetInterestLevelMessage::NO_INTEREST);
-    is_started_ = false;
-  }
+  delegate_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&BlimpLocationProvider::Delegate::RequestAccuracy, delegate_,
+                 GeolocationSetInterestLevelMessage::NO_INTEREST));
+  is_started_ = false;
 }
 
 const device::Geoposition& BlimpLocationProvider::GetPosition() {
@@ -50,16 +69,20 @@ const device::Geoposition& BlimpLocationProvider::GetPosition() {
 
 void BlimpLocationProvider::OnPermissionGranted() {
   DCHECK(is_started_);
-  if (delegate_) {
-    delegate_->OnPermissionGranted();
-  }
+  delegate_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&BlimpLocationProvider::Delegate::OnPermissionGranted,
+                 delegate_));
 }
 
 void BlimpLocationProvider::SetUpdateCallback(
     const LocationProviderUpdateCallback& callback) {
-  if (delegate_) {
-    delegate_->SetUpdateCallback(base::Bind(callback, base::Unretained(this)));
-  }
+  delegate_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&BlimpLocationProvider::Delegate::SetUpdateCallback, delegate_,
+                 base::Bind(&InvokeGeopositionCallback,
+                            base::ThreadTaskRunnerHandle::Get(),
+                            base::Bind(callback, this))));
 }
 
 }  // namespace engine