IPC: Fix attachment brokering race condition.

content/browser/renderer_host/render_process_host_impl.cc

 // Copyright 2012 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.
 
 // Represents the browser side of the browser <--> renderer communication
 // channel. There will be one RenderProcessHost per renderer process.
 
 #include "content/browser/renderer_host/render_process_host_impl.h"
 
 #include <algorithm>
@@ skipping 710 matching lines @@
   if (renderer_path.empty())
     return false;
 
   channel_connected_ = false;
   sent_render_process_ready_ = false;
 
   // Setup the IPC channel.
   const std::string channel_id =
       IPC::Channel::GenerateVerifiedChannelID(std::string());
   channel_ = CreateChannelProxy(channel_id);
-#if USE_ATTACHMENT_BROKER
-  IPC::AttachmentBroker::GetGlobal()->RegisterCommunicationChannel(
-      channel_.get(), content::BrowserThread::GetMessageLoopProxyForThread(
-      content::BrowserThread::IO));
-#endif
 
   // Call the embedder first so that their IPC filters have priority.
   GetContentClient()->browser()->RenderProcessWillLaunch(this);
 
   CreateMessageFilters();
   RegisterMojoServices();
 
   if (run_renderer_in_process()) {
     DCHECK(g_renderer_main_thread_factory);
     // Crank up a thread and run the initialization there.  With the way that
@@ skipping 75 matching lines @@
     // for Android WebView to maintain backward compatibility.
     // See crbug.com/526842 for details.
 #if defined(OS_ANDROID)
     if (GetContentClient()->UsingSynchronousCompositing()) {
       return IPC::SyncChannel::Create(
           IPC::ChannelMojo::CreateServerFactory(std::move(handle)), this,
           runner.get(), true, &never_signaled_);
     }
 #endif  // OS_ANDROID
 
-    return IPC::ChannelProxy::Create(

[Charlie Reis, 2016/04/22 20:00:45]

Your CL description makes it sounds like this call isn't safe, since it calls
Init before Register.  Is there any way to make it safe in general (by passing
in what's needed to register, perhaps), rather than trying to patch up all the
call sites?

[erikchen, 2016/04/25 20:10:57]

I considered this. The reason I chose not to do this is because attachment
brokering is only supported on Mac and Windows, and this ends up cluttering the
public interface on other OSes. 

-        IPC::ChannelMojo::CreateServerFactory(std::move(handle)), this,
-        runner.get());
+  std::unique_ptr<IPC::ChannelProxy> channel(
+      new IPC::ChannelProxy(this, runner.get()));
+#if USE_ATTACHMENT_BROKER
+  IPC::AttachmentBroker::GetGlobal()->RegisterCommunicationChannel(
+      channel.get(), content::BrowserThread::GetMessageLoopProxyForThread(
+      content::BrowserThread::IO));
+#endif
+  channel->Init(IPC::ChannelMojo::CreateServerFactory(std::move(handle)), true);
+  return channel;
   }
 
     // Do NOT expand ifdef or run time condition checks here! See comment above.
 #if defined(OS_ANDROID)
   if (GetContentClient()->UsingSynchronousCompositing()) {
     return IPC::SyncChannel::Create(channel_id, IPC::Channel::MODE_SERVER, this,
                                     runner.get(), true, &never_signaled_);
   }
 #endif  // OS_ANDROID
 
-  return IPC::ChannelProxy::Create(channel_id, IPC::Channel::MODE_SERVER, this,
-                                   runner.get());
+  std::unique_ptr<IPC::ChannelProxy> channel(
+      new IPC::ChannelProxy(this, runner.get()));
+#if USE_ATTACHMENT_BROKER
+  IPC::AttachmentBroker::GetGlobal()->RegisterCommunicationChannel(
+      channel.get(), content::BrowserThread::GetMessageLoopProxyForThread(
+      content::BrowserThread::IO));
+#endif
+  channel->Init(channel_id, IPC::Channel::MODE_SERVER, true);
+  return channel;
 }
 
 void RenderProcessHostImpl::CreateMessageFilters() {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   const base::CommandLine& browser_command_line =
       *base::CommandLine::ForCurrentProcess();
   AddFilter(new ResourceSchedulerFilter(GetID()));
   MediaInternals* media_internals = MediaInternals::GetInstance();
   media::AudioManager* audio_manager =
       BrowserMainLoop::GetInstance()->audio_manager();
@@ skipping 1969 matching lines @@
 
     // Skip widgets in other processes.
     if (rvh->GetProcess()->GetID() != GetID())
       continue;
 
     rvh->OnWebkitPreferencesChanged();
   }
 }
 
 }  // namespace content