Remove lock from SharedMemoryDataConsumerHandle::Context destructor

SharedMemoryDataConsumerHandle::Context posts tasks while its lock_
locked. That means a lock held by the TaskRunner will be locked
after |lock_| is locked. On the other hand, the Context may be
destructed in the middle of TaskRunner destruction, which means we
cannot lock |lock_| in the Context destruction. Otherwise, a
lock-order-inversion problem will be introduced.

This CL stops locking |lock_| in the destructor. It is safe because
no one accesses to Content's member variables without a valid
scoped_refptr to the Context.

BUG=651747

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

content/child/shared_memory_data_consumer_handle.cc

@@ -89,19 +89,16 @@ class SharedMemoryDataConsumerHandle::Context final
   }
   void ClearQueue() {
     lock_.AssertAcquired();
-    for (auto* data : queue_) {
-      delete data;
-    }
     queue_.clear();
     first_offset_ = 0;
   }
   RequestPeer::ThreadSafeReceivedData* Top() {
     lock_.AssertAcquired();
-    return queue_.front();
+    return queue_.front().get();
   }
   void Push(std::unique_ptr<RequestPeer::ThreadSafeReceivedData> data) {
     lock_.AssertAcquired();
-    queue_.push_back(data.release());
+    queue_.push_back(std::move(data));
   }
   size_t first_offset() const {
     lock_.AssertAcquired();
@@ -188,7 +185,6 @@ class SharedMemoryDataConsumerHandle::Context final
     first_offset_ += s;
     auto* top = Top();
     if (static_cast<size_t>(top->length()) <= first_offset_) {
-      delete top;
       queue_.pop_front();
       first_offset_ = 0;
     }
@@ -232,14 +228,8 @@ class SharedMemoryDataConsumerHandle::Context final
   }
   void Clear() {
     lock_.AssertAcquired();
-    for (auto* data : queue_) {
-      delete data;
-    }
-    queue_.clear();
-    first_offset_ = 0;
+    ClearQueue();
     client_ = nullptr;
-    // Note this doesn't work in the destructor if |on_reader_detached_| is not
-    // null. We have an assert in the destructor.
     ResetOnReaderDetached();
   }
   // Must be called with |lock_| not aquired.
@@ -249,21 +239,13 @@ class SharedMemoryDataConsumerHandle::Context final
   }
 
   friend class base::RefCountedThreadSafe<Context>;
-  ~Context() {
-    base::AutoLock lock(lock_);
-    DCHECK(on_reader_detached_.is_null());
-
-    // This is necessary because the queue stores raw pointers.
-    Clear();
-  }
+  ~Context() = default;
 
   base::Lock lock_;
   // |result_| stores the ultimate state of this handle if it has. Otherwise,
   // |Ok| is set.
   Result result_;
-  // TODO(yhirano): Use std::deque<std::unique_ptr<ThreadSafeReceivedData>>
-  // once it is allowed.
-  std::deque<RequestPeer::ThreadSafeReceivedData*> queue_;
+  std::deque<std::unique_ptr<RequestPeer::ThreadSafeReceivedData>> queue_;
   size_t first_offset_;
   Client* client_;
   scoped_refptr<base::SingleThreadTaskRunner> notification_task_runner_;

content/child/shared_memory_data_consumer_handle_unittest.cc

@@ -19,6 +19,7 @@
 #include "base/message_loop/message_loop.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
+#include "base/strings/string_split.h"
 #include "base/task_runner.h"
 #include "base/threading/thread.h"
 #include "base/threading/thread_task_runner_handle.h"
@@ -974,15 +975,22 @@ TEST(SharedMemoryDataConsumerHandleBackpressureTest, CloseAndReset) {
   reader.reset();
   logger->Add("4");
 
-  EXPECT_EQ(
-      "1\n"
-      "2\n"
-      "3\n"
-      "data1 is destructed.\n"
-      "data2 is destructed.\n"
-      "data3 is destructed.\n"
-      "4\n",
-      logger->log());
+  std::vector<std::string> log = base::SplitString(
+      logger->log(), "\n", base::KEEP_WHITESPACE, base::SPLIT_WANT_ALL);
+
+  ASSERT_EQ(8u, log.size());
+  EXPECT_EQ("1", log[0]);
+  EXPECT_EQ("2", log[1]);
+  EXPECT_EQ("3", log[2]);
+  EXPECT_EQ("4", log[6]);
+  EXPECT_EQ("", log[7]);
+
+  // The destruction order doesn't matter in this case.
+  std::vector<std::string> destruction_entries = {log[3], log[4], log[5]};
+  std::sort(destruction_entries.begin(), destruction_entries.end());
+  EXPECT_EQ(destruction_entries[0], "data1 is destructed.");
+  EXPECT_EQ(destruction_entries[1], "data2 is destructed.");
+  EXPECT_EQ(destruction_entries[2], "data3 is destructed.");
 }
 
 TEST(SharedMemoryDataConsumerHandleWithoutBackpressureTest, AddData) {