Connect PPAPI IPC channels for non-SFI mode.

ppapi/proxy/plugin_main.cc

Index: ppapi/proxy/plugin_main.cc
diff --git a/ppapi/proxy/plugin_main_nacl.cc b/ppapi/proxy/plugin_main.cc
similarity index 85%
rename from ppapi/proxy/plugin_main_nacl.cc
rename to ppapi/proxy/plugin_main.cc
index 1677cd69ebfb0bbf798a2bbc053847ae69bd3760..ce7db7b01f7440b7086636ff5b01f82c3dc451c8 100644
--- a/ppapi/proxy/plugin_main_nacl.cc
+++ b/ppapi/proxy/plugin_main.cc
@@ -11,16 +11,16 @@
 // IPC_MESSAGE_MACROS_LOG_ENABLED so ppapi_messages.h will generate the
 // ViewMsgLog et al. functions.
 
+#include "base/at_exit.h"
 #include "base/command_line.h"
+#include "base/lazy_instance.h"
 #include "base/message_loop/message_loop.h"
 #include "base/strings/string_number_conversions.h"
 #include "base/synchronization/waitable_event.h"
 #include "base/threading/thread.h"
-#include "components/tracing/child_trace_message_filter.h"
 #include "ipc/ipc_channel_handle.h"
 #include "ipc/ipc_logging.h"
 #include "ipc/ipc_message.h"
-#include "native_client/src/shared/srpc/nacl_srpc.h"
 #include "native_client/src/untrusted/irt/irt_ppapi.h"
 #include "ppapi/c/ppp.h"
 #include "ppapi/c/ppp_instance.h"
@@ -33,6 +33,11 @@
 #include "ppapi/shared_impl/ppapi_switches.h"
 #include "ppapi/shared_impl/ppb_audio_shared.h"
 
+#if defined(__native_client__)
+#include "components/tracing/child_trace_message_filter.h"
+#include "native_client/src/shared/srpc/nacl_srpc.h"
+#endif
+
 #if defined(IPC_MESSAGE_LOG_ENABLED)
 #include "base/containers/hash_tables.h"
 
@@ -54,6 +59,45 @@ using ppapi::proxy::PluginProxyDelegate;
 using ppapi::proxy::ProxyChannel;
 using ppapi::proxy::SerializedHandle;
 
+namespace ppapi {
+namespace proxy {
+namespace {
+
+// On non-SFI mode, the FDs of IPC channels are different from the hard coded
+// ones. These values will be overwritten by SetIPCFileDescriptors() below.
+// The first FD (based on NACL_CHROME_DESC_BASE) is the IPC channel to the
+// browser, and the second one is to the renderer.
+int g_nacl_ipc_browser_fd = NACL_CHROME_DESC_BASE;
+int g_nacl_ipc_renderer_fd = NACL_CHROME_DESC_BASE + 1;
+
+// We cannot use the default traits for LazyInstance for the
+// base::WaitableEvent, because it doesn't have the default constructor.
+struct WaitableEventLazyInstanceTraits
+    : public base::DefaultLazyInstanceTraits<base::WaitableEvent> {
+  static base::WaitableEvent* New(void* instance) {
+    return new (instance) base::WaitableEvent(true, false);
+  }
+};
+base::LazyInstance<base::WaitableEvent, WaitableEventLazyInstanceTraits>
+    g_ppapi_start_event = LAZY_INSTANCE_INITIALIZER;
+
+}  // namespace
+
+// For non-SFI mode, the browser_ipc_fd and renderer_ipc_fd are different from
+// the hard-coded numbers. So, this overwrites the values directly.
+void SetIPCFileDescriptors(int ipc_browser_fd, int ipc_renderer_fd) {
+  g_nacl_ipc_browser_fd = ipc_browser_fd;
+  g_nacl_ipc_renderer_fd = ipc_renderer_fd;
+}
+
+void WaitForPpapiStartEvent() {
+  g_ppapi_start_event.Pointer()->Wait();
+}
+
+}  // namespace proxy
+}  // namespace ppapi
+
+
 namespace {
 
 // This class manages communication between the plugin and the browser, and
@@ -110,15 +154,18 @@ PpapiDispatcher::PpapiDispatcher(scoped_refptr<base::MessageLoopProxy> io_loop)
   // The first FD (based on NACL_CHROME_DESC_BASE) is the IPC channel to the
   // browser.
   IPC::ChannelHandle channel_handle(
-      "NaCl IPC", base::FileDescriptor(NACL_CHROME_DESC_BASE, false));
+      "NaCl IPC",
+      base::FileDescriptor(ppapi::proxy::g_nacl_ipc_browser_fd, false));
   // We don't have/need a PID since handle sharing happens outside of the
   // NaCl sandbox.
   InitWithChannel(this, base::kNullProcessId, channel_handle,
                   false);  // Channel is server.
   channel()->AddFilter(new ppapi::proxy::PluginMessageFilter(
       NULL, PluginGlobals::Get()->resource_reply_thread_registrar()));
+#if defined(__native_client_)

[Mark Seaborn, 2014/02/07 23:54:10]

Can you comment why this is conditional?

[hidehiko, 2014/02/10 08:18:34]

Oops sorry, I don't think we need this ifdef guard (this is just a junk from my
test code).
I'll remove this from the code-review CL.

   channel()->AddFilter(
       new tracing::ChildTraceMessageFilter(message_loop_.get()));
+#endif
 }
 
 base::MessageLoopProxy* PpapiDispatcher::GetIPCMessageLoop() {
@@ -235,7 +282,8 @@ void PpapiDispatcher::OnMsgInitializeNaClDispatcher(
   // The second FD (based on NACL_CHROME_DESC_BASE) is the IPC channel to the
   // renderer.
   IPC::ChannelHandle channel_handle(
-      "nacl", base::FileDescriptor(NACL_CHROME_DESC_BASE + 1, false));
+      "nacl",
+      base::FileDescriptor(ppapi::proxy::g_nacl_ipc_renderer_fd, false));
   if (!dispatcher->InitPluginWithChannel(this, base::kNullProcessId,
                                          channel_handle, false)) {
     delete dispatcher;
@@ -276,22 +324,27 @@ void PpapiDispatcher::SetPpapiKeepAliveThrottleFromCommandLine() {
 
 void PpapiPluginRegisterThreadCreator(
     const struct PP_ThreadFunctions* thread_functions) {
+#if defined(__native_client__)

[Mark Seaborn, 2014/02/07 23:54:10]

Can you add a comment explaining why, with a TODO?

[hidehiko, 2014/02/10 08:18:34]

This is just not yet implemented on non-SFI mode. Will add TODO.

   // Initialize all classes that need to create threads that call back into
   // user code.
   ppapi::PPB_Audio_Shared::SetThreadFunctions(thread_functions);
+#endif
 }
 
 int PpapiPluginMain() {
   // Though it isn't referenced here, we must instantiate an AtExitManager.
   base::AtExitManager exit_manager;
   base::MessageLoop loop;
+#if defined(IPC_MESSAGE_LOG_ENABLED)
   IPC::Logging::set_log_function_map(&g_log_function_mapping);
+#endif
   ppapi::proxy::PluginGlobals plugin_globals;
   base::Thread io_thread("Chrome_NaClIOThread");
   base::Thread::Options options;
   options.message_loop_type = base::MessageLoop::TYPE_IO;
   io_thread.StartWithOptions(options);
 
+#if defined(__native_client__)

[Mark Seaborn, 2014/02/07 23:54:10]

Can you add a comment saying why this is conditional?

[hidehiko, 2014/02/10 08:18:34]

IIUC, this SRPC is for open_resource, and it will be replaced by IPC channels.
Is it correct?
Also, if so, can we postpone to work on re-enabling SRPC server for now with
TODO?

[Mark Seaborn, 2014/02/10 18:57:35]

Actually, the SRPC server launched by this call probably isn't used at all. 
(open_resource() is handled by different code.)
Yes, of course.  I was just asking for a comment in the code explaining why the
conditional is there...

[hidehiko, 2014/02/12 15:00:38]

Done.

   // Start up the SRPC server on another thread. Otherwise, when it blocks
   // on an RPC, the PPAPI proxy will hang. Do this before we initialize the
   // module and start the PPAPI proxy so that the NaCl plugin can continue
@@ -300,10 +353,13 @@ int PpapiPluginMain() {
   if (!NaClSrpcAcceptClientOnThread(srpc_methods)) {
     return 1;
   }
+#endif
 
   PpapiDispatcher ppapi_dispatcher(io_thread.message_loop_proxy());
   plugin_globals.set_plugin_proxy_delegate(&ppapi_dispatcher);
 
+  ppapi::proxy::g_ppapi_start_event.Pointer()->Signal();

[Mark Seaborn, 2014/02/07 23:54:10]

As I commented on https://codereview.chromium.org/139993009/, this shouldn't be
necessary.  load_module can return immediately.  It doesn't have to wait until
user code calls ppapi_start() before returning.

[hidehiko, 2014/02/10 08:18:34]

I think there is a circular dependency again here.
If we return load-module SRPC immediately, as we already created the socket pair
and the client-side FD is already passed to the browser/renderer, they "can"
send a message to the plugin, even before here. (It'd be queued). IIUC, actually
the renderer sends an IPC to the plugin just after load-module (and following
procedures I skipped by "#if 0" guard) is done.

Currently, Dispatchers creates the IPC::SyncChannel, and in its ctor, it starts
to listen the channel on IO thread. This could happen before adding some filters
(timing issue), so that some initial messages would be wrongly handled. Can we
change the initialization order inside Dispatchers, instead? Otherwise, it is
necessary to wait for the Dispatcher initialization completion.

WDYT?

[Mark Seaborn, 2014/02/10 18:57:35]

That should be OK because, as you say, the message is queued...
Are you speculating, or have you observed this to be a problem?  e.g. Is there a
problem with startup if you insert a sleep in a place that would make the race
wider?

I would guess there isn't a problem, because:
 1) the message can arrive early currently under NaCl and this presumably works;
 2) I'd guess channel()->AddFilter() sets a filter that will be used by the
current thread after the current thread returns to the event loop and receives
the message, so it doesn't matter if the IO thread has already received the
message when AddFilter() is called.

But I could be wrong about (2) (since AddFilter() claims a lock), in which case
I'd be wrong about (1) as well.

[hidehiko, 2014/02/12 15:00:38]

I still think we have the problem as I wrote in the previous reply.
However, I found that my fix was wrong (we need to wait until the Listener for
the IPC channel between the plugin and the renderer is initialized).

Also, we still have an option to change some initialization of IPC channel
filters, I think. WDYT?

- hidehiko

+
   loop.Run();
 
   return 0;