[Sensors] Improvements in shared buffer managing

device/generic_sensor/platform_sensor.cc

 // Copyright 2016 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/generic_sensor/platform_sensor.h"
 
 #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) {}
 
 PlatformSensor::~PlatformSensor() {
   provider_->RemoveSensor(GetType());
 }
 
 mojom::SensorType PlatformSensor::GetType() const {
   return type_;
@@ skipping 45 matching lines @@
 void PlatformSensor::RemoveClient(Client* client) {
   DCHECK(client);
   clients_.RemoveObserver(client);
   auto client_entry = config_map_.find(client);
   if (client_entry != config_map_.end()) {
     config_map_.erase(client_entry);
     UpdateSensorInternal(config_map_);
   }
 }
 
+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_);
   Client* client;
   while ((client = it.GetNext()) != nullptr) {
     if (!client->IsNotificationSuspended())
       client->OnSensorReadingChanged();
   }
 }
 
@@ skipping 17 matching lines @@
 
   if (!optimal_configuration) {
     StopSensor();
     return true;
   }
 
   return StartSensor(*optimal_configuration);
 }
 
 }  // namespace device