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 86 matching lines @@
       notification_task_runner_->PostTask(
           FROM_HERE, base::Bind(&Context::NotifyInternal, this, false));
     }
   }
   void ReleaseReaderLock() {
     DCHECK(notification_task_runner_);
     notification_task_runner_ = nullptr;
     client_ = nullptr;
   }
   void PostNotify() {
-    auto runner = notification_task_runner_;
+    scoped_refptr<base::SingleThreadTaskRunner> runner;
+    {
+      base::AutoLock lock(lock_);
+      runner = notification_task_runner_;
+    }
     if (!runner)
       return;
     // We don't re-post the task when the runner changes while waiting for
     // this task because in this case a new reader is obtained and
     // notification is already done at the reader creation time if necessary.
     runner->PostTask(FROM_HERE,
                      base::Bind(&Context::NotifyInternal, this, false));
   }
   void Notify() { NotifyInternal(true); }
   // This function doesn't work in the destructor if |on_reader_detached_| is
@@ skipping 35 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 102 matching lines @@
   bool needs_notification = false;
 
   {
     base::AutoLock lock(context_->lock());
     if (context_->result() == Ok) {
       context_->set_result(Done);
       context_->ResetOnReaderDetached();
       needs_notification = context_->IsEmpty();
     }
   }
   if (needs_notification) {

[yhirano, 2015/07/15 13:30:09]

I decoupled this section because I though PostNotify doesn't need lock. As it
was wrong can we move this block in the lock-protected area? I don't know if
taking two small locks is better than taking one medium-sized lock.

[hiroshige, 2015/07/15 21:35:40]

Done.
Taking one lock instead of two is at least good for readability, I think.

     // 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());
@@ skipping 141 matching lines @@
 SharedMemoryDataConsumerHandle::ReaderImpl*
 SharedMemoryDataConsumerHandle::obtainReaderInternal(Client* client) {
   return new ReaderImpl(context_, client);
 }
 
 const char* SharedMemoryDataConsumerHandle::debugName() const {
   return "SharedMemoryDataConsumerHandle";
 }
 
 }  // namespace content