Revert of Eliminate a potential race in IPC::ChannelProxy (https://co…

…dereview.chromium.org/183553004/)

Reason for revert:
Since this has landed, testsuite content_browsertests is failing on bot Android Tests (dbg) on the chromium.linux waterfall.

Specifically, the WebContentsImplBrowserTest.OpenURLSubframe test is consistently crashing, with the following DCHECK:

[FATAL:device_orientation_message_filter.cc(18)] Check failed: BrowserThread::CurrentlyOn(BrowserThread::IO).

This corresponds the the following DCHECK about being on the IO thread:

DeviceOrientationMessageFilter::~DeviceOrientationMessageFilter() {
  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
  if (is_started_)
    DeviceInertialSensorService::GetInstance()->RemoveConsumer(
        CONSUMER_TYPE_ORIENTATION);
}

This same DCHECK failed in one of the try jobs on this CL: http://build.chromium.org/p/tryserver.chromium/builders/android_dbg_triggered_tests/builds/133648

Since this DCHECK has never been observed to fail before (certainly not in the last 200 builds), and has failed both in this CL's try job and twice in a row on the waterfall since landing this CL, it seems very likely that this CL is the cause.

At a guess, the changes to ChannelProxy::Context::OnRemoveFilter seem quite relevant here.

Original issue's description:
> Eliminate a potential race in IPC::ChannelProxy
> 
> Doing the following steps with ChannelProxy leads to a data race:
> 1) Create the ChannelProxy, but don't initialize it.
> 2) Add a filter.
> 3) Init the ChannelProxy.
> 
> The problem is, AddFilter() posts a task from the Listener thread to the IPC task runner to do OnAddFilter. Prior to this patch, OnAddFilter will try to read channel_ even though channel_ may not have been initialized, and it's accessed without any synchronization.
> 
> This patch only really adds the filter if peer_pid_ has been set on the IPC::Channel thread; otherwise, it waits until the connection has been established to really add filters.
> 
> See the bug for more detail.
> 
> BUG=244383
> 
> Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=256188

TBR=jam@chromium.org,dmichael@chromium.org
NOTREECHECKS=true
NOTRY=true
BUG=244383

Review URL: https://codereview.chromium.org/194923004

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@256221 0039d316-1c4b-4281-b951-d872f2087c98

ipc/ipc_channel_nacl.cc

@@ -17,7 +17,6 @@
 #include "base/task_runner_util.h"
 #include "base/threading/simple_thread.h"
 #include "ipc/file_descriptor_set_posix.h"
-#include "ipc/ipc_listener.h"
 #include "ipc/ipc_logging.h"
 #include "native_client/src/public/imc_syscalls.h"
 #include "native_client/src/public/imc_types.h"
@@ -139,15 +138,9 @@ Channel::ChannelImpl::~ChannelImpl() {
   Close();
 }
 
-base::ProcessId Channel::ChannelImpl::peer_pid() const {
-  // This shouldn't actually get used in the untrusted side of the proxy, and we
-  // don't have the real pid anyway.
-  return -1;
-}
-
 bool Channel::ChannelImpl::Connect() {
   if (pipe_ == -1) {
-    DLOG(WARNING) << "Channel creation failed: " << pipe_name_;
+    DLOG(INFO) << "Channel creation failed: " << pipe_name_;
     return false;
   }
 
@@ -171,10 +164,6 @@ bool Channel::ChannelImpl::Connect() {
   waiting_connect_ = false;
   // If there were any messages queued before connection, send them.
   ProcessOutgoingMessages();
-  base::MessageLoopProxy::current()->PostTask(FROM_HERE,
-      base::Bind(&Channel::ChannelImpl::CallOnChannelConnected,
-                 weak_ptr_factory_.GetWeakPtr()));
-
   return true;
 }
 
@@ -304,10 +293,6 @@ bool Channel::ChannelImpl::ProcessOutgoingMessages() {
   return true;
 }
 
-void Channel::ChannelImpl::CallOnChannelConnected() {
-  listener()->OnChannelConnected(peer_pid());
-}
-
 Channel::ChannelImpl::ReadState Channel::ChannelImpl::ReadData(
     char* buffer,
     int buffer_len,
@@ -387,7 +372,9 @@ void Channel::Close() {
 }
 
 base::ProcessId Channel::peer_pid() const {
-  return channel_impl_->peer_pid();
+  // This shouldn't actually get used in the untrusted side of the proxy, and we
+  // don't have the real pid anyway.
+  return -1;
 }
 
 bool Channel::Send(Message* message) {

ipc/ipc_channel_nacl.h

@@ -40,7 +40,6 @@ class Channel::ChannelImpl : public internal::ChannelReader {
   virtual ~ChannelImpl();
 
   // Channel implementation.
-  base::ProcessId peer_pid() const;
   bool Connect();
   void Close();
   bool Send(Message* message);
@@ -54,7 +53,6 @@ class Channel::ChannelImpl : public internal::ChannelReader {
 
   bool CreatePipe(const IPC::ChannelHandle& channel_handle);
   bool ProcessOutgoingMessages();
-  void CallOnChannelConnected();
 
   // ChannelReader implementation.
   virtual ReadState ReadData(char* buffer,

ipc/ipc_channel_proxy.cc

@@ -156,7 +156,7 @@ void ChannelProxy::Context::ClearIPCTaskRunner() {
 
 void ChannelProxy::Context::CreateChannel(const IPC::ChannelHandle& handle,
                                           const Channel::Mode& mode) {
-  DCHECK(!channel_);
+  DCHECK(channel_.get() == NULL);
   channel_id_ = handle.name;
   channel_.reset(new Channel(handle, mode, this));
 }
@@ -196,15 +196,17 @@ bool ChannelProxy::Context::OnMessageReceivedNoFilter(const Message& message) {
 
 // Called on the IPC::Channel thread
 void ChannelProxy::Context::OnChannelConnected(int32 peer_pid) {
-  // We cache off the peer_pid so it can be safely accessed from both threads.
-  peer_pid_ = channel_->peer_pid();
-
   // Add any pending filters.  This avoids a race condition where someone
   // creates a ChannelProxy, calls AddFilter, and then right after starts the
   // peer process.  The IO thread could receive a message before the task to add
   // the filter is run on the IO thread.
   OnAddFilter();
 
+  // We cache off the peer_pid so it can be safely accessed from both threads.
+  peer_pid_ = channel_->peer_pid();
+  for (size_t i = 0; i < filters_.size(); ++i)
+    filters_[i]->OnChannelConnected(peer_pid);
+
   // See above comment about using listener_task_runner_ here.
   listener_task_runner_->PostTask(
       FROM_HERE, base::Bind(&Context::OnDispatchConnected, this));
@@ -241,7 +243,7 @@ void ChannelProxy::Context::OnChannelOpened() {
 void ChannelProxy::Context::OnChannelClosed() {
   // It's okay for IPC::ChannelProxy::Close to be called more than once, which
   // would result in this branch being taken.
-  if (!channel_)
+  if (!channel_.get())
     return;
 
   for (size_t i = 0; i < filters_.size(); ++i) {
@@ -266,24 +268,16 @@ void ChannelProxy::Context::Clear() {
 
 // Called on the IPC::Channel thread
 void ChannelProxy::Context::OnSendMessage(scoped_ptr<Message> message) {
-  if (!channel_) {
+  if (!channel_.get()) {
     OnChannelClosed();
     return;
   }
-
   if (!channel_->Send(message.release()))
     OnChannelError();
 }
 
 // Called on the IPC::Channel thread
 void ChannelProxy::Context::OnAddFilter() {
-  // Our OnChannelConnected method has not yet been called, so we can't be
-  // sure that channel_ is valid yet. When OnChannelConnected *is* called,
-  // it invokes OnAddFilter, so any pending filter(s) will be added at that
-  // time.
-  if (peer_pid_ == base::kNullProcessId)
-    return;
-
   std::vector<scoped_refptr<MessageFilter> > new_filters;
   {
     base::AutoLock auto_lock(pending_filters_lock_);
@@ -295,28 +289,19 @@ void ChannelProxy::Context::OnAddFilter() {
 
     message_filter_router_->AddFilter(new_filters[i].get());
 
-    // The channel has already been created and connected, so we need to
-    // inform the filters right now.
-    new_filters[i]->OnFilterAdded(channel_.get());
-    new_filters[i]->OnChannelConnected(peer_pid_);
+    // If the channel has already been created, then we need to send this
+    // message so that the filter gets access to the Channel.
+    if (channel_.get())
+      new_filters[i]->OnFilterAdded(channel_.get());
+    // Ditto for if the channel has been connected.
+    if (peer_pid_)
+      new_filters[i]->OnChannelConnected(peer_pid_);
   }
 }
 
 // Called on the IPC::Channel thread
 void ChannelProxy::Context::OnRemoveFilter(MessageFilter* filter) {
-  if (peer_pid_ == base::kNullProcessId) {
-    // The channel is not yet connected, so any filters are still pending.
-    base::AutoLock auto_lock(pending_filters_lock_);
-    for (size_t i = 0; i < pending_filters_.size(); ++i) {
-      if (pending_filters_[i].get() == filter) {
-        filter->OnFilterRemoved();
-        pending_filters_.erase(pending_filters_.begin() + i);
-        return;
-      }
-    }
-    return;
-  }
-  if (!channel_)
+  if (!channel_.get())
     return;  // The filters have already been deleted.
 
   message_filter_router_->RemoveFilter(filter);

ipc/ipc_channel_proxy.h

@@ -233,10 +233,6 @@ class IPC_EXPORT ChannelProxy : public Sender, public base::NonThreadSafe {
     // List of filters.  This is only accessed on the IPC thread.
     std::vector<scoped_refptr<MessageFilter> > filters_;
     scoped_refptr<base::SingleThreadTaskRunner> ipc_task_runner_;
-
-    // Note, channel_ may be set on the Listener thread or the IPC thread.
-    // But once it has been set, it must only be read or cleared on the IPC
-    // thread.
     scoped_ptr<Channel> channel_;
     std::string channel_id_;
     bool channel_connected_called_;