[Sensors] Improvements in shared buffer managing

device/generic_sensor/platform_sensor.cc

Index: device/generic_sensor/platform_sensor.cc
diff --git a/device/generic_sensor/platform_sensor.cc b/device/generic_sensor/platform_sensor.cc
index bca1fae37f9c6f07d63bd4c62d14b50c622839be..edfcf37a1a392f1eda43d5ccc59d66a0808dedfb 100644
--- a/device/generic_sensor/platform_sensor.cc
+++ b/device/generic_sensor/platform_sensor.cc
@@ -6,15 +6,19 @@
 
 #include <utility>
 
+#include "base/threading/thread_task_runner_handle.h"
 #include "device/generic_sensor/platform_sensor_provider.h"
 #include "device/generic_sensor/public/cpp/platform_sensor_configuration.h"
 
 namespace device {
 
-PlatformSensor::PlatformSensor(mojom::SensorType type,
-                               mojo::ScopedSharedBufferMapping mapping,
-                               PlatformSensorProvider* provider)
-    : shared_buffer_mapping_(std::move(mapping)),
+PlatformSensor::PlatformSensor(
+    mojom::SensorType type,
+    mojo::ScopedSharedBufferMapping mapping,
+    PlatformSensorProvider* provider,
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner)
+    : task_runner_(task_runner),
+      shared_buffer_mapping_(std::move(mapping)),
       type_(type),
       provider_(provider),
       weak_factory_(this) {}
@@ -80,6 +84,21 @@ void PlatformSensor::RemoveClient(Client* client) {
   }
 }
 
+void PlatformSensor::UpdateSensorReading(const SensorReading& reading,
+                                         bool notify_clients) {
+  ReadingBuffer* buffer =
+      static_cast<ReadingBuffer*>(shared_buffer_mapping_.get());
+  auto& seqlock = buffer->seqlock.value();
+  seqlock.WriteBegin();
+  buffer->reading = reading;
+  seqlock.WriteEnd();
+
+  if (notify_clients)
+    task_runner_->PostTask(

[Ken Rockot(use gerrit already), 2016/10/10 16:02:08]

I believe this is incorrect. The PlatformSensor instance lives on thread A, and
presumably |task_runner_| can run tasks on some arbitrary other thread B.

|clients_| is owned by PlatformSensor and is not thread-safe, yet you're
accessing it on both threads.

WeakPtrs vended by WeakFactory must be dereferenced and invalidated on the same
thread. Since you only invalidate them implicitly in the destructor (on thread
A) it is not OK to dereference them on thread B.

[Reilly Grant (use Gerrit), 2016/10/10 16:30:41]

So far it looks like |task_runner_| is always the same thread the PlatformSensor
was constructed on but it took some digging to convince myself of that. If you
want to cache the result of base::ThreadTaskRunnerHandle::Get() then I suggest
doing it in the base PlatformSensor constructor instead of making readers check
that that is always what is being passed in in every subclass.

[Mikhail, 2016/10/10 18:29:19]

Right, |task_runner_| is PlatformSensor's native thread. However on many
platforms (Android, Mac, Linux) sensor data is obtained on a different thread,
so it's useful to have |task_runner_| for notifications from there.
I'll switch to initializing it in constructor.

+        FROM_HERE, base::Bind(&PlatformSensor::NotifySensorReadingChanged,
+                              weak_factory_.GetWeakPtr()));
+}
+
 void PlatformSensor::NotifySensorReadingChanged() {
   using ClientsList = decltype(clients_);
   ClientsList::Iterator it(&clients_);