[sensors](Linux) Fix tsan data race in sensor reader

This CL fixes a tsan data race, which is caused by calling StopFetchingData
from different threads. It must not be allowed. Use the same thread by using
a PostTask.

BUG=673760
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_tsan_rel_ng

Review-Url: https://codereview.chromium.org/2569763004
Cr-Commit-Position: refs/heads/master@{#439798}

build/sanitizers/tsan_suppressions.cc

@@ -270,9 +270,6 @@ char kTSanDefaultSuppressions[] =
 // http://crbug.com/587199
 "race:base::TimerTest_OneShotTimer_CustomTaskRunner_Test::TestBody\n"
 
-// http://crbug.com/673760
-"race:device::PollingSensorReader::StopFetchingData\n"
-
 // End of suppressions.
 ;  // Please keep this semicolon.
 

device/generic_sensor/platform_sensor_linux.cc

@@ -29,12 +29,16 @@ PlatformSensorLinux::PlatformSensorLinux(
       default_configuration_(
           PlatformSensorConfiguration(sensor_device->device_frequency)),
       reporting_mode_(sensor_device->reporting_mode),
+      polling_thread_task_runner_(std::move(polling_thread_task_runner)),
       weak_factory_(this) {
-  sensor_reader_ =
-      SensorReader::Create(sensor_device, this, polling_thread_task_runner);
+  sensor_reader_ = SensorReader::Create(
+      sensor_device, weak_factory_.GetWeakPtr(), task_runner_);
 }
 
-PlatformSensorLinux::~PlatformSensorLinux() = default;
+PlatformSensorLinux::~PlatformSensorLinux() {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  polling_thread_task_runner_->DeleteSoon(FROM_HERE, sensor_reader_.release());
+}
 
 mojom::ReportingMode PlatformSensorLinux::GetReportingMode() {
   DCHECK(task_runner_->BelongsToCurrentThread());
@@ -62,15 +66,18 @@ void PlatformSensorLinux::NotifyPlatformSensorError() {
 bool PlatformSensorLinux::StartSensor(
     const PlatformSensorConfiguration& configuration) {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  if (!sensor_reader_)
-    return false;
-  return sensor_reader_->StartFetchingData(configuration);
+  polling_thread_task_runner_->PostTask(
+      FROM_HERE,
+      base::Bind(&SensorReader::StartFetchingData,
+                 base::Unretained(sensor_reader_.get()), configuration));
+  return true;
 }
 
 void PlatformSensorLinux::StopSensor() {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  DCHECK(sensor_reader_);
-  sensor_reader_->StopFetchingData();
+  polling_thread_task_runner_->PostTask(
+      FROM_HERE, base::Bind(&SensorReader::StopFetchingData,
+                            base::Unretained(sensor_reader_.get())));
 }
 
 bool PlatformSensorLinux::CheckSensorConfiguration(

device/generic_sensor/platform_sensor_linux.h

@@ -45,6 +45,8 @@ class PlatformSensorLinux : public PlatformSensor {
   const PlatformSensorConfiguration default_configuration_;
   const mojom::ReportingMode reporting_mode_;
 
+  scoped_refptr<base::SingleThreadTaskRunner> polling_thread_task_runner_;
+
   // A sensor reader that reads values from sensor files
   // and stores them to a SensorReading structure.
   std::unique_ptr<SensorReader> sensor_reader_;

device/generic_sensor/platform_sensor_provider_linux.cc

@@ -5,9 +5,11 @@
 #include "device/generic_sensor/platform_sensor_provider_linux.h"
 
 #include "base/memory/singleton.h"
+#include "base/task_runner_util.h"
 #include "base/threading/thread.h"
 #include "device/generic_sensor/linux/sensor_data_linux.h"
 #include "device/generic_sensor/platform_sensor_linux.h"
+#include "device/generic_sensor/platform_sensor_reader_linux.h"
 
 namespace device {
 
@@ -59,8 +61,9 @@ void PlatformSensorProviderLinux::SensorDeviceFound(
     mojom::SensorType type,
     mojo::ScopedSharedBufferMapping mapping,
     const PlatformSensorProviderBase::CreateSensorCallback& callback,
-    SensorInfoLinux* sensor_device) {
+    const SensorInfoLinux* sensor_device) {
   DCHECK(CalledOnValidThread());
+  DCHECK(sensor_device);
 
   if (!StartPollingThread()) {
     callback.Run(nullptr);

device/generic_sensor/platform_sensor_provider_linux.h

@@ -58,7 +58,7 @@ class DEVICE_GENERIC_SENSOR_EXPORT PlatformSensorProviderLinux
       mojom::SensorType type,
       mojo::ScopedSharedBufferMapping mapping,
       const PlatformSensorProviderBase::CreateSensorCallback& callback,
-      SensorInfoLinux* sensor_device);
+      const SensorInfoLinux* sensor_device);
 
   bool StartPollingThread();
 

device/generic_sensor/platform_sensor_reader_linux.cc

@@ -17,14 +17,13 @@ namespace device {
 
 class PollingSensorReader : public SensorReader {
  public:
-  PollingSensorReader(
-      const SensorInfoLinux* sensor_device,
-      PlatformSensorLinux* sensor,
-      scoped_refptr<base::SingleThreadTaskRunner> polling_task_runner);
+  PollingSensorReader(const SensorInfoLinux* sensor_device,
+                      base::WeakPtr<PlatformSensorLinux> sensor,
+                      scoped_refptr<base::SingleThreadTaskRunner> task_runner);
   ~PollingSensorReader() override;
 
   // SensorReader implements:
-  bool StartFetchingData(
+  void StartFetchingData(
       const PlatformSensorConfiguration& configuration) override;
   void StopFetchingData() override;
 
@@ -47,59 +46,53 @@ class PollingSensorReader : public SensorReader {
   // Used to apply scalings and invert signs if needed.
   const SensorPathsLinux::ReaderFunctor apply_scaling_func_;
 
-  // Owned pointer to a timer. Will be deleted on a polling thread once
-  // destructor is called.
-  base::RepeatingTimer* timer_;
-
-  base::WeakPtrFactory<PollingSensorReader> weak_factory_;
+  // Repeating timer for data polling.
+  base::RepeatingTimer timer_;
 
   DISALLOW_COPY_AND_ASSIGN(PollingSensorReader);
 };
 
 PollingSensorReader::PollingSensorReader(
     const SensorInfoLinux* sensor_device,
-    PlatformSensorLinux* sensor,
-    scoped_refptr<base::SingleThreadTaskRunner> polling_task_runner)
-    : SensorReader(sensor, polling_task_runner),
+    base::WeakPtr<PlatformSensorLinux> sensor,
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner)
+    : SensorReader(sensor, std::move(task_runner)),
       sensor_file_paths_(sensor_device->device_reading_files),
       scaling_value_(sensor_device->device_scaling_value),
       offset_value_(sensor_device->device_offset_value),
-      apply_scaling_func_(sensor_device->apply_scaling_func),
-      timer_(new base::RepeatingTimer()),
-      weak_factory_(this) {}
+      apply_scaling_func_(sensor_device->apply_scaling_func) {}
 
 PollingSensorReader::~PollingSensorReader() {
-  polling_task_runner_->DeleteSoon(FROM_HERE, timer_);
+  DCHECK(thread_checker_.CalledOnValidThread());
 }
 
-bool PollingSensorReader::StartFetchingData(
+void PollingSensorReader::StartFetchingData(
     const PlatformSensorConfiguration& configuration) {
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (is_reading_active_)
     StopFetchingData();
-
-  return polling_task_runner_->PostTask(
-      FROM_HERE, base::Bind(&PollingSensorReader::InitializeTimer,
-                            weak_factory_.GetWeakPtr(), configuration));
+  InitializeTimer(configuration);
 }
 
 void PollingSensorReader::StopFetchingData() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   is_reading_active_ = false;
-  timer_->Stop();
+  timer_.Stop();
 }
 
 void PollingSensorReader::InitializeTimer(
     const PlatformSensorConfiguration& configuration) {
-  DCHECK(polling_task_runner_->BelongsToCurrentThread());
-  timer_->Start(FROM_HERE, base::TimeDelta::FromMicroseconds(
-                               base::Time::kMicrosecondsPerSecond /
-                               configuration.frequency()),
-                this, &PollingSensorReader::PollForData);
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(!is_reading_active_);
+  timer_.Start(FROM_HERE, base::TimeDelta::FromMicroseconds(
+                              base::Time::kMicrosecondsPerSecond /
+                              configuration.frequency()),
+               this, &PollingSensorReader::PollForData);
   is_reading_active_ = true;
 }
 
 void PollingSensorReader::PollForData() {
-  DCHECK(polling_task_runner_->BelongsToCurrentThread());
-  base::ThreadRestrictions::AssertIOAllowed();
+  DCHECK(thread_checker_.CalledOnValidThread());
 
   SensorReading readings;
   DCHECK_LE(sensor_file_paths_.size(), arraysize(readings.values));
@@ -127,36 +120,41 @@ void PollingSensorReader::PollForData() {
   if (is_reading_active_) {
     task_runner_->PostTask(
         FROM_HERE, base::Bind(&PlatformSensorLinux::UpdatePlatformSensorReading,
-                              base::Unretained(sensor_), readings));
+                              sensor_, readings));
   }
 }
 
 // static
 std::unique_ptr<SensorReader> SensorReader::Create(
     const SensorInfoLinux* sensor_device,
-    PlatformSensorLinux* sensor,
-    scoped_refptr<base::SingleThreadTaskRunner> polling_thread_task_runner) {
+    base::WeakPtr<PlatformSensorLinux> sensor,
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
+  base::ThreadRestrictions::AssertIOAllowed();
   // TODO(maksims): implement triggered reading. At the moment,
   // only polling read is supported.
   return base::MakeUnique<PollingSensorReader>(sensor_device, sensor,
-                                               polling_thread_task_runner);
+                                               std::move(task_runner));
 }
 
 SensorReader::SensorReader(
-    PlatformSensorLinux* sensor,
-    scoped_refptr<base::SingleThreadTaskRunner> polling_task_runner)
+    base::WeakPtr<PlatformSensorLinux> sensor,
+    scoped_refptr<base::SingleThreadTaskRunner> task_runner)
     : sensor_(sensor),
-      polling_task_runner_(polling_task_runner),
-      task_runner_(base::ThreadTaskRunnerHandle::Get()),
-      is_reading_active_(false) {}
+      task_runner_(std::move(task_runner)),
+      is_reading_active_(false) {
+  thread_checker_.DetachFromThread();
+}
 
-SensorReader::~SensorReader() = default;
+SensorReader::~SensorReader() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+}
 
 void SensorReader::NotifyReadError() {
+  DCHECK(thread_checker_.CalledOnValidThread());
   if (is_reading_active_) {
     task_runner_->PostTask(
-        FROM_HERE, base::Bind(&PlatformSensorLinux::NotifyPlatformSensorError,
-                              base::Unretained(sensor_)));
+        FROM_HERE,
+        base::Bind(&PlatformSensorLinux::NotifyPlatformSensorError, sensor_));
   }
 }
 

device/generic_sensor/platform_sensor_reader_linux.h

@@ -5,7 +5,11 @@
 #ifndef DEVICE_GENERIC_SENSOR_PLATFORM_SENSOR_READER_LINUX_H_
 #define DEVICE_GENERIC_SENSOR_PLATFORM_SENSOR_READER_LINUX_H_
 
+#include "base/callback.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/weak_ptr.h"
+#include "base/threading/thread_checker.h"
+#include "device/generic_sensor/generic_sensor_export.h"
 
 namespace base {
 class SingleThreadTaskRunner;
@@ -13,45 +17,49 @@ class SingleThreadTaskRunner;
 
 namespace device {
 
-class PlatformSensorLinux;
 class PlatformSensorConfiguration;
+class PlatformSensorLinux;
 struct SensorInfoLinux;
 
 // A generic reader class that can be implemented with two different strategies:
-// polling and on trigger.
+// polling and on trigger. All methods are not thread-safe and must be called
+// on a polling thread that allows I/O.
 class SensorReader {
  public:
   // Creates a new instance of SensorReader. At the moment, only polling
   // reader is supported.
   static std::unique_ptr<SensorReader> Create(
       const SensorInfoLinux* sensor_device,
-      PlatformSensorLinux* sensor,
-      scoped_refptr<base::SingleThreadTaskRunner> polling_thread_task_runner);
+      base::WeakPtr<PlatformSensorLinux> sensor,
+      scoped_refptr<base::SingleThreadTaskRunner> task_runner);
 
   virtual ~SensorReader();
 
   // Starts fetching data based on strategy this reader has chosen.
-  // Only polling strategy is supported at the moment. Thread safe.
-  virtual bool StartFetchingData(
+  // Only polling strategy is supported at the moment.
+  virtual void StartFetchingData(
       const PlatformSensorConfiguration& configuration) = 0;
 
-  // Stops fetching data. Thread safe.
+  // Stops fetching data.
   virtual void StopFetchingData() = 0;
 
  protected:
-  SensorReader(PlatformSensorLinux* sensor,
-               scoped_refptr<base::SingleThreadTaskRunner> polling_task_runner);
+  SensorReader(base::WeakPtr<PlatformSensorLinux> sensor,
+               scoped_refptr<base::SingleThreadTaskRunner> task_runner);
 
   // Notifies |sensor_| about an error.
   void NotifyReadError();
 
-  // Non-owned pointer to a sensor that created this reader.
-  PlatformSensorLinux* sensor_;
+  // In builds with DCHECK enabled checks that methods of this
+  // and derived classes are called on a right thread.
+  base::ThreadChecker thread_checker_;
 
-  // A task runner that is used to poll data.
-  scoped_refptr<base::SingleThreadTaskRunner> polling_task_runner_;
+  // A sensor that this reader is owned by and notifies about errors and
+  // readings to.
+  base::WeakPtr<PlatformSensorLinux> sensor_;
 
-  // A task runner that belongs to a thread this reader is created on.
+  // A task runner that is used to report about new readings and errors
+  // to a |sensor_|.
   scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
 
   // Indicates if reading is active.