Fix data races on |SharedMemoryDataConsumerHandle::Context::notification_task_runner_|

content/child/shared_memory_data_consumer_handle.cc

 // Copyright 2015 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 "content/child/shared_memory_data_consumer_handle.h"
 
 #include <algorithm>
 #include <deque>
 
 #include "base/bind.h"
@@ skipping 142 matching lines @@
   }
   void set_is_two_phase_read_in_progress(bool b) {
     is_two_phase_read_in_progress_ = b;
   }
   base::Lock& lock() { return lock_; }
 
  private:
   void NotifyInternal(bool repost) {
     // Note that this function is not protected by |lock_|.
 
-    auto runner = notification_task_runner_;
+    scoped_refptr<base::SingleThreadTaskRunner> runner;
+    {
+      base::AutoLock lock(lock_);
+      runner = notification_task_runner_;
+    }
     if (!runner)
       return;
 
     if (runner->BelongsToCurrentThread()) {
       // It is safe to access member variables without lock because |client_|
       // is bound to the current thread.
       if (client_)
         client_->didGetReadable();
       return;
     }
@@ skipping 92 matching lines @@
 
   if (needs_notification) {
     // We CAN issue the notification synchronously if the associated reader
     // lives in this thread, because this function cannot be called in the
     // client's callback.
     context_->Notify();
   }
 }
 
 void SharedMemoryDataConsumerHandle::Writer::Close() {
-  bool needs_notification = false;
-
-  {
-    base::AutoLock lock(context_->lock());
-    if (context_->result() == Ok) {
-      context_->set_result(Done);
-      context_->ResetOnReaderDetached();
-      needs_notification = context_->IsEmpty();
+  base::AutoLock lock(context_->lock());
+  if (context_->result() == Ok) {
+    context_->set_result(Done);
+    context_->ResetOnReaderDetached();
+    if (context_->IsEmpty()) {
+      // We cannot issue the notification synchronously because this function
+      // can be called in the client's callback.
+      context_->PostNotify();

[kinuko, 2015/07/16 03:28:44]

If some methods of Context are assumed to be called with a lock held while
others are not, it'd be better to clearly document about the lock condition and
to have AssertAquired in each method.  The current usage of the context lock
feels a bit error prone.

[hiroshige, 2015/07/16 05:35:58]

Added comments/assertions in Patch Set 4.

     }
   }
-  if (needs_notification) {
-    // We cannot issue the notification synchronously because this function can
-    // be called in the client's callback.
-    context_->PostNotify();
-  }
 }
 
 void SharedMemoryDataConsumerHandle::Writer::Fail() {
-  bool needs_notification = false;
-  {
-    base::AutoLock lock(context_->lock());
-    if (context_->result() == Ok) {
-      // TODO(yhirano): Use an appropriate error code other than
-      // UnexpectedError.
-      context_->set_result(UnexpectedError);
+  base::AutoLock lock(context_->lock());
+  if (context_->result() == Ok) {
+    // TODO(yhirano): Use an appropriate error code other than
+    // UnexpectedError.
+    context_->set_result(UnexpectedError);
 
-      if (context_->is_two_phase_read_in_progress()) {
-        // If we are in two-phase read session, we cannot discard the data. We
-        // will clear the queue at the end of the session.
-      } else {
-        context_->ClearQueue();
-      }
+    if (context_->is_two_phase_read_in_progress()) {
+      // If we are in two-phase read session, we cannot discard the data. We
+      // will clear the queue at the end of the session.
+    } else {
+      context_->ClearQueue();
+    }
 
-      context_->ResetOnReaderDetached();
-      needs_notification = true;
-    }
-  }
-  if (needs_notification) {
+    context_->ResetOnReaderDetached();
     // We cannot issue the notification synchronously because this function can
     // be called in the client's callback.
     context_->PostNotify();
   }
 }
 
 SharedMemoryDataConsumerHandle::ReaderImpl::ReaderImpl(
     scoped_refptr<Context> context,
     Client* client)
     : context_(context) {
@@ skipping 114 matching lines @@
 SharedMemoryDataConsumerHandle::ReaderImpl*
 SharedMemoryDataConsumerHandle::obtainReaderInternal(Client* client) {
   return new ReaderImpl(context_, client);
 }
 
 const char* SharedMemoryDataConsumerHandle::debugName() const {
   return "SharedMemoryDataConsumerHandle";
 }
 
 }  // namespace content