Oops, calling {Producer,Consumer}CloseImplNoLock() isn't the same as calling {Producer,Consumer}Clo…

mojo/edk/system/data_pipe.cc

Index: mojo/edk/system/data_pipe.cc
diff --git a/mojo/edk/system/data_pipe.cc b/mojo/edk/system/data_pipe.cc
index 3b71d6d025be5be714fde249eba85b210d03a78e..aede49280b922bb2d5e689ecdec5c0f98050f3de 100644
--- a/mojo/edk/system/data_pipe.cc
+++ b/mojo/edk/system/data_pipe.cc
@@ -91,17 +91,7 @@ void DataPipe::ProducerCancelAllAwakables() {
 
 void DataPipe::ProducerClose() {
   base::AutoLock locker(lock_);
-  DCHECK(producer_open_);
-  producer_open_ = false;
-  DCHECK(has_local_producer_no_lock());
-  producer_awakable_list_.reset();
-  // Not a bug, except possibly in "user" code.
-  DVLOG_IF(2, producer_in_two_phase_write_no_lock())
-      << "Producer closed with active two-phase write";
-  producer_two_phase_max_num_bytes_written_ = 0;
-  ProducerCloseImplNoLock();
-  AwakeConsumerAwakablesForStateChangeNoLock(
-      ConsumerGetHandleSignalsStateImplNoLock());
+  ProducerCloseNoLock();
 }
 
 MojoResult DataPipe::ProducerWriteData(UserPointer<const void> elements,
@@ -271,17 +261,7 @@ void DataPipe::ConsumerCancelAllAwakables() {
 
 void DataPipe::ConsumerClose() {
   base::AutoLock locker(lock_);
-  DCHECK(consumer_open_);
-  consumer_open_ = false;
-  DCHECK(has_local_consumer_no_lock());
-  consumer_awakable_list_.reset();
-  // Not a bug, except possibly in "user" code.
-  DVLOG_IF(2, consumer_in_two_phase_read_no_lock())
-      << "Consumer closed with active two-phase read";
-  consumer_two_phase_max_num_bytes_read_ = 0;
-  ConsumerCloseImplNoLock();
-  AwakeProducerAwakablesForStateChangeNoLock(
-      ProducerGetHandleSignalsStateImplNoLock());
+  ConsumerCloseNoLock();
 }
 
 MojoResult DataPipe::ConsumerReadData(UserPointer<void> elements,
@@ -501,6 +481,36 @@ DataPipe::~DataPipe() {
   DCHECK(!consumer_awakable_list_);
 }
 
+void DataPipe::ProducerCloseNoLock() {
+  lock_.AssertAcquired();
+  DCHECK(producer_open_);
+  producer_open_ = false;
+  DCHECK(has_local_producer_no_lock());
+  producer_awakable_list_.reset();
+  // Not a bug, except possibly in "user" code.
+  DVLOG_IF(2, producer_in_two_phase_write_no_lock())
+      << "Producer closed with active two-phase write";
+  producer_two_phase_max_num_bytes_written_ = 0;
+  ProducerCloseImplNoLock();

[yzshen1, 2015/02/19 18:07:50]

I feel that usually XXXImpl should do all (or most) of XXX's work. This doesn't
seem the case. Maybe we should change it to use a more specific name? It helps
to avoid such confusion in the future.

+  AwakeConsumerAwakablesForStateChangeNoLock(
+      ConsumerGetHandleSignalsStateImplNoLock());
+}
+
+void DataPipe::ConsumerCloseNoLock() {
+  lock_.AssertAcquired();
+  DCHECK(consumer_open_);
+  consumer_open_ = false;
+  DCHECK(has_local_consumer_no_lock());
+  consumer_awakable_list_.reset();
+  // Not a bug, except possibly in "user" code.
+  DVLOG_IF(2, consumer_in_two_phase_read_no_lock())
+      << "Consumer closed with active two-phase read";
+  consumer_two_phase_max_num_bytes_read_ = 0;
+  ConsumerCloseImplNoLock();

[yzshen1, 2015/02/19 18:07:50]

ditto

+  AwakeProducerAwakablesForStateChangeNoLock(
+      ProducerGetHandleSignalsStateImplNoLock());
+}
+
 void DataPipe::AwakeProducerAwakablesForStateChangeNoLock(
     const HandleSignalsState& new_producer_state) {
   lock_.AssertAcquired();