Fix race condition in DataFetcherSharedMemoryBase

A race condition can occur when a polling thread tries to obtain a pointer to the shared memory via a std::map, while that map is modified by the main thread.
This patch implements a fix such that the polling thread does not have direct access to the std::map where the shared memory pointers are stored. Instead the pointer is passed as an argument of the Start() method.

This patch fixes the DataFetcherSharedMemoryBaseTest.DoesPollMotionAndOrientation test on linux tsan.

BUG=284959

Review URL: https://chromiumcodereview.appspot.com/23441047

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

content/browser/device_orientation/data_fetcher_shared_memory.h

@@ -7,10 +7,12 @@
 
 #include "content/browser/device_orientation/data_fetcher_shared_memory_base.h"
 
-#if defined(OS_MACOSX)
+#if !defined(OS_ANDROID)
 #include "content/common/device_motion_hardware_buffer.h"
 #include "content/common/device_orientation/device_orientation_hardware_buffer.h"
+#endif
 
+#if defined(OS_MACOSX)
 class SuddenMotionSensor;
 #endif
 
@@ -24,16 +26,17 @@ class CONTENT_EXPORT DataFetcherSharedMemory
   virtual ~DataFetcherSharedMemory();
 
  private:
-
-  virtual bool Start(ConsumerType consumer_type) OVERRIDE;
+  virtual bool Start(ConsumerType consumer_type, void* buffer) OVERRIDE;
   virtual bool Stop(ConsumerType consumer_type) OVERRIDE;
 
+#if !defined(OS_ANDROID)
+  DeviceMotionHardwareBuffer* motion_buffer_;
+  DeviceOrientationHardwareBuffer* orientation_buffer_;
+#endif
 #if defined(OS_MACOSX)
   virtual void Fetch(unsigned consumer_bitmask) OVERRIDE;
   virtual bool IsPolling() const OVERRIDE;
 
-  DeviceMotionHardwareBuffer* motion_buffer_;
-  DeviceOrientationHardwareBuffer* orientation_buffer_;
   scoped_ptr<SuddenMotionSensor> sudden_motion_sensor_;
 #endif
 

content/browser/device_orientation/data_fetcher_shared_memory_android.cc

@@ -17,8 +17,7 @@ DataFetcherSharedMemory::DataFetcherSharedMemory() {
 DataFetcherSharedMemory::~DataFetcherSharedMemory() {
 }
 
-bool DataFetcherSharedMemory::Start(ConsumerType consumer_type) {
-  void* buffer = GetSharedMemoryBuffer(consumer_type);
+bool DataFetcherSharedMemory::Start(ConsumerType consumer_type, void* buffer) {
   DCHECK(buffer);
 
   switch (consumer_type) {

content/browser/device_orientation/data_fetcher_shared_memory_base.cc

@@ -36,7 +36,7 @@ class DataFetcherSharedMemoryBase::PollingThread : public base::Thread {
   PollingThread(const char* name, DataFetcherSharedMemoryBase* fetcher);
   virtual ~PollingThread();
 
-  void AddConsumer(ConsumerType consumer_type);
+  void AddConsumer(ConsumerType consumer_type, void* buffer);
   void RemoveConsumer(ConsumerType consumer_type);
 
   unsigned GetConsumersBitmask() const { return consumers_bitmask_; }
@@ -63,9 +63,9 @@ DataFetcherSharedMemoryBase::PollingThread::~PollingThread() {
 }
 
 void DataFetcherSharedMemoryBase::PollingThread::AddConsumer(
-    ConsumerType consumer_type) {
+    ConsumerType consumer_type, void* buffer) {
   DCHECK(fetcher_);
-  if (!fetcher_->Start(consumer_type))
+  if (!fetcher_->Start(consumer_type, buffer))
     return;
 
   consumers_bitmask_ |= consumer_type;
@@ -119,7 +119,8 @@ bool DataFetcherSharedMemoryBase::StartFetchingDeviceData(
   if (started_consumers_ & consumer_type)
     return true;
 
-  if (!GetSharedMemoryBuffer(consumer_type))
+  void* buffer = GetSharedMemoryBuffer(consumer_type);
+  if (!buffer)
     return false;
 
   if (IsPolling()) {
@@ -129,9 +130,9 @@ bool DataFetcherSharedMemoryBase::StartFetchingDeviceData(
         FROM_HERE,
         base::Bind(&PollingThread::AddConsumer,
                    base::Unretained(polling_thread_.get()),
-                   consumer_type));
+                   consumer_type, buffer));
   } else {
-    if (!Start(consumer_type))
+    if (!Start(consumer_type, buffer))
       return false;
   }
 

content/browser/device_orientation/data_fetcher_shared_memory_base.h

@@ -45,8 +45,6 @@ class CONTENT_EXPORT DataFetcherSharedMemoryBase {
   DataFetcherSharedMemoryBase();
   virtual ~DataFetcherSharedMemoryBase();
 
-  void* GetSharedMemoryBuffer(ConsumerType consumer_type);
-
   // Returns the message loop of the polling thread.
   // Returns NULL if there is no polling thread.
   base::MessageLoop* GetPollingMessageLoop() const;
@@ -62,12 +60,13 @@ class CONTENT_EXPORT DataFetcherSharedMemoryBase {
   // Start() method should call InitSharedMemoryBuffer() to get the shared
   // memory pointer. If IsPolling() is true both Start() and Stop() methods
   // are called from the |polling_thread_|.
-  virtual bool Start(ConsumerType consumer_type) = 0;
+  virtual bool Start(ConsumerType consumer_type, void* buffer) = 0;
   virtual bool Stop(ConsumerType consumer_type) = 0;
 
  private:
   bool InitAndStartPollingThreadIfNecessary();
   base::SharedMemory* GetSharedMemory(ConsumerType consumer_type);
+  void* GetSharedMemoryBuffer(ConsumerType consumer_type);
 
   unsigned started_consumers_;
 

content/browser/device_orientation/data_fetcher_shared_memory_base_unittest.cc

@@ -33,15 +33,16 @@ class FakeDataFetcher : public DataFetcherSharedMemoryBase {
   }
   virtual ~FakeDataFetcher() { }
 
-  bool Init(ConsumerType consumer_type) {
+  bool Init(ConsumerType consumer_type, void* buffer) {
+    EXPECT_TRUE(buffer);
+
     switch (consumer_type) {
       case CONSUMER_TYPE_MOTION:
-        motion_buffer_ = static_cast<DeviceMotionHardwareBuffer*>(
-            GetSharedMemoryBuffer(consumer_type));
+        motion_buffer_ = static_cast<DeviceMotionHardwareBuffer*>(buffer);
         break;
       case CONSUMER_TYPE_ORIENTATION:
-        orientation_buffer_ = static_cast<DeviceOrientationHardwareBuffer*>(
-            GetSharedMemoryBuffer(consumer_type));
+        orientation_buffer_ =
+            static_cast<DeviceOrientationHardwareBuffer*>(buffer);
         break;
       default:
         return false;
@@ -128,12 +129,12 @@ class FakeNonPollingDataFetcher : public FakeDataFetcher {
   FakeNonPollingDataFetcher() { }
   virtual ~FakeNonPollingDataFetcher() { }
 
-  virtual bool Start(ConsumerType consumer_type) OVERRIDE {
+  virtual bool Start(ConsumerType consumer_type, void* buffer) OVERRIDE {
     base::SharedMemoryHandle handle = GetSharedMemoryHandleForProcess(
         consumer_type, base::GetCurrentProcessHandle());
     EXPECT_TRUE(base::SharedMemory::IsHandleValid(handle));
 
-    Init(consumer_type);
+    Init(consumer_type, buffer);
     switch (consumer_type) {
       case CONSUMER_TYPE_MOTION:
         UpdateMotion();
@@ -182,13 +183,13 @@ class FakePollingDataFetcher : public FakeDataFetcher {
   FakePollingDataFetcher() { }
   virtual ~FakePollingDataFetcher() { }
 
-  virtual bool Start(ConsumerType consumer_type) OVERRIDE {
+  virtual bool Start(ConsumerType consumer_type, void* buffer) OVERRIDE {
     EXPECT_TRUE(base::MessageLoop::current() == GetPollingMessageLoop());
     base::SharedMemoryHandle handle = GetSharedMemoryHandleForProcess(
         consumer_type, base::GetCurrentProcessHandle());
     EXPECT_TRUE(base::SharedMemory::IsHandleValid(handle));
 
-    Init(consumer_type);
+    Init(consumer_type, buffer);
     switch (consumer_type) {
       case CONSUMER_TYPE_MOTION:
         start_motion_.Signal();

content/browser/device_orientation/data_fetcher_shared_memory_default.cc

@@ -12,7 +12,8 @@ namespace {
 
 static bool SetMotionBuffer(content::DeviceMotionHardwareBuffer* buffer,
     bool enabled) {
-  DCHECK(buffer);
+  if (!buffer)
+    return false;
   buffer->seqlock.WriteBegin();
   buffer->data.allAvailableSensorsAreActive = enabled;
   buffer->seqlock.WriteEnd();
@@ -29,14 +30,13 @@ DataFetcherSharedMemory::DataFetcherSharedMemory() {
 DataFetcherSharedMemory::~DataFetcherSharedMemory() {
 }
 
-bool DataFetcherSharedMemory::Start(ConsumerType consumer_type) {
-  void* buffer = GetSharedMemoryBuffer(consumer_type);
+bool DataFetcherSharedMemory::Start(ConsumerType consumer_type, void* buffer) {
   DCHECK(buffer);
 
   switch (consumer_type) {
     case CONSUMER_TYPE_MOTION:
-      return SetMotionBuffer(
-          static_cast<DeviceMotionHardwareBuffer*>(buffer), true);
+      motion_buffer_ = static_cast<DeviceMotionHardwareBuffer*>(buffer);
+      return SetMotionBuffer(motion_buffer_, true);
     case CONSUMER_TYPE_ORIENTATION:
       NOTIMPLEMENTED();
       break;
@@ -47,13 +47,10 @@ bool DataFetcherSharedMemory::Start(ConsumerType consumer_type) {
 }
 
 bool DataFetcherSharedMemory::Stop(ConsumerType consumer_type) {
-  void* buffer = GetSharedMemoryBuffer(consumer_type);
-  DCHECK(buffer);
 
   switch (consumer_type) {
     case CONSUMER_TYPE_MOTION:
-      return SetMotionBuffer(
-          static_cast<DeviceMotionHardwareBuffer*>(buffer), false);
+      return SetMotionBuffer(motion_buffer_, false);
     case CONSUMER_TYPE_ORIENTATION:
       NOTIMPLEMENTED();
       break;

content/browser/device_orientation/data_fetcher_shared_memory_mac.cc

@@ -115,10 +115,8 @@ bool DataFetcherSharedMemory::IsPolling() const {
   return true;
 }
 
-bool DataFetcherSharedMemory::Start(ConsumerType consumer_type) {
+bool DataFetcherSharedMemory::Start(ConsumerType consumer_type, void* buffer) {
   DCHECK(base::MessageLoop::current() == GetPollingMessageLoop());
-
-  void* buffer = GetSharedMemoryBuffer(consumer_type);
   DCHECK(buffer);
 
   switch (consumer_type) {

tools/valgrind/tsan/suppressions.txt

@@ -1129,17 +1129,3 @@
   fun:content::UtilityThreadImpl::UtilityThreadImpl
   fun:content::UtilityMainThread::InitInternal
 }
-{
-   bug_284959
-   ThreadSanitizer:Race
-   fun:std::_Rb_tree_insert_and_rebalance
-   fun:std::_Rb_tree::_M_insert_
-   fun:std::_Rb_tree::_M_insert_unique_
-   fun:std::map::insert
-   fun:std::map::operator[]
-   fun:content::DataFetcherSharedMemoryBase::GetSharedMemory
-   fun:content::DataFetcherSharedMemoryBase::GetSharedMemoryBuffer
-   fun:content::DataFetcherSharedMemoryBase::StartFetchingDeviceData
-   fun:content::::DataFetcherSharedMemoryBaseTest_DoesPollMotionAndOrientation_Test::TestBody
-   fun:testing::internal::HandleSehExceptionsInMethodIfSupported
-}