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

content/child/shared_memory_data_consumer_handle.cc

Index: content/child/shared_memory_data_consumer_handle.cc
diff --git a/content/child/shared_memory_data_consumer_handle.cc b/content/child/shared_memory_data_consumer_handle.cc
index f11435ada87c22572379c59ce9b80f2e50c85b44..d73bd529ba552a782f70454e120eb96041ae27a8 100644
--- a/content/child/shared_memory_data_consumer_handle.cc
+++ b/content/child/shared_memory_data_consumer_handle.cc
@@ -160,7 +160,11 @@ class SharedMemoryDataConsumerHandle::Context final
   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;
 
@@ -273,44 +277,33 @@ void SharedMemoryDataConsumerHandle::Writer::AddData(
 }
 
 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);
-
-      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();
-      }
+  base::AutoLock lock(context_->lock());
+  if (context_->result() == Ok) {
+    // TODO(yhirano): Use an appropriate error code other than
+    // UnexpectedError.
+    context_->set_result(UnexpectedError);
 
-      context_->ResetOnReaderDetached();
-      needs_notification = true;
+    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 (needs_notification) {
+
+    context_->ResetOnReaderDetached();
     // We cannot issue the notification synchronously because this function can
     // be called in the client's callback.
     context_->PostNotify();