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

components/nacl/browser/nacl_process_host.cc

Index: components/nacl/browser/nacl_process_host.cc
diff --git a/components/nacl/browser/nacl_process_host.cc b/components/nacl/browser/nacl_process_host.cc
index 033502986c932dc6c41873e3eb18163df66670dd..624d2180c08a4a883e23742cb6e89210df8febc8 100644
--- a/components/nacl/browser/nacl_process_host.cc
+++ b/components/nacl/browser/nacl_process_host.cc
@@ -821,6 +821,30 @@ bool NaClProcessHost::StartNaClExecution() {
 
   // 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_);