Allow mojo::system::RawChannel::Delegate methods to destroy the RawChannel.

mojo/edk/system/raw_channel.cc

 // Copyright 2014 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 "mojo/edk/system/raw_channel.h"
 
 #include <string.h>
 
 #include <algorithm>
 
@@ skipping 138 matching lines @@
       static_cast<const char*>(message->transport_data()->buffer()),
       transport_data_buffer_size};
   buffers->push_back(buffer2);
 }
 
 // RawChannel ------------------------------------------------------------------
 
 RawChannel::RawChannel()
     : message_loop_for_io_(nullptr),
       delegate_(nullptr),
-      read_stopped_(false),
+      set_on_shutdown_(nullptr),
       write_stopped_(false),
       weak_ptr_factory_(this) {
 }
 
 RawChannel::~RawChannel() {
   DCHECK(!read_buffer_);
   DCHECK(!write_buffer_);
 
   // No need to take the |write_lock_| here -- if there are still weak pointers
   // outstanding, then we're hosed anyway (since we wouldn't be able to
@@ skipping 35 matching lines @@
 void RawChannel::Shutdown() {
   DCHECK_EQ(base::MessageLoop::current(), message_loop_for_io_);
 
   base::AutoLock locker(write_lock_);
 
   LOG_IF(WARNING, !write_buffer_->message_queue_.empty())
       << "Shutting down RawChannel with write buffer nonempty";
 
   // Reset the delegate so that it won't receive further calls.
   delegate_ = nullptr;
-  read_stopped_ = true;
+  if (set_on_shutdown_) {
+    *set_on_shutdown_ = true;
+    set_on_shutdown_ = nullptr;
+  }
   write_stopped_ = true;
   weak_ptr_factory_.InvalidateWeakPtrs();
 
   OnShutdownNoLock(read_buffer_.Pass(), write_buffer_.Pass());
 }
 
 // Reminder: This must be thread-safe.
 bool RawChannel::WriteMessage(scoped_ptr<MessageInTransit> message) {
   DCHECK(message);
 
@@ skipping 31 matching lines @@
 
 // Reminder: This must be thread-safe.
 bool RawChannel::IsWriteBufferEmpty() {
   base::AutoLock locker(write_lock_);
   return write_buffer_->message_queue_.empty();
 }
 
 void RawChannel::OnReadCompleted(IOResult io_result, size_t bytes_read) {
   DCHECK_EQ(base::MessageLoop::current(), message_loop_for_io_);
 
-  if (read_stopped_) {

[yzshen1, 2015/01/21 23:03:42]

Without this NOTREACHED(), maybe we should add more comments for
OnShutdownNoLock(), saying that after OnShutdownNoLock() subclasses shouldn't
make any OnReadCompleted() call.

[viettrungluu, 2015/01/21 23:34:31]

Done.

-    NOTREACHED();
-    return;
-  }
-
   // Keep reading data in a loop, and dispatch messages if enough data is
   // received. Exit the loop if any of the following happens:
   //   - one or more messages were dispatched;
   //   - the last read failed, was a partial read or would block;
   //   - |Shutdown()| was called.
   do {
     switch (io_result) {
       case IO_SUCCEEDED:
         break;
       case IO_FAILED_SHUTDOWN:
       case IO_FAILED_BROKEN:
       case IO_FAILED_UNKNOWN:
-        read_stopped_ = true;
         CallOnError(ReadIOResultToError(io_result));
-        return;
+        return;  // |this| may have been destroyed in |CallOnError()|.
       case IO_PENDING:
         NOTREACHED();
         return;
     }
 
     read_buffer_->num_valid_bytes_ += bytes_read;
 
     // Dispatch all the messages that we can.
     bool did_dispatch_message = false;
     // Tracks the offset of the first undispatched message in |read_buffer_|.
@@ skipping 14 matching lines @@
            remaining_bytes >= message_size) {
       MessageInTransit::View message_view(
           message_size, &read_buffer_->buffer_[read_buffer_start]);
       DCHECK_EQ(message_view.total_size(), message_size);
 
       const char* error_message = nullptr;
       if (!message_view.IsValid(GetSerializedPlatformHandleSize(),
                                 &error_message)) {
         DCHECK(error_message);
         LOG(ERROR) << "Received invalid message: " << error_message;
-        read_stopped_ = true;
         CallOnError(Delegate::ERROR_READ_BAD_MESSAGE);
-        return;
+        return;  // |this| may have been destroyed in |CallOnError()|.
       }
 
       if (message_view.type() == MessageInTransit::kTypeRawChannel) {
         if (!OnReadMessageForRawChannel(message_view)) {
-          read_stopped_ = true;
           CallOnError(Delegate::ERROR_READ_BAD_MESSAGE);
-          return;
+          return;  // |this| may have been destroyed in |CallOnError()|.
         }
       } else {
         embedder::ScopedPlatformHandleVectorPtr platform_handles;
         if (message_view.transport_data_buffer()) {
           size_t num_platform_handles;
           const void* platform_handle_table;
           TransportData::GetPlatformHandleTable(
               message_view.transport_data_buffer(), &num_platform_handles,
               &platform_handle_table);
 
           if (num_platform_handles > 0) {
             platform_handles =
                 GetReadPlatformHandles(num_platform_handles,
                                        platform_handle_table).Pass();
             if (!platform_handles) {
               LOG(ERROR) << "Invalid number of platform handles received";
-              read_stopped_ = true;
               CallOnError(Delegate::ERROR_READ_BAD_MESSAGE);
-              return;
+              return;  // |this| may have been destroyed in |CallOnError()|.
             }
           }
         }
 
         // TODO(vtl): In the case that we aren't expecting any platform handles,
         // for the POSIX implementation, we should confirm that none are stored.
 
         // Dispatch the message.
+        // Detect the case when |Shutdown()| is called; subsequent destruction
+        // is also permitted then.
+        bool shutdown_called = false;
+        DCHECK(!set_on_shutdown_);
+        set_on_shutdown_ = &shutdown_called;
         DCHECK(delegate_);
         delegate_->OnReadMessage(message_view, platform_handles.Pass());
-        if (read_stopped_) {
-          // |Shutdown()| was called in |OnReadMessage()|.
-          // TODO(vtl): Add test for this case.
+        if (shutdown_called)
           return;
-        }
+        set_on_shutdown_ = nullptr;
       }
 
       did_dispatch_message = true;
 
       // Update our state.
       read_buffer_start += message_size;
       remaining_bytes -= message_size;
     }
 
     if (read_buffer_start > 0) {
@@ skipping 47 matching lines @@
 
     if (write_stopped_) {
       NOTREACHED();
       return;
     }
 
     did_fail = !OnWriteCompletedNoLock(io_result, platform_handles_written,
                                        bytes_written);
   }
 
-  if (did_fail)
+  if (did_fail) {
     CallOnError(Delegate::ERROR_WRITE);
+    return;  // |this| may have been destroyed in |CallOnError()|.
+  }
 }
 
 void RawChannel::EnqueueMessageNoLock(scoped_ptr<MessageInTransit> message) {
   write_lock_.AssertAcquired();
   write_buffer_->message_queue_.push_back(message.release());
 }
 
 bool RawChannel::OnReadMessageForRawChannel(
     const MessageInTransit::View& message_view) {
   // No non-implementation specific |RawChannel| control messages.
@@ skipping 16 matching lines @@
     case IO_PENDING:
       NOTREACHED();
       break;
   }
   return Delegate::ERROR_READ_UNKNOWN;
 }
 
 void RawChannel::CallOnError(Delegate::Error error) {
   DCHECK_EQ(base::MessageLoop::current(), message_loop_for_io_);
   // TODO(vtl): Add a "write_lock_.AssertNotAcquired()"?
-  if (delegate_)
+  if (delegate_) {
     delegate_->OnError(error);
+    return;  // |this| may have been destroyed in |OnError()|.
+  }
 }
 
 bool RawChannel::OnWriteCompletedNoLock(IOResult io_result,
                                         size_t platform_handles_written,
                                         size_t bytes_written) {
   write_lock_.AssertAcquired();
 
   DCHECK(!write_stopped_);
   DCHECK(!write_buffer_->message_queue_.empty());
 
@@ skipping 23 matching lines @@
 
   write_stopped_ = true;
   STLDeleteElements(&write_buffer_->message_queue_);
   write_buffer_->platform_handles_offset_ = 0;
   write_buffer_->data_offset_ = 0;
   return false;
 }
 
 }  // namespace system
 }  // namespace mojo