Add an IPC channel between the NaCl loader process and the renderer.

Add an IPC channel between the NaCl loader process and the renderer.
BUG=116317
TEST=manual

[bbudge, 2012-04-25 00:28:05]

This is just work in progress. It opens an IPC::Channel in the sel_ldr process
given a channel name from the browser.

Next steps:
1) add a preprocessor flag so it can be landed.
2) try to connect from the renderer side.
3) send a message!

[Mark Seaborn, 2012-04-25 00:40:39]

http://codereview.chromium.org/10214007/diff/1/chrome/browser/nacl_host/nacl_...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/10214007/diff/1/chrome/browser/nacl_host/nacl_...
chrome/browser/nacl_host/nacl_process_host.cc:412: std::string channel_name;
This doesn't need to go in NaClInternal.  You can put it directly in
NaClProcessHost.

The header says:

  // Internal class that holds the nacl::Handle objecs so that
  // nacl_process_host.h doesn't include NaCl headers.  Needed since it's
  // included by src\content, which can't depend on the NaCl gyp file because it
  // depends on chrome.gyp (circular dependency).
  struct NaClInternal;

[bbudge, 2012-04-25 00:50:04]

http://codereview.chromium.org/10214007/diff/1/chrome/browser/nacl_host/nacl_...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/10214007/diff/1/chrome/browser/nacl_host/nacl_...
chrome/browser/nacl_host/nacl_process_host.cc:412: std::string channel_name;
Yes, I'll remove it from this struct.

http://codereview.chromium.org/10214007/diff/1/chrome/browser/nacl_host/nacl_...
chrome/browser/nacl_host/nacl_process_host.cc:521:
IPC::Channel::GenerateVerifiedChannelID(std::string());
This bit of code doesn't seem to work, in that the named pipe (on Windows) can't
be created in nacl_listener. Experimenting, I mimicked the name that nacl_imc
uses for it's socket pairs and that worked. It's surprising that this should be
sensitive to the name string.

[Mark Seaborn, 2012-04-25 00:57:39]

On 24 April 2012 17:50, <bbudge@chromium.org> wrote:

> http://codereview.chromium.**org/10214007/diff/1/chrome/**
>
browser/nacl_host/nacl_**process_host.cc#newcode521<http://codereview.chromium.org/10214007/diff/1/chrome/browser/nacl_host/nacl_process_host.cc#newcode521>
> chrome/browser/nacl_host/nacl_**process_host.cc:521:
> IPC::Channel::**GenerateVerifiedChannelID(std:**:string());
> This bit of code doesn't seem to work, in that the named pipe (on
> Windows) can't be created in nacl_listener. Experimenting, I mimicked
> the name that nacl_imc uses for it's socket pairs and that worked. It's
> surprising that this should be sensitive to the name string.
>

If the pipe name matters, it's probably the Windows sandbox restricting
what can be opened.  See content/common/sandbox_policy.cc.

Mark

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To post to this group, send email to native-client-reviews@googlegroups.com.
To unsubscribe from this group, send email to
native-client-reviews+unsubscribe@googlegroups.com.
For more options, visit this group at
http://groups.google.com/group/native-client-reviews?hl=en.

[Mark Seaborn, 2012-04-25 00:57:40]

On 24 April 2012 17:50, <bbudge@chromium.org> wrote:

> http://codereview.chromium.**org/10214007/diff/1/chrome/**
>
browser/nacl_host/nacl_**process_host.cc#newcode521<http://codereview.chromium.org/10214007/diff/1/chrome/browser/nacl_host/nacl_process_host.cc#newcode521>
> chrome/browser/nacl_host/nacl_**process_host.cc:521:
> IPC::Channel::**GenerateVerifiedChannelID(std:**:string());
> This bit of code doesn't seem to work, in that the named pipe (on
> Windows) can't be created in nacl_listener. Experimenting, I mimicked
> the name that nacl_imc uses for it's socket pairs and that worked. It's
> surprising that this should be sensitive to the name string.
>

If the pipe name matters, it's probably the Windows sandbox restricting
what can be opened.  See content/common/sandbox_policy.cc.

Mark

[bbudge, 2012-04-25 01:03:31]

> 
> If the pipe name matters, it's probably the Windows sandbox restricting
> what can be opened.  See content/common/sandbox_policy.cc.
> 
> Mark

Yes, the name must start with 'chrome.nacl'. Thanks.

http://code.google.com/codesearch#OAMlx_jo-ck/src/content/common/sandbox_poli...

[dmichael (off chromium), 2012-04-25 21:06:00]

https://chromiumcodereview.appspot.com/10214007/diff/1/chrome/nacl/nacl_liste...
File chrome/nacl/nacl_listener.cc (right):

https://chromiumcodereview.appspot.com/10214007/diff/1/chrome/nacl/nacl_liste...
chrome/nacl/nacl_listener.cc:186: ipc_channel->Init(params.channel_name,
IPC::Channel::MODE_SERVER, true);
You're not doing anything with ipc_channel yet. Is that intentional? If so,
maybe it would be good to have a TODO here?

[bbudge, 2012-04-25 23:52:58]

Yep, that'll be next. This update gets the channel name into the renderer where
I verified I can open the client side. Tomorrow I hope to get this over to
PepperPluginDelegateImpl so we can set up the proxy.

> You're not doing anything with ipc_channel yet. Is that intentional? If so,
> maybe it would be good to have a TODO here?

[bbudge, 2012-04-26 01:22:01]

On 2012/04/25 23:53:02, bbudge1 wrote:

I'm thinking of changing the way I report back to the renderer. By adding a
param to the LaunchNaCl msg, I'm painted into a corner because the receiving
code can't do anything with it. I think I need another message to the
RenderView, which can pass it to PepperPluginDelegateImpl, which already has
code to create the HostDispatcher.

[bbudge, 2012-04-26 22:58:24]

This patch adds the renderer side setup. If everything is working, both sides of
the IPC channel and the renderer side of the proxy should be set up.

We need a NaCl repository change, so the NaCl plugin can pass the PP_Instance
when launching sel_ldr. This is intercepted by the code in
chrome/renderer/chrome_ppapi_interfaces.cc and used to invoke code in
PepperPluginDelegateImpl where all the action is. Most of the rest is just
plumbing.

Mark is looking at moving the NaCl repository code into Chrome so we can avoid a
complex landing.

[dmichael (off chromium), 2012-04-27 17:14:36]

Since I'm still learning the architecture of all this, my opinion shouldn't
count for much. But it lgtm.

https://chromiumcodereview.appspot.com/10214007/diff/23001/content/public/ren...
File content/public/renderer/render_view.h (right):

https://chromiumcodereview.appspot.com/10214007/diff/23001/content/public/ren...
content/public/renderer/render_view.h:135: virtual void CreateHostDispatcher(
It would probably make sense to make the name clearer, like... 
CreatePepperHostDispatcher, or CreatePluginHostDispatcher?

https://chromiumcodereview.appspot.com/10214007/diff/23001/content/public/ren...
content/public/renderer/render_view.h:139: const std::string&
channel_handle_name) = 0;
linter is pointing out you should include string...  (there's no
standards-conforming way to forward reference it afaik)

https://chromiumcodereview.appspot.com/10214007/diff/23001/ppapi/c/private/pp...
File ppapi/c/private/ppb_nacl_private.h (right):

https://chromiumcodereview.appspot.com/10214007/diff/23001/ppapi/c/private/pp...
ppapi/c/private/ppb_nacl_private.h:21: bool (*LaunchSelLdr)(int instance,
It would be clearer to use PP_Instance, since that's what it is interpreted as.

https://chromiumcodereview.appspot.com/10214007/diff/23001/ppapi/native_clien...
File ppapi/native_client/src/trusted/plugin/nacl_entry_points.h (right):

https://chromiumcodereview.appspot.com/10214007/diff/23001/ppapi/native_clien...
ppapi/native_client/src/trusted/plugin/nacl_entry_points.h:44: typedef bool
(*LaunchNaClProcessFunc)(int instance, const char* url,
You included PP_Instance; did you mean for that to be the param type here?

[bbudge, 2012-04-27 17:46:56]

http://codereview.chromium.org/10214007/diff/23001/content/public/renderer/re...
File content/public/renderer/render_view.h (right):

http://codereview.chromium.org/10214007/diff/23001/content/public/renderer/re...
content/public/renderer/render_view.h:135: virtual void CreateHostDispatcher(
On 2012/04/27 17:14:36, dmichael wrote:
> It would probably make sense to make the name clearer, like... 
> CreatePepperHostDispatcher, or CreatePluginHostDispatcher?

Done.

http://codereview.chromium.org/10214007/diff/23001/content/public/renderer/re...
content/public/renderer/render_view.h:139: const std::string&
channel_handle_name) = 0;
On 2012/04/27 17:14:36, dmichael wrote:
> linter is pointing out you should include string...  (there's no
> standards-conforming way to forward reference it afaik)

Done.

http://codereview.chromium.org/10214007/diff/23001/ppapi/c/private/ppb_nacl_p...
File ppapi/c/private/ppb_nacl_private.h (right):

http://codereview.chromium.org/10214007/diff/23001/ppapi/c/private/ppb_nacl_p...
ppapi/c/private/ppb_nacl_private.h:21: bool (*LaunchSelLdr)(int instance,
Yeah, this is supposed to be PP_Instance. Mark Seaborn is moving some stuff from
the NaCl repo which made it easier to use an int.
On 2012/04/27 17:14:36, dmichael wrote:
> It would be clearer to use PP_Instance, since that's what it is interpreted
as.

http://codereview.chromium.org/10214007/diff/23001/ppapi/native_client/src/tr...
File ppapi/native_client/src/trusted/plugin/nacl_entry_points.h (right):

http://codereview.chromium.org/10214007/diff/23001/ppapi/native_client/src/tr...
ppapi/native_client/src/trusted/plugin/nacl_entry_points.h:44: typedef bool
(*LaunchNaClProcessFunc)(int instance, const char* url,
Yep. This will be PP_Instance after Mark makes his changes.
On 2012/04/27 17:14:36, dmichael wrote:
> You included PP_Instance; did you mean for that to be the param type here?

[bbudge, 2012-04-27 23:53:37]

This update adds a notification event from the NaCl process (sel_ldr) that the
server side of the IPC channel has been created. The renderer is able to connect
without error.

On refreshing the page I get a crash when the plugin_instance Var is deref'd -
because it isn't in the VarTracker.

I should still verify that I can send a message.

[bbudge, 2012-04-30 17:21:42]

Refresh patch after 10180015:
http://codereview.chromium.org/10180015/

[bbudge, 2012-05-02 23:45:02]

Modify this to conform to Mark's latest changes, and to start the proxy after
the Nexe is ready to be started.

[dmichael (off chromium), 2012-05-04 22:51:42]

https://chromiumcodereview.appspot.com/10214007/diff/52002/chrome/browser/nac...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://chromiumcodereview.appspot.com/10214007/diff/52002/chrome/browser/nac...
chrome/browser/nacl_host/nacl_process_host.cc:941: const ChildProcessData& data
= process_->GetData();
Why did you move this farther from where it's first used? Did you mean to use it
on 949?

https://chromiumcodereview.appspot.com/10214007/diff/52002/chrome/nacl/nacl_l...
File chrome/nacl/nacl_listener.cc (right):

https://chromiumcodereview.appspot.com/10214007/diff/52002/chrome/nacl/nacl_l...
chrome/nacl/nacl_listener.cc:197: static MyListener test_listener;
Does it make sense for this to be a file-scope static? Can't it just be a member
of NaClListener?

(In any case, it definitely has to change...  we don't allow static initializers
because too many of them will kill startup speed.)

https://chromiumcodereview.appspot.com/10214007/diff/52002/chrome/renderer/pe...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

https://chromiumcodereview.appspot.com/10214007/diff/52002/chrome/renderer/pe...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:38: #include
"webkit/plugins/ppapi/ppapi_plugin_instance.h"
These includes should probably be merged with those above, right?

https://chromiumcodereview.appspot.com/10214007/diff/52002/ppapi/native_clien...
File ppapi/native_client/src/trusted/plugin/nacl_entry_points.h (right):

https://chromiumcodereview.appspot.com/10214007/diff/52002/ppapi/native_clien...
ppapi/native_client/src/trusted/plugin/nacl_entry_points.h:19: typedef bool
(*LaunchNaClProcessFunc)(int instance_id,
I'm still not clear on why you can't just use PP_Instance here, since you have
the include above...

couldn't you avoid the reinterpret_cast<> in module_ppapi.cc?

https://chromiumcodereview.appspot.com/10214007/diff/52002/ppapi/native_clien...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://chromiumcodereview.appspot.com/10214007/diff/52002/ppapi/native_clien...
ppapi/native_client/src/trusted/plugin/plugin.cc:1219: if (start_ppapi_proxy &&
start_ppapi_proxy(pp_instance())) {
We want start_ppapi_proxy to not be called by default, right? (Maybe just make
it NULL in the browser side, enabled with conditional compilation or something
for now). We have to make sure to get that bit right before check in, at least,
though I'm guessing you're just exploring how to make it work before worrying
about making sure it's off.

[bbudge, 2012-05-07 18:08:52]

Replaced IPC::Channel in sel_ldr process with Brett's NaClIPCManager. I still
need to connect the NaClIPCAdapters to the untrusted Channels.

http://codereview.chromium.org/10214007/diff/52002/chrome/browser/nacl_host/n...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/10214007/diff/52002/chrome/browser/nacl_host/n...
chrome/browser/nacl_host/nacl_process_host.cc:941: const ChildProcessData& data
= process_->GetData();
On 2012/05/04 22:51:42, dmichael wrote:
> Why did you move this farther from where it's first used? Did you mean to use
it
> on 949?

Done.

http://codereview.chromium.org/10214007/diff/52002/chrome/nacl/nacl_listener.cc
File chrome/nacl/nacl_listener.cc (right):

http://codereview.chromium.org/10214007/diff/52002/chrome/nacl/nacl_listener....
chrome/nacl/nacl_listener.cc:197: static MyListener test_listener;
This is just experimental hacking to verify that the channel is working.
Removed.
On 2012/05/04 22:51:42, dmichael wrote:
> Does it make sense for this to be a file-scope static? Can't it just be a
member
> of NaClListener?
> 
> (In any case, it definitely has to change...  we don't allow static
initializers
> because too many of them will kill startup speed.)

http://codereview.chromium.org/10214007/diff/52002/ppapi/native_client/src/tr...
File ppapi/native_client/src/trusted/plugin/nacl_entry_points.h (right):

http://codereview.chromium.org/10214007/diff/52002/ppapi/native_client/src/tr...
ppapi/native_client/src/trusted/plugin/nacl_entry_points.h:19: typedef bool
(*LaunchNaClProcessFunc)(int instance_id,
This is a vestige of back when this was in the NaCl repo. Done.
On 2012/05/04 22:51:42, dmichael wrote:
> I'm still not clear on why you can't just use PP_Instance here, since you have
> the include above...
> 
> couldn't you avoid the reinterpret_cast<> in module_ppapi.cc?

http://codereview.chromium.org/10214007/diff/52002/ppapi/native_client/src/tr...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

http://codereview.chromium.org/10214007/diff/52002/ppapi/native_client/src/tr...
ppapi/native_client/src/trusted/plugin/plugin.cc:1219: if (start_ppapi_proxy &&
start_ppapi_proxy(pp_instance())) {
That's correct, we need a switch before this can land.
On 2012/05/04 22:51:42, dmichael wrote:
> We want start_ppapi_proxy to not be called by default, right? (Maybe just make
> it NULL in the browser side, enabled with conditional compilation or something
> for now). We have to make sure to get that bit right before check in, at
least,
> though I'm guessing you're just exploring how to make it work before worrying
> about making sure it's off.

http://codereview.chromium.org/10214007/diff/74005/chrome/renderer/pepper/ppb...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

http://codereview.chromium.org/10214007/diff/74005/chrome/renderer/pepper/ppb...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:44: using WebKit::WebView;
I will get these sorted and pruned down to just what's needed before landing.

http://codereview.chromium.org/10214007/diff/74005/chrome/renderer/pepper/ppb...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:53: base::ProcessId
g_plugin_process_id;
I need to figure out what to do with these globals. This is just experimental as
jschuh's work will likely change this too.

[dmichael (off chromium), 2012-05-07 20:30:19]

https://chromiumcodereview.appspot.com/10214007/diff/74005/chrome/nacl/nacl_i...
File chrome/nacl/nacl_ipc_adapter.cc (right):

https://chromiumcodereview.appspot.com/10214007/diff/74005/chrome/nacl/nacl_i...
chrome/nacl/nacl_ipc_adapter.cc:122:
DCHECK(io_thread_data_.channel_->Connect());
You don't want to do this inside DCHECK, since it will only run in debug builds.

https://chromiumcodereview.appspot.com/10214007/diff/74005/chrome/nacl/nacl_i...
File chrome/nacl/nacl_ipc_manager.h (right):

https://chromiumcodereview.appspot.com/10214007/diff/74005/chrome/nacl/nacl_i...
chrome/nacl/nacl_ipc_manager.h:13: #include "base/threading/thread.h"
Should you instead be adding base/message_loop_proxy.h?

[bbudge, 2012-05-07 21:39:17]

Moved some files into a separate CL for NaClIPCManager/Adapter.

http://codereview.chromium.org/10214007/diff/74005/chrome/nacl/nacl_ipc_adapt...
File chrome/nacl/nacl_ipc_adapter.cc (right):

http://codereview.chromium.org/10214007/diff/74005/chrome/nacl/nacl_ipc_adapt...
chrome/nacl/nacl_ipc_adapter.cc:122:
DCHECK(io_thread_data_.channel_->Connect());
I think that's OK. I've seen it in other calls to connect. I don't think there's
much we can do if it fails.
On 2012/05/07 20:30:19, dmichael wrote:
> You don't want to do this inside DCHECK, since it will only run in debug
builds.

http://codereview.chromium.org/10214007/diff/74005/chrome/nacl/nacl_ipc_manag...
File chrome/nacl/nacl_ipc_manager.h (right):

http://codereview.chromium.org/10214007/diff/74005/chrome/nacl/nacl_ipc_manag...
chrome/nacl/nacl_ipc_manager.h:13: #include "base/threading/thread.h"
On 2012/05/07 20:30:19, dmichael wrote:
> Should you instead be adding base/message_loop_proxy.h?

Done.

[bbudge, 2012-05-09 18:29:34]

Added runtime flag to enable IPC Proxy code.

[dmichael (off chromium), 2012-05-10 19:38:58]

https://chromiumcodereview.appspot.com/10214007/diff/83003/chrome/browser/nac...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

https://chromiumcodereview.appspot.com/10214007/diff/83003/chrome/browser/nac...
chrome/browser/nacl_host/nacl_process_host.cc:841:
switches::kEnableNaClIPCProxy)) {
I think you forgot to add the file where you defined this to your CL.

https://chromiumcodereview.appspot.com/10214007/diff/83003/chrome/nacl/nacl_l...
File chrome/nacl/nacl_listener.cc (right):

https://chromiumcodereview.appspot.com/10214007/diff/83003/chrome/nacl/nacl_l...
chrome/nacl/nacl_listener.cc:208: // can reply to the renderer, which will then
try to connect as client.
heh...  I think I'm going to need to draw a sequence diagram of this.

https://chromiumcodereview.appspot.com/10214007/diff/83003/chrome/nacl/nacl_l...
File chrome/nacl/nacl_listener.h (right):

https://chromiumcodereview.appspot.com/10214007/diff/83003/chrome/nacl/nacl_l...
chrome/nacl/nacl_listener.h:51: NaClIPCManager nacl_ipc_manager_;
I think this will be scoped_refptr<NaCLIPCAdapter> now.

https://chromiumcodereview.appspot.com/10214007/diff/83003/chrome/nacl/nacl_l...
chrome/nacl/nacl_listener.h:51: NaClIPCManager nacl_ipc_manager_;
I think this will be scoped_refptr<NaCLIPCAdapter> now.

https://chromiumcodereview.appspot.com/10214007/diff/83003/chrome/nacl/nacl_l...
chrome/nacl/nacl_listener.h:52: void* nacl_ppapi_channel_;
And this is either NaClDesc*, or you might not need to save it at all... we have
to figure out what you do with the NaClDesc :-)

https://chromiumcodereview.appspot.com/10214007/diff/83003/chrome/renderer/pe...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

https://chromiumcodereview.appspot.com/10214007/diff/83003/chrome/renderer/pe...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:46: IPC::ChannelHandle
g_plugin_ppapi_channel;
I think this one will have to be a LazyInstance if it stays global. The next two
are probably okay as-is.

https://chromiumcodereview.appspot.com/10214007/diff/83003/chrome/renderer/pe...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:48: base::ProcessId
g_plugin_process_id;
Suppose one page has >1 NaCl module embedded on it...  won't we need 1 of each
of these per instance? Maybe they should be out-params of LaunchSelLdr, like
imc_handles is?

https://chromiumcodereview.appspot.com/10214007/diff/83003/chrome/renderer/pe...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:76: // Using the RenderView,
establish the HostDispatcher for the PPAPI proxy.
I think this comment might be better right before you do it, instead of right
before the sanity checks.

https://chromiumcodereview.appspot.com/10214007/diff/83003/chrome/renderer/pe...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:88:
switches::kEnableNaClIPCProxy)) {
Maybe you should check for that switch at the beginning, and bail? I know that
the first half of the function doesn't do anything interesting, but putting the
switch check early will make sure we don't accidentally add some interesting
behavior outside of the switch check.

https://chromiumcodereview.appspot.com/10214007/diff/83003/content/renderer/p...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

https://chromiumcodereview.appspot.com/10214007/diff/83003/content/renderer/p...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:282:
plugin_process_handle,
nit: I think the indent is off by 1

https://chromiumcodereview.appspot.com/10214007/diff/83003/ppapi/c/private/pp...
File ppapi/c/private/ppb_nacl_private.h (right):

https://chromiumcodereview.appspot.com/10214007/diff/83003/ppapi/c/private/pp...
ppapi/c/private/ppb_nacl_private.h:27: bool (*StartPpapiProxy)(PP_Instance
instance);
Can you use PP_Bool? I know this file is already not following usual PPAPI
conventions, but I'd rather not add any more unusual things.

https://chromiumcodereview.appspot.com/10214007/diff/83003/ppapi/native_clien...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

https://chromiumcodereview.appspot.com/10214007/diff/83003/ppapi/native_clien...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:649: PP_Instance
instance = plugin_->pp_instance();
You don't appear to use instance

[bbudge, 2012-05-11 01:21:16]

Some tweaks and responses to dmichael's comments. Still not ready to land.

http://codereview.chromium.org/10214007/diff/83003/chrome/browser/nacl_host/n...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/10214007/diff/83003/chrome/browser/nacl_host/n...
chrome/browser/nacl_host/nacl_process_host.cc:841:
switches::kEnableNaClIPCProxy)) {
Separate CL. http://codereview.chromium.org/10392004/

http://codereview.chromium.org/10214007/diff/83003/chrome/nacl/nacl_listener.cc
File chrome/nacl/nacl_listener.cc (right):

http://codereview.chromium.org/10214007/diff/83003/chrome/nacl/nacl_listener....
chrome/nacl/nacl_listener.cc:208: // can reply to the renderer, which will then
try to connect as client.
I couldn't get it to work creating the channels in the browser process. I might
revisit that.

http://codereview.chromium.org/10214007/diff/83003/chrome/renderer/pepper/ppb...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

http://codereview.chromium.org/10214007/diff/83003/chrome/renderer/pepper/ppb...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:48: base::ProcessId
g_plugin_process_id;
This is just for the sake of getting something working. It will probably need to
be a map. That's why I added the PP_Instance param to LaunchSelLdr, so we can
match things up.

http://codereview.chromium.org/10214007/diff/83003/chrome/renderer/pepper/ppb...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:76: // Using the RenderView,
establish the HostDispatcher for the PPAPI proxy.
On 2012/05/10 19:38:58, dmichael wrote:
> I think this comment might be better right before you do it, instead of right
> before the sanity checks.

Done.

http://codereview.chromium.org/10214007/diff/83003/chrome/renderer/pepper/ppb...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:88:
switches::kEnableNaClIPCProxy)) {
Yes indeed.

http://codereview.chromium.org/10214007/diff/83003/content/renderer/pepper/pe...
File content/renderer/pepper/pepper_plugin_delegate_impl.cc (right):

http://codereview.chromium.org/10214007/diff/83003/content/renderer/pepper/pe...
content/renderer/pepper/pepper_plugin_delegate_impl.cc:282:
plugin_process_handle,
On 2012/05/10 19:38:58, dmichael wrote:
> nit: I think the indent is off by 1

Done.

http://codereview.chromium.org/10214007/diff/83003/ppapi/native_client/src/tr...
File ppapi/native_client/src/trusted/plugin/service_runtime.cc (right):

http://codereview.chromium.org/10214007/diff/83003/ppapi/native_client/src/tr...
ppapi/native_client/src/trusted/plugin/service_runtime.cc:649: PP_Instance
instance = plugin_->pp_instance();
On 2012/05/10 19:38:58, dmichael wrote:
> You don't appear to use instance

Done.

[Mark Seaborn, 2012-05-11 22:45:30]

FYI

https://chromiumcodereview.appspot.com/10214007/diff/69032/chrome/nacl/nacl_l...
File chrome/nacl/nacl_listener.cc (right):

https://chromiumcodereview.appspot.com/10214007/diff/69032/chrome/nacl/nacl_l...
chrome/nacl/nacl_listener.cc:206: switches::kEnableNaClIPCProxy)) {
There are two problems with this in the nacl_helper process on Linux:
 * this doesn't link (switches::kEnableNaClIPCProxy isn't defined at link time).
 * this flag does not get passed through to the nacl_helper process, so this
returns false even if I use the string "enable-nacl-ipc-proxy" directly.

[bbudge, 2012-05-15 15:41:56]

Updated to changes made by jschuh (process handles removed), dmichael
(NaClIPCManager removed), and brettw.

Added code to make channel work for OS_POSIX.

[bbudge, 2012-05-15 15:44:01]

http://codereview.chromium.org/10214007/diff/69032/chrome/nacl/nacl_listener.cc
File chrome/nacl/nacl_listener.cc (right):

http://codereview.chromium.org/10214007/diff/69032/chrome/nacl/nacl_listener....
chrome/nacl/nacl_listener.cc:206: switches::kEnableNaClIPCProxy)) {
Changed to use a bool sent from the browser.

[dmichael (off chromium), 2012-05-23 00:41:03]

http://codereview.chromium.org/10214007/diff/79076/chrome/renderer/pepper/ppb...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

http://codereview.chromium.org/10214007/diff/79076/chrome/renderer/pepper/ppb...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:166: virtual const void*
GetProxiedInterface(const char* name) {
OVERRIDE for these?

http://codereview.chromium.org/10214007/diff/79076/chrome/renderer/pepper/ppb...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:208:
plugin_module->InitAsProxied(out_of_process_proxy.release());
I think this might cause trouble...  this module has already been initialized.

One possible problem with this... If you look at "GetPluginInterface" in
webkit/plugins/ppapi/plugin_module.cc. Before this point, it will return
interface pointers for the NaCl plugin's PPP interfaces. After this point, it
will return pointers to the IPC proxy's host-side PPP interfaces. So... 
anything cached before this (like ppapi_plugin_instance.cc does in its
Load*Interface() functions) will never get sent to the IPC proxy. We'll have to
figure some strategy for this.

[dmichael (off chromium), 2012-05-23 21:24:12]

http://codereview.chromium.org/10214007/diff/79076/chrome/browser/nacl_host/n...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/10214007/diff/79076/chrome/browser/nacl_host/n...
chrome/browser/nacl_host/nacl_process_host.cc:603: 
I think you need to set params.enable_ipc_proxy here. This is why it's crashing
locally for me... it's coming out "true" always, so OnPpapiChannelCreated is
called twice.

[bbudge, 2012-05-23 21:42:19]

Refresh, fix flag passing in NaClProcessHost, install IPC fd in NaClListener.
IPC should make it into sandbox.

http://codereview.chromium.org/10214007/diff/79076/chrome/browser/nacl_host/n...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/10214007/diff/79076/chrome/browser/nacl_host/n...
chrome/browser/nacl_host/nacl_process_host.cc:603: 
On 2012/05/23 21:24:12, dmichael wrote:
> I think you need to set params.enable_ipc_proxy here. This is why it's
crashing
> locally for me... it's coming out "true" always, so OnPpapiChannelCreated is
> called twice.

Done. Thanks for tracking this down.

http://codereview.chromium.org/10214007/diff/79076/chrome/renderer/pepper/ppb...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

http://codereview.chromium.org/10214007/diff/79076/chrome/renderer/pepper/ppb...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:166: virtual const void*
GetProxiedInterface(const char* name) {
On 2012/05/23 00:41:04, dmichael wrote:
> OVERRIDE for these?

Done.

[bbudge, 2012-06-19 23:53:57]

This is finally ready for review. PTAL

To start up the IPC proxy, start Chrome with the --enable-nacl-ipc-proxy switch.
You will also need to build the IPC IRT. Ask bbudge or dmichael for details, as
not all patches have landed for that.

[bbudge, 2012-06-20 00:06:52]

Missed a file.

[bbudge, 2012-06-20 00:07:51]

Mark for nacl_process_host and nacl_listener.
Brett for renderer side.
David plugin?

[Mark Seaborn, 2012-06-20 19:24:39]

http://codereview.chromium.org/10214007/diff/121014/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (left):

http://codereview.chromium.org/10214007/diff/121014/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:559: bool
NaClProcessHost::ReplyToRenderer() {
Why do you need to merge ReplyToRenderer() and StartNaClExecution()?  When
enable_ipc_proxy is true, you can just defer the call to ReplyToRenderer() until
OnPpapiChannelCreated().  That would avoid a handle lifetime/cleanup issue
below...

http://codereview.chromium.org/10214007/diff/121014/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:607:
internal_->sockets_for_renderer.clear();
Removing this call isn't quite right.  If NaClProcessHost() gets destroyed, it
will try to close handles in sockets_for_renderer that have already been closed
(via DUPLICATE_CLOSE_SOURCE), which is unsafe.  The code isn't right before this
change, but the change increases the window in which things can go wrong.

http://codereview.chromium.org/10214007/diff/121014/chrome/common/render_mess...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/10214007/diff/121014/chrome/common/render_mess...
chrome/common/render_messages.h:496: base::ProcessId /* NaCl process id */)
See other comment about g_plugin_process_id being assigned but never read.  That
means you can remove this result parameter, right?

http://codereview.chromium.org/10214007/diff/121014/chrome/nacl/nacl_ipc_adap...
File chrome/nacl/nacl_ipc_adapter.cc (right):

http://codereview.chromium.org/10214007/diff/121014/chrome/nacl/nacl_ipc_adap...
chrome/nacl/nacl_ipc_adapter.cc:77: msg->ndesc_length = 0;  // Messages with
handles aren't supported yet.
Nit: "descriptors" is the proper term in this context, not "handles" (which is a
Windows-specific term).

http://codereview.chromium.org/10214007/diff/121014/chrome/nacl/nacl_listener.cc
File chrome/nacl/nacl_listener.cc (right):

http://codereview.chromium.org/10214007/diff/121014/chrome/nacl/nacl_listener...
chrome/nacl/nacl_listener.cc:207: #if defined (OS_POSIX)
There should be no space after 'defined'

http://codereview.chromium.org/10214007/diff/121014/chrome/renderer/pepper/pp...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

http://codereview.chromium.org/10214007/diff/121014/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:47:
base::LazyInstance<IPC::ChannelHandle> g_ipc_channel_handle =
Why is this a global variable?  A global is very likely to go wrong here.  What
needs to be changed so that this is not a global?

http://codereview.chromium.org/10214007/diff/121014/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:49: base::ProcessId
g_plugin_process_id;
This variable is assigned to but never read.

http://codereview.chromium.org/10214007/diff/121014/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/module_ppapi.cc (right):

http://codereview.chromium.org/10214007/diff/121014/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/module_ppapi.cc:16: StartPpapiProxyFunc
start_ppapi_proxy;
Does this have to be a global?  Can't plugin.cc call
GetBrowserInterface(PPB_NACL_PRIVATE_INTERFACE) itself?

http://codereview.chromium.org/10214007/diff/121014/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

http://codereview.chromium.org/10214007/diff/121014/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin.cc:615: if
(init_done_cb.pp_completion_callback().func != NULL) {
Is there is reason this isn't just
CHECK(init_done_cb.pp_completion_callback().func != NULL)?  Is it valid/expected
for func to be NULL?

http://codereview.chromium.org/10214007/diff/121014/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/plugin.h (right):

http://codereview.chromium.org/10214007/diff/121014/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin.h:521: bool started_ipc_proxy_;
Would 'using_ipc_proxy_' be more appropriate?  "started" suggests this is used
for starting something only if it's not already started.

[dmichael (off chromium), 2012-06-20 22:34:31]

http://codereview.chromium.org/10214007/diff/121014/chrome/renderer/pepper/pp...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

http://codereview.chromium.org/10214007/diff/121014/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:93: : shutdown_event_(true,
false) {}
style nit: I'd put } on the next line to be consistent.

http://codereview.chromium.org/10214007/diff/121014/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:114: // Stubbed out
SyncMessageStatusReceiver, required by HostDispatcher.
Should we add a TODO?

http://codereview.chromium.org/10214007/diff/121014/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:208:
plugin_module->InitAsProxiedNaCl(out_of_process_proxy.release(),
nit: We should probably use Pass() here and take scoped_ptr in InitAsProxiedNaCl
to clarify that it's transfer of ownership.

http://codereview.chromium.org/10214007/diff/121014/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

http://codereview.chromium.org/10214007/diff/121014/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin.cc:645: if (started_ipc_proxy_)
We should probably add a comment here about why we shortcut this method
(StartSrpcServices would fail, StartJSObjectProxy doesn't make sense in the IPC
proxy). Returning true here doesn't really make sense for the plugin's standard
usage.

[bbudge, 2012-06-21 00:00:46]

Added webkit::ppapi::PluginModule for change suggested by David. This change
also includes a fix to shutdown so we can navigate back to a page and it won't
hang.

http://codereview.chromium.org/10214007/diff/121014/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (left):

http://codereview.chromium.org/10214007/diff/121014/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:559: bool
NaClProcessHost::ReplyToRenderer() {
On 2012/06/20 19:24:40, Mark Seaborn wrote:
> Why do you need to merge ReplyToRenderer() and StartNaClExecution()?  When
> enable_ipc_proxy is true, you can just defer the call to ReplyToRenderer()
until
> OnPpapiChannelCreated().  That would avoid a handle lifetime/cleanup issue
> below...
I merged these because we now have to wait for the asynchronous reply from
sel_ldr before replying to the renderer. This was the cleanest way I could see
to do this. The non-IPC case looks a little strange but I favored the IPC proxy
case because we'll remove the old path in the near future. See my comment about
handle cleanup below.

http://codereview.chromium.org/10214007/diff/121014/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:607:
internal_->sockets_for_renderer.clear();
On 2012/06/20 19:24:40, Mark Seaborn wrote:
> Removing this call isn't quite right.  If NaClProcessHost() gets destroyed, it
> will try to close handles in sockets_for_renderer that have already been
closed
> (via DUPLICATE_CLOSE_SOURCE), which is unsafe.  The code isn't right before
this
> change, but the change increases the window in which things can go wrong.
I'm adding a TODO to fix this. I think we should clean up here at the point of
failure.

http://codereview.chromium.org/10214007/diff/121014/chrome/common/render_mess...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/10214007/diff/121014/chrome/common/render_mess...
chrome/common/render_messages.h:496: base::ProcessId /* NaCl process id */)
On 2012/06/20 19:24:40, Mark Seaborn wrote:
> See other comment about g_plugin_process_id being assigned but never read. 
That
> means you can remove this result parameter, right?

Done.

http://codereview.chromium.org/10214007/diff/121014/chrome/nacl/nacl_ipc_adap...
File chrome/nacl/nacl_ipc_adapter.cc (right):

http://codereview.chromium.org/10214007/diff/121014/chrome/nacl/nacl_ipc_adap...
chrome/nacl/nacl_ipc_adapter.cc:77: msg->ndesc_length = 0;  // Messages with
handles aren't supported yet.
On 2012/06/20 19:24:40, Mark Seaborn wrote:
> Nit: "descriptors" is the proper term in this context, not "handles" (which is
a
> Windows-specific term).

Done.

http://codereview.chromium.org/10214007/diff/121014/chrome/nacl/nacl_listener.cc
File chrome/nacl/nacl_listener.cc (right):

http://codereview.chromium.org/10214007/diff/121014/chrome/nacl/nacl_listener...
chrome/nacl/nacl_listener.cc:207: #if defined (OS_POSIX)
On 2012/06/20 19:24:40, Mark Seaborn wrote:
> There should be no space after 'defined'

Done.

http://codereview.chromium.org/10214007/diff/121014/chrome/renderer/pepper/pp...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

http://codereview.chromium.org/10214007/diff/121014/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:47:
base::LazyInstance<IPC::ChannelHandle> g_ipc_channel_handle =
On 2012/06/20 19:24:40, Mark Seaborn wrote:
> Why is this a global variable?  A global is very likely to go wrong here. 
What
> needs to be changed so that this is not a global?

Changed to a global map that associates ChannelHandles with PP_Instances.

http://codereview.chromium.org/10214007/diff/121014/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:49: base::ProcessId
g_plugin_process_id;
On 2012/06/20 19:24:40, Mark Seaborn wrote:
> This variable is assigned to but never read.

Done.

http://codereview.chromium.org/10214007/diff/121014/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:93: : shutdown_event_(true,
false) {}
On 2012/06/20 22:34:32, dmichael wrote:
> style nit: I'd put } on the next line to be consistent.

Done.

http://codereview.chromium.org/10214007/diff/121014/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:114: // Stubbed out
SyncMessageStatusReceiver, required by HostDispatcher.
On 2012/06/20 22:34:32, dmichael wrote:
> Should we add a TODO?

Done.

http://codereview.chromium.org/10214007/diff/121014/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:208:
plugin_module->InitAsProxiedNaCl(out_of_process_proxy.release(),
On 2012/06/20 22:34:32, dmichael wrote:
> nit: We should probably use Pass() here and take scoped_ptr in
InitAsProxiedNaCl
> to clarify that it's transfer of ownership.

Done.

http://codereview.chromium.org/10214007/diff/121014/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/module_ppapi.cc (right):

http://codereview.chromium.org/10214007/diff/121014/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/module_ppapi.cc:16: StartPpapiProxyFunc
start_ppapi_proxy;
On 2012/06/20 19:24:40, Mark Seaborn wrote:
> Does this have to be a global?  Can't plugin.cc call
> GetBrowserInterface(PPB_NACL_PRIVATE_INTERFACE) itself?

It could but I would have to duplicate the code here to get the interface, check
that it's non-NULL, print an error message if not. I'm just following the
pattern established by 'launch_nacl_process' here.

http://codereview.chromium.org/10214007/diff/121014/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

http://codereview.chromium.org/10214007/diff/121014/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin.cc:615: if
(init_done_cb.pp_completion_callback().func != NULL) {
On 2012/06/20 19:24:40, Mark Seaborn wrote:
> Is there is reason this isn't just
> CHECK(init_done_cb.pp_completion_callback().func != NULL)?  Is it
valid/expected
> for func to be NULL?

Done.

http://codereview.chromium.org/10214007/diff/121014/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin.cc:645: if (started_ipc_proxy_)
On 2012/06/20 22:34:32, dmichael wrote:
> We should probably add a comment here about why we shortcut this method
> (StartSrpcServices would fail, StartJSObjectProxy doesn't make sense in the
IPC
> proxy). Returning true here doesn't really make sense for the plugin's
standard
> usage.

Done.

http://codereview.chromium.org/10214007/diff/121014/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/plugin.h (right):

http://codereview.chromium.org/10214007/diff/121014/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin.h:521: bool started_ipc_proxy_;
On 2012/06/20 19:24:40, Mark Seaborn wrote:
> Would 'using_ipc_proxy_' be more appropriate?  "started" suggests this is used
> for starting something only if it's not already started.

Done.

[bbudge, 2012-06-21 00:03:27]

cc'ing Brad Nelson

[bbudge, 2012-06-21 00:04:09]

really cc'ing Brad this time.

[bbudge, 2012-06-21 10:18:16]

Add an IPC channel between the NaCl loader process and the renderer.
BUG=116317
TEST=manual

[bbudge, 2012-06-21 13:23:02]

Fix issue with PP_Instance - ChannelHandle map, needed to add an #include.

[Mark Seaborn, 2012-06-21 15:30:03]

http://codereview.chromium.org/10214007/diff/121014/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (left):

http://codereview.chromium.org/10214007/diff/121014/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:559: bool
NaClProcessHost::ReplyToRenderer() {
On 2012/06/21 00:00:46, bbudge1 wrote:
> On 2012/06/20 19:24:40, Mark Seaborn wrote:
> > Why do you need to merge ReplyToRenderer() and
> > StartNaClExecution()?  When enable_ipc_proxy is true, you can
> > just defer the call to ReplyToRenderer() until
> > OnPpapiChannelCreated().  That would avoid a handle
> > lifetime/cleanup issue below...
>
> I merged these because we now have to wait for the asynchronous
> reply from sel_ldr before replying to the renderer. This was
> the cleanest way I could see to do this. The non-IPC case looks
> a little strange but I favored the IPC proxy case because we'll
> remove the old path in the near future. See my comment about
> handle cleanup below.

I understand why you've done it, but I don't think you need to move all this
code around.  I think you can do something like this:

bool NaClProcessHost::SendStart() {
  if (!enable_ipc_proxy_) {
    if (!ReplyToRenderer(IPC::ChannelHandle()))
      return false;
    }
  }
  return StartNaClExecution();
}

void NaClProcessHost::OnPpapiChannelCreated(
    const IPC::ChannelHandle& channel_handle) {
  DCHECK(enable_ipc_proxy_);
  ReplyToRenderer(channel_handle);
}

+ add a ChannelHandle arg to ReplyToRenderer()

http://codereview.chromium.org/10214007/diff/121014/chrome/renderer/pepper/pp...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

http://codereview.chromium.org/10214007/diff/121014/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:47:
base::LazyInstance<IPC::ChannelHandle> g_ipc_channel_handle =
On 2012/06/21 00:00:46, bbudge1 wrote:
> On 2012/06/20 19:24:40, Mark Seaborn wrote:
> > Why is this a global variable?  A global is very likely to go wrong
> > here.  What needs to be changed so that this is not a global?
> 
> Changed to a global map that associates ChannelHandles with PP_Instances.

OK, but using a global still isn't very good.  If there's an error between
adding the channel to the global map and using it, you'll leak the channel.  Can
you change LaunchSelLdr() to return the ChannelHandle as well?  You already have
it returning the PP_Instance.

http://codereview.chromium.org/10214007/diff/121014/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/module_ppapi.cc (right):

http://codereview.chromium.org/10214007/diff/121014/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/module_ppapi.cc:16: StartPpapiProxyFunc
start_ppapi_proxy;
On 2012/06/21 00:00:46, bbudge1 wrote:
> On 2012/06/20 19:24:40, Mark Seaborn wrote:
> > Does this have to be a global?  Can't plugin.cc call
> > GetBrowserInterface(PPB_NACL_PRIVATE_INTERFACE) itself?
> 
> It could but I would have to duplicate the code here to get the
> interface, check that it's non-NULL, print an error message if not.
> I'm just following the pattern established by 'launch_nacl_process' here.

Well, launch_nacl_process is a global for legacy reasons: it predates the
existence of the PPB_NaCl_Private interface.  Actually, it predates PPAPI too, I
think.  It's not a good pattern to copy. :-)

Extensive error checking for calling
GetBrowserInterface(PPB_NACL_PRIVATE_INTERFACE) isn't strictly necessary because
you will only get to this code after module_ppapi.cc has already checked this. 
So you could just do CHECK() instead of a non-NULL check + logging.  Given that
it's a Chromium-internal interface, a CHECK() is fine.

[bbudge, 2012-06-21 18:16:53]

More comments addressed. PTAL

http://codereview.chromium.org/10214007/diff/121014/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (left):

http://codereview.chromium.org/10214007/diff/121014/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:559: bool
NaClProcessHost::ReplyToRenderer() {
On 2012/06/21 15:30:03, Mark Seaborn wrote:
> On 2012/06/21 00:00:46, bbudge1 wrote:
> > On 2012/06/20 19:24:40, Mark Seaborn wrote:
> > > Why do you need to merge ReplyToRenderer() and
> > > StartNaClExecution()?  When enable_ipc_proxy is true, you can
> > > just defer the call to ReplyToRenderer() until
> > > OnPpapiChannelCreated().  That would avoid a handle
> > > lifetime/cleanup issue below...
> >
> > I merged these because we now have to wait for the asynchronous
> > reply from sel_ldr before replying to the renderer. This was
> > the cleanest way I could see to do this. The non-IPC case looks
> > a little strange but I favored the IPC proxy case because we'll
> > remove the old path in the near future. See my comment about
> > handle cleanup below.
> 
> I understand why you've done it, but I don't think you need to move all this
> code around.  I think you can do something like this:
> 
> bool NaClProcessHost::SendStart() {
>   if (!enable_ipc_proxy_) {
>     if (!ReplyToRenderer(IPC::ChannelHandle()))
>       return false;
>     }
>   }
>   return StartNaClExecution();
> }
> 
> void NaClProcessHost::OnPpapiChannelCreated(
>     const IPC::ChannelHandle& channel_handle) {
>   DCHECK(enable_ipc_proxy_);
>   ReplyToRenderer(channel_handle);
> }
> 
> + add a ChannelHandle arg to ReplyToRenderer()

Done. Good idea, it's a much smaller change this way.

http://codereview.chromium.org/10214007/diff/121014/chrome/renderer/pepper/pp...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

http://codereview.chromium.org/10214007/diff/121014/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:47:
base::LazyInstance<IPC::ChannelHandle> g_ipc_channel_handle =
On 2012/06/21 15:30:03, Mark Seaborn wrote:
> On 2012/06/21 00:00:46, bbudge1 wrote:
> > On 2012/06/20 19:24:40, Mark Seaborn wrote:
> > > Why is this a global variable?  A global is very likely to go wrong
> > > here.  What needs to be changed so that this is not a global?
> > 
> > Changed to a global map that associates ChannelHandles with PP_Instances.
> 
> OK, but using a global still isn't very good.  If there's an error between
> adding the channel to the global map and using it, you'll leak the channel. 
Can
> you change LaunchSelLdr() to return the ChannelHandle as well?  You already
have
> it returning the PP_Instance.

An IPC::ChannelHandle is a small struct that contains a std::string. The channel
handle has to be used here. The plugin has no knowledge of the IPC channel.

http://codereview.chromium.org/10214007/diff/121014/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/module_ppapi.cc (right):

http://codereview.chromium.org/10214007/diff/121014/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/module_ppapi.cc:16: StartPpapiProxyFunc
start_ppapi_proxy;
On 2012/06/21 15:30:03, Mark Seaborn wrote:
> On 2012/06/21 00:00:46, bbudge1 wrote:
> > On 2012/06/20 19:24:40, Mark Seaborn wrote:
> > > Does this have to be a global?  Can't plugin.cc call
> > > GetBrowserInterface(PPB_NACL_PRIVATE_INTERFACE) itself?
> > 
> > It could but I would have to duplicate the code here to get the
> > interface, check that it's non-NULL, print an error message if not.
> > I'm just following the pattern established by 'launch_nacl_process' here.
> 
> Well, launch_nacl_process is a global for legacy reasons: it predates the
> existence of the PPB_NaCl_Private interface.  Actually, it predates PPAPI too,
I
> think.  It's not a good pattern to copy. :-)
> 
> Extensive error checking for calling
> GetBrowserInterface(PPB_NACL_PRIVATE_INTERFACE) isn't strictly necessary
because
> you will only get to this code after module_ppapi.cc has already checked this.

> So you could just do CHECK() instead of a non-NULL check + logging.  Given
that
> it's a Chromium-internal interface, a CHECK() is fine.

Done.

[dmichael (off chromium), 2012-06-21 18:18:16]

http://codereview.chromium.org/10214007/diff/143003/chrome/renderer/pepper/pp...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

http://codereview.chromium.org/10214007/diff/143003/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:70:
g_channel_handle_map.Get()[instance] = channel_handle;
optional: This will over-write the instance if it's already there (which
shouldn't happen). You could add a CHECK or DCHECK above that
count(instance)==0.

http://codereview.chromium.org/10214007/diff/143003/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

http://codereview.chromium.org/10214007/diff/143003/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin.cc:617:
pp::Module::Get()->core()->CallOnMainThread(0, init_done_cb, PP_OK);
We probably ought to comment to explain this too...  I think normally the
init_done_cb gets invoked as a result of receiving a response from
StartJSObjectProxy (maybe...  I've forgotten since I traced through it). In the
IPC proxy case, we don't invoke that, so we explicitly schedule it here.

http://codereview.chromium.org/10214007/diff/143003/webkit/plugins/ppapi/plug...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/10214007/diff/143003/webkit/plugins/ppapi/plug...
webkit/plugins/ppapi/plugin_module.cc:569: }
As we discussed, this is not going to work for multiple instances...  I'd argue
that it needs a TODO at least.

I'd also prefer if you don't add any more features to this CL, and save that
stuff for a later CL, so we can get it landed. Having to understand a bunch of
new stuff slows down the review cycle.

http://codereview.chromium.org/10214007/diff/143003/webkit/plugins/ppapi/plug...
webkit/plugins/ppapi/plugin_module.cc:595: DCHECK(!reserve_instance_id_) <<
"Only expect one set.";
Why did you move this check? It doesn't make sense here to me.

[bbudge, 2012-06-21 19:28:17]

Eliminated global map in ppb_nacl_private_impl.cc as per Mark's suggestion.
While this affected another handful of files, the changes just plumb through an
extra return value to the plugin. Addressed all comments. PTAL

http://codereview.chromium.org/10214007/diff/143003/chrome/renderer/pepper/pp...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

http://codereview.chromium.org/10214007/diff/143003/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:70:
g_channel_handle_map.Get()[instance] = channel_handle;
On 2012/06/21 18:18:16, dmichael wrote:
> optional: This will over-write the instance if it's already there (which
> shouldn't happen). You could add a CHECK or DCHECK above that
> count(instance)==0.

Next patch removes this global and passes the pointer back to the caller.

http://codereview.chromium.org/10214007/diff/143003/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

http://codereview.chromium.org/10214007/diff/143003/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin.cc:617:
pp::Module::Get()->core()->CallOnMainThread(0, init_done_cb, PP_OK);
On 2012/06/21 18:18:16, dmichael wrote:
> We probably ought to comment to explain this too...  I think normally the
> init_done_cb gets invoked as a result of receiving a response from
> StartJSObjectProxy (maybe...  I've forgotten since I traced through it). In
the
> IPC proxy case, we don't invoke that, so we explicitly schedule it here.

Done.

http://codereview.chromium.org/10214007/diff/143003/webkit/plugins/ppapi/plug...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/10214007/diff/143003/webkit/plugins/ppapi/plug...
webkit/plugins/ppapi/plugin_module.cc:569: }
On 2012/06/21 18:18:16, dmichael wrote:
> As we discussed, this is not going to work for multiple instances...  I'd
argue
> that it needs a TODO at least.
> 
> I'd also prefer if you don't add any more features to this CL, and save that
> stuff for a later CL, so we can get it landed. Having to understand a bunch of
> new stuff slows down the review cycle.

I'll add a TODO for this. PluginModule is already broken for multiple instances
and this just makes things behave better in the single instance case. Sorry the
patch is growing. It's a definite improvement when playing with the new proxy as
you can test multiple nexes now.

http://codereview.chromium.org/10214007/diff/143003/webkit/plugins/ppapi/plug...
webkit/plugins/ppapi/plugin_module.cc:595: DCHECK(!reserve_instance_id_) <<
"Only expect one set.";
On 2012/06/21 18:18:16, dmichael wrote:
> Why did you move this check? It doesn't make sense here to me.

The HostDispatcher sets it and it can be set multiple times now. Discussed this
with Brett.

[dmichael (off chromium), 2012-06-21 19:42:29]

http://codereview.chromium.org/10214007/diff/143003/webkit/plugins/ppapi/plug...
File webkit/plugins/ppapi/plugin_module.cc (right):

http://codereview.chromium.org/10214007/diff/143003/webkit/plugins/ppapi/plug...
webkit/plugins/ppapi/plugin_module.cc:595: DCHECK(!reserve_instance_id_) <<
"Only expect one set.";
On 2012/06/21 19:28:18, bbudge1 wrote:
> On 2012/06/21 18:18:16, dmichael wrote:
> > Why did you move this check? It doesn't make sense here to me.
> 
> The HostDispatcher sets it and it can be set multiple times now. Discussed
this
> with Brett.
So that would argue for simply removing the check. It doesn't make sense here.
You're now DCHECKing that you do not have a reserve_instance_id function, when
in the out-of-process case, you need it. Your OOP tests are all crashing on the
try run.

[Mark Seaborn, 2012-06-21 19:49:49]

LGTM from my point of view

http://codereview.chromium.org/10214007/diff/151008/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/10214007/diff/151008/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:150:
CommandLine::ForCurrentProcess()->HasSwitch(
Nit: this would fit on the previous line

http://codereview.chromium.org/10214007/diff/151008/chrome/nacl/nacl_listener.cc
File chrome/nacl/nacl_listener.cc (right):

http://codereview.chromium.org/10214007/diff/151008/chrome/nacl/nacl_listener...
chrome/nacl/nacl_listener.cc:212: Send(new
NaClProcessHostMsg_PpapiChannelCreated(channel_handle));
Nit: check the return result of Send() and log+return on error

http://codereview.chromium.org/10214007/diff/151008/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

http://codereview.chromium.org/10214007/diff/151008/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin.cc:631:
reinterpret_cast<void*>(ipc_channel_handle_ptr.release()))) {
Nit: indent this by 4 more spaces so it looks like an argument.  Also it would
be better if the args lined up, i.e.

if (ppb_nacl->StartPpapiProxy(
        pp_instance(),
        reinterpret_cast<void*>(ipc_channel_handle_ptr.release()))) {
...

http://codereview.chromium.org/10214007/diff/151008/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/sel_ldr_launcher_chrome.h (right):

http://codereview.chromium.org/10214007/diff/151008/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/sel_ldr_launcher_chrome.h:16: virtual
bool Start(PP_Instance instance,
BTW, this doesn't need to be virtual, does it?

[brettw, 2012-06-21 20:59:43]

lgtm

http://codereview.chromium.org/10214007/diff/151008/chrome/common/render_mess...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/10214007/diff/151008/chrome/common/render_mess...
chrome/common/render_messages.h:489: // the process and return a handle to an
IMC channel.
Can you add a note here about the ipc_channel_handle? I presume it's null when
you're using the old proxy?

http://codereview.chromium.org/10214007/diff/151008/chrome/renderer/pepper/pp...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

http://codereview.chromium.org/10214007/diff/151008/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:107: return &shutdown_event_;
I think this is supposed to be
  return ChildProcess::current()->GetShutDownEvent();
since the caller wants *the* shutdown event rather than some random event (I
actually forget what this is for). Then you can delete your event in the class.

http://codereview.chromium.org/10214007/diff/151008/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:119: // TODO(bbudge) Implement
something like PepperHungPluginFilter.
This should say "hook up to PepperHungPluginFilter" instead (we should be able
to re-use it).

[dmichael (off chromium), 2012-06-21 21:09:35]

lgtm, once you remove the bad DCHECK in plugin_module.cc

[bbudge, 2012-06-21 21:20:01]

OK, addressed all comments.

http://codereview.chromium.org/10214007/diff/151008/chrome/browser/nacl_host/...
File chrome/browser/nacl_host/nacl_process_host.cc (right):

http://codereview.chromium.org/10214007/diff/151008/chrome/browser/nacl_host/...
chrome/browser/nacl_host/nacl_process_host.cc:150:
CommandLine::ForCurrentProcess()->HasSwitch(
On 2012/06/21 19:49:49, Mark Seaborn wrote:
> Nit: this would fit on the previous line

Done.

http://codereview.chromium.org/10214007/diff/151008/chrome/common/render_mess...
File chrome/common/render_messages.h (right):

http://codereview.chromium.org/10214007/diff/151008/chrome/common/render_mess...
chrome/common/render_messages.h:489: // the process and return a handle to an
IMC channel.
On 2012/06/21 20:59:44, brettw wrote:
> Can you add a note here about the ipc_channel_handle? I presume it's null when
> you're using the old proxy?

Done.

http://codereview.chromium.org/10214007/diff/151008/chrome/renderer/pepper/pp...
File chrome/renderer/pepper/ppb_nacl_private_impl.cc (right):

http://codereview.chromium.org/10214007/diff/151008/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:107: return &shutdown_event_;
On 2012/06/21 20:59:44, brettw wrote:
> I think this is supposed to be
>   return ChildProcess::current()->GetShutDownEvent();
> since the caller wants *the* shutdown event rather than some random event (I
> actually forget what this is for). Then you can delete your event in the
class.

Done.

http://codereview.chromium.org/10214007/diff/151008/chrome/renderer/pepper/pp...
chrome/renderer/pepper/ppb_nacl_private_impl.cc:119: // TODO(bbudge) Implement
something like PepperHungPluginFilter.
On 2012/06/21 20:59:44, brettw wrote:
> This should say "hook up to PepperHungPluginFilter" instead (we should be able
> to re-use it).

Done.

http://codereview.chromium.org/10214007/diff/151008/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

http://codereview.chromium.org/10214007/diff/151008/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/plugin.cc:631:
reinterpret_cast<void*>(ipc_channel_handle_ptr.release()))) {
On 2012/06/21 19:49:49, Mark Seaborn wrote:
> Nit: indent this by 4 more spaces so it looks like an argument.  Also it would
> be better if the args lined up, i.e.
> 
> if (ppb_nacl->StartPpapiProxy(
>         pp_instance(),
>         reinterpret_cast<void*>(ipc_channel_handle_ptr.release()))) {
> ...

Done.

http://codereview.chromium.org/10214007/diff/151008/ppapi/native_client/src/t...
File ppapi/native_client/src/trusted/plugin/sel_ldr_launcher_chrome.h (right):

http://codereview.chromium.org/10214007/diff/151008/ppapi/native_client/src/t...
ppapi/native_client/src/trusted/plugin/sel_ldr_launcher_chrome.h:16: virtual
bool Start(PP_Instance instance,
On 2012/06/21 19:49:49, Mark Seaborn wrote:
> BTW, this doesn't need to be virtual, does it?

Done.

[bbudge, 2012-06-21 21:28:30]

On 2012/06/21 21:09:35, dmichael wrote:
> lgtm, once you remove the bad DCHECK in plugin_module.cc

Somehow I added that back but to the wrong method. Fixed.

[bbudge, 2012-06-21 21:56:32]

Backed out change to shutdown event in ppb_nacl_private_impl. Added a TODO to
expose it in content API.

[commit-bot: I haz the power, 2012-06-21 22:47:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/bbudge@chromium.org/10214007/144024

[commit-bot: I haz the power, 2012-06-21 22:47:35]

Presubmit check for 10214007-144024 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Missing PPAPI IDL for private interface, please generate IDL:

***************
  ppapi/c/private/ppb_nacl_private.h
***************

Presubmit checks took 3.1s to calculate.

[Mark Seaborn, 2012-06-22 01:09:17]

Things I spotted while looking for the problem that caused the bot failure...

https://chromiumcodereview.appspot.com/10214007/diff/144024/ppapi/native_clie...
File ppapi/native_client/src/trusted/plugin/plugin.cc (right):

https://chromiumcodereview.appspot.com/10214007/diff/144024/ppapi/native_clie...
ppapi/native_client/src/trusted/plugin/plugin.cc:125: const PPB_NaCl_Private*
GetNaclInterface() {
Nit: Should be "GetNaClInterface"

https://chromiumcodereview.appspot.com/10214007/diff/144024/ppapi/native_clie...
ppapi/native_client/src/trusted/plugin/plugin.cc:126: pp::Module *module =
pp::Module::Get();
Nit: Should be "* " here.

https://chromiumcodereview.appspot.com/10214007/diff/144024/ppapi/native_clie...
ppapi/native_client/src/trusted/plugin/plugin.cc:620: nacl::scoped_ptr<char>
ipc_channel_handle_ptr(
Storing a ChannelHandle as a nacl::scoped_ptr<char> seems like a bad idea. 
You'd still be leaking a handle/FD or other std::string internals on error.  It
would be better not to try to free it here, and if there's a leak, comment it
with a TODO.

https://chromiumcodereview.appspot.com/10214007/diff/144024/ppapi/native_clie...
ppapi/native_client/src/trusted/plugin/plugin.cc:630: if
(ppb_nacl->StartPpapiProxy(
Thinking about this more, if StartPpapiProxy() is called immediate after
Start(), why not fold it into the LaunchSelLdr() function?  You can make
LaunchSelLdr() return a result indicating whether we're using the new proxy.

[bbudge, 2012-06-22 01:32:50]

On 2012/06/22 01:09:17, Mark Seaborn wrote:
> Things I spotted while looking for the problem that caused the bot failure...
> 
>
https://chromiumcodereview.appspot.com/10214007/diff/144024/ppapi/native_clie...
> File ppapi/native_client/src/trusted/plugin/plugin.cc (right):
> 
>
https://chromiumcodereview.appspot.com/10214007/diff/144024/ppapi/native_clie...
> ppapi/native_client/src/trusted/plugin/plugin.cc:125: const PPB_NaCl_Private*
> GetNaclInterface() {
> Nit: Should be "GetNaClInterface"
> 
>
https://chromiumcodereview.appspot.com/10214007/diff/144024/ppapi/native_clie...
> ppapi/native_client/src/trusted/plugin/plugin.cc:126: pp::Module *module =
> pp::Module::Get();
> Nit: Should be "* " here.
> 
>
https://chromiumcodereview.appspot.com/10214007/diff/144024/ppapi/native_clie...
> ppapi/native_client/src/trusted/plugin/plugin.cc:620: nacl::scoped_ptr<char>
> ipc_channel_handle_ptr(
> Storing a ChannelHandle as a nacl::scoped_ptr<char> seems like a bad idea. 
> You'd still be leaking a handle/FD or other std::string internals on error. 
It
> would be better not to try to free it here, and if there's a leak, comment it
> with a TODO.
> 
>
https://chromiumcodereview.appspot.com/10214007/diff/144024/ppapi/native_clie...
> ppapi/native_client/src/trusted/plugin/plugin.cc:630: if
> (ppb_nacl->StartPpapiProxy(
> Thinking about this more, if StartPpapiProxy() is called immediate after
> Start(), why not fold it into the LaunchSelLdr() function?  You can make
> LaunchSelLdr() return a result indicating whether we're using the new proxy.

I like your idea of merging the calls to LaunchSelLdr and StartPpapiProxy. This
will work as things stand now.