Fix NaCl plugin shutdown when running the Chrome PPAPI proxy.

Fix NaCl plugin shutdown when running the Chrome PPAPI proxy.
BUG=116317
TEST=manual

Changes the order that DidDestroy for the untrusted plugin and the trusted
NaCl plugin are called. This allows the plugin to initiate shutdown before
the untrusted plugin, which may hang.
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151217

[bbudge, 2012-08-08 20:31:57]

This fixed the audio hanging issue. I'm not sure about this though. Shut down is
tricky.

http://codereview.chromium.org/10824220/diff/1/webkit/plugins/ppapi/ppapi_plu...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/10824220/diff/1/webkit/plugins/ppapi/ppapi_plu...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:409:
instance_interface_->DidDestroy(pp_instance());
If this hangs, is there an issue about the code that follows not being run? Or
is hanging not possible?

[dmichael (off chromium), 2012-08-08 22:28:52]

http://codereview.chromium.org/10824220/diff/1/webkit/plugins/ppapi/ppapi_plu...
File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):

http://codereview.chromium.org/10824220/diff/1/webkit/plugins/ppapi/ppapi_plu...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:408:
nacl_plugin_instance_interface_->DidDestroy(pp_instance());
This *should* kill the sel_ldr process where the nacl app is running. Not sure
why it isn't working :-/

http://codereview.chromium.org/10824220/diff/1/webkit/plugins/ppapi/ppapi_plu...
webkit/plugins/ppapi/ppapi_plugin_instance.cc:409:
instance_interface_->DidDestroy(pp_instance());
On 2012/08/08 20:31:57, bbudge1 wrote:
> If this hangs, is there an issue about the code that follows not being run? Or
> is hanging not possible?
Hmm, it *is* a synchronous message. But if the process is killed while handling
the message, we should get some kind of error?

I don't really understand why the order of these should matter. It seems like
there's a race condition, and somewhere we're not doing the right thing. Did you
ever find out if the audio was repeating on a short interval, or if the plugin
was clearly still producing audio?

[bbudge, 2012-08-08 23:02:02]

On 2012/08/08 22:28:52, dmichael wrote:
>
http://codereview.chromium.org/10824220/diff/1/webkit/plugins/ppapi/ppapi_plu...
> File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):
> 
>
http://codereview.chromium.org/10824220/diff/1/webkit/plugins/ppapi/ppapi_plu...
> webkit/plugins/ppapi/ppapi_plugin_instance.cc:408:
> nacl_plugin_instance_interface_->DidDestroy(pp_instance());
> This *should* kill the sel_ldr process where the nacl app is running. Not sure
> why it isn't working :-/
> 
>
http://codereview.chromium.org/10824220/diff/1/webkit/plugins/ppapi/ppapi_plu...
> webkit/plugins/ppapi/ppapi_plugin_instance.cc:409:
> instance_interface_->DidDestroy(pp_instance());
> On 2012/08/08 20:31:57, bbudge1 wrote:
> > If this hangs, is there an issue about the code that follows not being run?
Or
> > is hanging not possible?
> Hmm, it *is* a synchronous message. But if the process is killed while
handling
> the message, we should get some kind of error?
> 
> I don't really understand why the order of these should matter. It seems like
> there's a race condition, and somewhere we're not doing the right thing. Did
you
> ever find out if the audio was repeating on a short interval, or if the plugin
> was clearly still producing audio?

I think the order matters because the plugin (untrusted) never returns from its
DidDestroy call. It destructs the Audio resource and gets as far as calling
pthread_join on the audio thread, but the thread never gets the shutdown message
(from the browser), so it keeps running.

With this change, the plugin has a chance to delete itself, which makes the
right things happen in the renderer. It seems weird, but when I call in this
order, everything seems to shut down correctly.

[bbudge, 2012-08-08 23:05:23]

On 2012/08/08 22:28:52, dmichael wrote:
>
http://codereview.chromium.org/10824220/diff/1/webkit/plugins/ppapi/ppapi_plu...
> File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):
> 
>
http://codereview.chromium.org/10824220/diff/1/webkit/plugins/ppapi/ppapi_plu...
> webkit/plugins/ppapi/ppapi_plugin_instance.cc:408:
> nacl_plugin_instance_interface_->DidDestroy(pp_instance());
> This *should* kill the sel_ldr process where the nacl app is running. Not sure
> why it isn't working :-/
> 
>
http://codereview.chromium.org/10824220/diff/1/webkit/plugins/ppapi/ppapi_plu...
> webkit/plugins/ppapi/ppapi_plugin_instance.cc:409:
> instance_interface_->DidDestroy(pp_instance());
> On 2012/08/08 20:31:57, bbudge1 wrote:
> > If this hangs, is there an issue about the code that follows not being run?
Or
> > is hanging not possible?
> Hmm, it *is* a synchronous message. But if the process is killed while
handling
> the message, we should get some kind of error?
> 
> I don't really understand why the order of these should matter. It seems like
> there's a race condition, and somewhere we're not doing the right thing. Did
you
> ever find out if the audio was repeating on a short interval, or if the plugin
> was clearly still producing audio?

To answer your question, I verified that the plugin is still producing audio
(it's call back is running on the audio thread). The audio thread doesn't quit
until it receives a notification from the sync socket.

[dmichael (off chromium), 2012-08-09 17:14:21]

On 2012/08/08 23:02:02, bbudge1 wrote:
> On 2012/08/08 22:28:52, dmichael wrote:
> >
>
http://codereview.chromium.org/10824220/diff/1/webkit/plugins/ppapi/ppapi_plu...
> > File webkit/plugins/ppapi/ppapi_plugin_instance.cc (right):
> > 
> >
>
http://codereview.chromium.org/10824220/diff/1/webkit/plugins/ppapi/ppapi_plu...
> > webkit/plugins/ppapi/ppapi_plugin_instance.cc:408:
> > nacl_plugin_instance_interface_->DidDestroy(pp_instance());
> > This *should* kill the sel_ldr process where the nacl app is running. Not
sure
> > why it isn't working :-/
> > 
> >
>
http://codereview.chromium.org/10824220/diff/1/webkit/plugins/ppapi/ppapi_plu...
> > webkit/plugins/ppapi/ppapi_plugin_instance.cc:409:
> > instance_interface_->DidDestroy(pp_instance());
> > On 2012/08/08 20:31:57, bbudge1 wrote:
> > > If this hangs, is there an issue about the code that follows not being
run?
> Or
> > > is hanging not possible?
> > Hmm, it *is* a synchronous message. But if the process is killed while
> handling
> > the message, we should get some kind of error?
> > 
> > I don't really understand why the order of these should matter. It seems
like
> > there's a race condition, and somewhere we're not doing the right thing. Did
> you
> > ever find out if the audio was repeating on a short interval, or if the
plugin
> > was clearly still producing audio?
> 
> I think the order matters because the plugin (untrusted) never returns from
its
> DidDestroy call. It destructs the Audio resource and gets as far as calling
> pthread_join on the audio thread, but the thread never gets the shutdown
message
> (from the browser), so it keeps running.
Okay, I think I see what's going on...  it looks to me like the stuff in
PPB_AudioShared _should_ work with either order. StopThread closes the socket,
which should make the thread exit (its "Run()" while loop should fail when
reading the socket). The problem is that sync_socket_nacl.cc doesn't implement
the "Cancellable" stuff at all.

> 
> With this change, the plugin has a chance to delete itself, which makes the
> right things happen in the renderer. It seems weird, but when I call in this
> order, everything seems to shut down correctly.
That makes sense...  it's because the plugin kills the process, so the later
call to the plugin's DidDestroy probably just fails (I think).

That said, I think this new order is good...  in fact, we might not even want to
call DidDestroy on the proxy I/F if this is a NaCl plugin, since it shouldn't do
anything anyway. This matches what the current SRPC proxy does...  it kills the
app without actually calling DidDestroy.

[brettw, 2012-08-10 20:22:49]

lgtm

[commit-bot: I haz the power, 2012-08-12 23:38:34]

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