Connect PPAPI IPC channels for non-SFI mode.

ppapi/proxy/plugin_main.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.
 

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

Can you keep this file named as plugin_main_nacl.cc?

I'd say Non-SFI Mode is still NaCl -- most of the code is in components/nacl/
already.

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

We need to remove _nacl.cc suffix, because we need to include the file into
non-unstrusted lib. Note that files with _nacl.cc suffix will be automatically
removed from the sources list, if the lib is not for untrusted.

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

OK, I see.  Can you explain that in the commit message, please?

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

Done.

 #include <map>
 #include <set>
 
 #include "build/build_config.h"
 // Need to include this before most other files because it defines
 // IPC_MESSAGE_LOG_ENABLED. We need to use it to define
 // 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"
 #include "ppapi/native_client/src/shared/ppapi_proxy/ppruntime.h"
 #include "ppapi/proxy/plugin_dispatcher.h"
 #include "ppapi/proxy/plugin_globals.h"
 #include "ppapi/proxy/plugin_message_filter.h"
 #include "ppapi/proxy/plugin_proxy_delegate.h"
 #include "ppapi/proxy/resource_reply_thread_registrar.h"
 #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"
 
 LogFunctionMap g_log_function_mapping;
 
 #define IPC_MESSAGE_MACROS_LOG_ENABLED
 #define IPC_LOG_TABLE_ADD_ENTRY(msg_id, logger) \
   g_log_function_mapping[msg_id] = logger
 
 #endif
 #include "ppapi/proxy/ppapi_messages.h"
 
 // This value must be sync'ed with the one defined in the sel_main_chrome.h.
 #define NACL_CHROME_DESC_BASE 6
 
 using ppapi::proxy::PluginDispatcher;
 using ppapi::proxy::PluginGlobals;
 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
 // manages the PluginDispatcher instances for communication between the plugin
 // and the renderer.
 class PpapiDispatcher : public ProxyChannel,
                         public PluginDispatcher::PluginDelegate,
                         public PluginProxyDelegate {
  public:
   explicit PpapiDispatcher(scoped_refptr<base::MessageLoopProxy> io_loop);
@@ skipping 36 matching lines @@
   base::WaitableEvent shutdown_event_;
 };
 
 PpapiDispatcher::PpapiDispatcher(scoped_refptr<base::MessageLoopProxy> io_loop)
     : next_plugin_dispatcher_id_(0),
       message_loop_(io_loop),
       shutdown_event_(true, false) {
   // 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() {
   return message_loop_.get();
 }
 
 base::WaitableEvent* PpapiDispatcher::GetShutdownEvent() {
   return &shutdown_event_;
 }
 
@@ skipping 96 matching lines @@
   if (error)
     ::exit(error);
 
   PluginDispatcher* dispatcher =
       new PluginDispatcher(::PPP_GetInterface, args.permissions,
                            args.off_the_record);
   // The channel handle's true name is not revealed here.
   // 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;
     return;
   }
   // From here, the dispatcher will manage its own lifetime according to the
   // lifetime of the attached channel.
 }
 
 void PpapiDispatcher::OnPluginDispatcherMessageReceived(
@@ skipping 20 matching lines @@
     ppapi::proxy::PluginGlobals::Get()->
         set_keepalive_throttle_interval_milliseconds(
             keepalive_throttle_interval_milliseconds);
   }
 }
 
 }  // namespace
 
 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
   // loading the app.
   static struct NaClSrpcHandlerDesc srpc_methods[] = { { NULL, NULL } };
   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;
 }