Non-SFI: move socketpair() from plugin process to browser process.

components/nacl/browser/nacl_process_host.cc

 // Copyright (c) 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.
 
 #include "components/nacl/browser/nacl_process_host.h"
 
 #include <algorithm>
 #include <string>
 #include <vector>
 
@@ skipping 803 matching lines @@
 }
 #endif
 
 bool NaClProcessHost::StartNaClExecution() {
   NaClBrowser* nacl_browser = NaClBrowser::GetInstance();
 
   NaClStartParams params;
 
   // Enable PPAPI proxy channel creation only for renderer processes.
   params.enable_ipc_proxy = enable_ppapi_proxy();
+  // Currently, non-SFI mode is supported only on Linux.
+#if defined(OS_LINUX)
+  if (params.enable_ipc_proxy && uses_nonsfi_mode_) {

[Mark Seaborn, 2015/04/15 20:52:05]

Nit: At the moment, uses_nonsfi_mode_ implies enable_ipc_proxy, I think.

To avoid making this conditional look more tested/supported than it is, you
could do:
if (uses_nonsfi_mode_) {
  DCHECK(params.enable_ipc_proxy);
  ...
}

[hidehiko, 2015/05/12 18:47:51]

Done.

+    if (!IPC::SocketPair(&params.browser_server_ppapi_fd.fd,
+                         &params.browser_client_ppapi_fd.fd) ||
+        !IPC::SocketPair(&params.renderer_server_ppapi_fd.fd,

[Mark Seaborn, 2015/04/15 20:52:05]

If the subsequent IPC::SocketPair() calls fail, won't this leak FDs, since the
base::FileDescriptor type used in NaClStartParams doesn't have a destructor?

Leaking FDs might be a pre-existing problem in this code path, but if we're
creating 8 more FDs here it could be more likely to fail due to the FD table
filling up.

[Mark Seaborn, 2015/05/12 06:46:10]

I guess you're trying to clean up that pre-existing problem above (in
https://codereview.chromium.org/1117883002/) before committing this change? 
Thanks for doing that.  However, I'm not sure you should block this change on
doing that cleanup, because the cleanup is rather difficult.  It's not clear
exactly how we should handle failures like these.

For comparison, Chromium has a policy to abort on memory allocation failure (at
least for fine-grained allocations):
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/yJUd6gKCYSE...

And in a previous discussion it has been suggested that there should be a
similar policy for out-of-FDs:
https://groups.google.com/a/chromium.org/forum/#!msg/chromium-dev/lFgKOYo3iYM...

So arguably it might be not too bad to crash or leak some FDs if one of these
IPC::SocketPair() calls fails.

This particular part is gated on uses_nonsfi_mode_, so it's not exposed to the
web.

Maybe you could just locally free the first socketpairs if a later
IPC::SocketPair() call fails, using a scoped-something?

[hidehiko, 2015/05/12 18:47:51]

Ok, done.

+                         &params.renderer_client_ppapi_fd.fd) ||
+        !IPC::SocketPair(&params.manifest_service_server_fd.fd,
+                         &params.manifest_service_client_fd.fd) ||
+        !IPC::SocketPair(&params.trusted_channel_server_fd.fd,
+                         &params.trusted_channel_client_fd.fd)) {
+      return false;
+    }
+    params.browser_server_ppapi_fd.auto_close = true;
+    params.browser_client_ppapi_fd.auto_close = true;

[Mark Seaborn, 2015/04/15 20:52:05]

Notice how the NaCl loader process is receiving these client FDs and then
immediately sending them back to the browser process via
NaClProcessHostMsg_PpapiChannelsCreated.

You could avoid that IPC round trip by calling OnPpapiChannelsCreated() here, in
StartNaClExecution().  Then you only need to send 4 FDs instead of 8.  That
ought to reduce startup time a little, and reduce the time during which the
renderer is blocked.  It would also simplify the logic of the code.

[hidehiko, 2015/05/12 18:47:51]

Done.

+    params.renderer_server_ppapi_fd.auto_close = true;
+    params.renderer_client_ppapi_fd.auto_close = true;
+    params.manifest_service_server_fd.auto_close = true;
+    params.manifest_service_client_fd.auto_close = true;
+    params.trusted_channel_server_fd.auto_close = true;
+    params.trusted_channel_client_fd.auto_close = true;
+  }
+#endif
+
   params.process_type = process_type_;
   bool enable_nacl_debug = enable_debug_stub_ &&
       NaClBrowser::GetDelegate()->URLMatchesDebugPatterns(manifest_url_);
   if (uses_nonsfi_mode_) {
     // Currently, non-SFI mode is supported only on Linux.
 #if defined(OS_LINUX)
     // In non-SFI mode, we do not use SRPC. Make sure that the socketpair is
     // not created.
     DCHECK(!socket_for_sel_ldr_.IsValid());
 #endif
@@ skipping 374 matching lines @@
         process.Pass(), info,
         base::MessageLoopProxy::current(),
         base::Bind(&NaClProcessHost::OnDebugExceptionHandlerLaunchedByBroker,
                    weak_factory_.GetWeakPtr()));
     return true;
   }
 }
 #endif
 
 }  // namespace nacl