PPAPI/NaCl: Move channel connection out of NaClIPCAdapter constructor

This should fix a rare race condition crash. (See comments in code for more info).

BUG=116317
TBR=bbudge1

Review URL: https://chromiumcodereview.appspot.com/11364183

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

3 files changed, 22 insertions, 2 deletions

diff --git a/chrome/nacl/nacl_ipc_adapter.cc b/chrome/nacl/nacl_ipc_adapter.cc
index 72b266f..1ee0c37 100644
--- a/chrome/nacl/nacl_ipc_adapter.cc
+++ b/chrome/nacl/nacl_ipc_adapter.cc
@@ -295,8 +295,11 @@ NaClIPCAdapter::NaClIPCAdapter(const IPC::ChannelHandle& handle,
       locked_data_() {
   io_thread_data_.channel_.reset(
       new IPC::Channel(handle, IPC::Channel::MODE_SERVER, this));
-  task_runner_->PostTask(FROM_HERE,
-      base::Bind(&NaClIPCAdapter::ConnectChannelOnIOThread, this));
+  // Note, we can not PostTask for ConnectChannelOnIOThread here. If we did,
+  // and that task ran before this constructor completes, the reference count
+  // would go to 1 and then to 0 because of the Task, before we've been returned
+  // to the owning scoped_refptr, which is supposed to give us our first
+  // ref-count.
 }
 
 NaClIPCAdapter::NaClIPCAdapter(scoped_ptr<IPC::Channel> channel,
@@ -308,6 +311,11 @@ NaClIPCAdapter::NaClIPCAdapter(scoped_ptr<IPC::Channel> channel,
   io_thread_data_.channel_ = channel.Pass();
 }
 
+void NaClIPCAdapter::ConnectChannel() {
+  task_runner_->PostTask(FROM_HERE,
+      base::Bind(&NaClIPCAdapter::ConnectChannelOnIOThread, this));
+}
+
 // Note that this message is controlled by the untrusted code. So we should be
 // skeptical of anything it contains and quick to give up if anything is fishy.
 int NaClIPCAdapter::Send(const NaClImcTypedMsgHdr* msg) {
@@ -523,6 +531,7 @@ bool NaClIPCAdapter::OnMessageReceived(const IPC::Message& msg) {
               IPC::Channel::GenerateVerifiedChannelID("nacl");
           scoped_refptr<NaClIPCAdapter> ipc_adapter(
               new NaClIPCAdapter(channel_handle, task_runner_));
+          ipc_adapter->ConnectChannel();
 #if defined(OS_POSIX)
           channel_handle.socket = base::FileDescriptor(
               ipc_adapter->TakeClientFileDescriptor(), true);
diff --git a/chrome/nacl/nacl_ipc_adapter.h b/chrome/nacl/nacl_ipc_adapter.h
index 5b323e3..3752139 100644
--- a/chrome/nacl/nacl_ipc_adapter.h
+++ b/chrome/nacl/nacl_ipc_adapter.h
@@ -72,12 +72,21 @@ class NaClIPCAdapter : public base::RefCountedThreadSafe<NaClIPCAdapter>,
   // Creates an adapter, using the thread associated with the given task
   // runner for posting messages. In normal use, the task runner will post to
   // the I/O thread of the process.
+  //
+  // If you use this constructor, you MUST call ConnectChannel after the
+  // NaClIPCAdapter is constructed, or the NaClIPCAdapter's channel will not be
+  // connected.
   NaClIPCAdapter(const IPC::ChannelHandle& handle, base::TaskRunner* runner);
 
   // Initializes with a given channel that's already created for testing
   // purposes. This function will take ownership of the given channel.
   NaClIPCAdapter(scoped_ptr<IPC::Channel> channel, base::TaskRunner* runner);
 
+  // Connect the channel. This must be called after the constructor that accepts
+  // an IPC::ChannelHandle, and causes the Channel to be connected on the IO
+  // thread.
+  void ConnectChannel();
+
   // Implementation of sendmsg. Returns the number of bytes written or -1 on
   // failure.
   int Send(const NaClImcTypedMsgHdr* msg);
diff --git a/chrome/nacl/nacl_listener.cc b/chrome/nacl/nacl_listener.cc
index d8e5dad..901afd0 100644
--- a/chrome/nacl/nacl_listener.cc
+++ b/chrome/nacl/nacl_listener.cc
@@ -205,6 +205,8 @@ void NaClListener::OnMsgStart(const nacl::NaClStartParams& params) {
         IPC::Channel::GenerateVerifiedChannelID("nacl");
     scoped_refptr<NaClIPCAdapter> ipc_adapter(
         new NaClIPCAdapter(handle, io_thread_.message_loop_proxy()));
+    ipc_adapter->ConnectChannel();
+
     // Pass a NaClDesc to the untrusted side. This will hold a ref to the
     // NaClIPCAdapter.
     args->initial_ipc_desc = ipc_adapter->MakeNaClDesc();