NaCl plugin: Make ServiceRuntime::Shutdown() kill the NaCl loader

NaCl plugin: Make ServiceRuntime::Shutdown() kill the NaCl loader

Previously, Shutdown would kill the loader by closing the SRPC command
channel. After SRPC removal, it became a no-op. But this caused a
deadlock in some cases where surfaway or embed element removal would
cause the PNaCl translator to block waiting to join the translation
thread while the translator nexe was waiting for more data. This change
makes the original shutdown sequence work as intended by making
ServiceRuntime::Shutdown() close the trusted plugin IPC channel, which
causes the service runtime to immediately exit.

1. This pokes a sort of arbitrary hole in the PPBNaClPrivate interface
but that interface is vestigial anyway.
2. Even aside from PPBNaClPrvate, there is a lot that could be simplified
or refactored about shutdown post-SRPC, but this is a minimal change
to fix the deadlock that users are experiencing, so we can get it
on a branch.

R=bbudge
BUG=645688
Committed: https://crrev.com/61ef841a46679445e532906eba407e4ba7ecda8e
Cr-Commit-Position: refs/heads/master@{#437418}

[Derek Schuff, 2016-12-07 22:19:49]

The CQ bit was checked by dschuff@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 22:20:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-07 22:26:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-07 22:26:32]

Dry run: Try jobs failed on following builders:
  win_clang on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)

[Derek Schuff, 2016-12-07 23:28:31]

The CQ bit was checked by dschuff@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 23:29:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Derek Schuff, 2016-12-07 23:38:40]

The CQ bit was checked by dschuff@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 23:39:16]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Derek Schuff, 2016-12-07 23:45:21]

The CQ bit was checked by dschuff@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-07 23:45:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-08 00:25:20]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-08 00:25:21]

Dry run: This issue passed the CQ dry run.

[bbudge, 2016-12-08 23:20:00]

Looking good, just a few comments.

https://codereview.chromium.org/2556983003/diff/80001/components/nacl/rendere...
File components/nacl/renderer/nexe_load_manager.h (right):

https://codereview.chromium.org/2556983003/diff/80001/components/nacl/rendere...
components/nacl/renderer/nexe_load_manager.h:71: void
close_trusted_plugin_channel();
nit: not a getter or setter, so use camel case and move decl with other regular
methods.

https://codereview.chromium.org/2556983003/diff/80001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private.h (right):

https://codereview.chromium.org/2556983003/diff/80001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private.h:208: 
remove stray line

https://codereview.chromium.org/2556983003/diff/80001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private.h:285: /* Terminate the NaCl loader
process associated with the instance */
nit: Terminates

https://codereview.chromium.org/2556983003/diff/80001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/2556983003/diff/80001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:889: auto* load_mgr =
GetNexeLoadManager(instance);
nit: is * necessary?

[Derek Schuff, 2016-12-09 00:11:15]

The CQ bit was checked by dschuff@chromium.org to run a CQ dry run

[Derek Schuff, 2016-12-09 00:11:34]

https://codereview.chromium.org/2556983003/diff/80001/components/nacl/rendere...
File components/nacl/renderer/nexe_load_manager.h (right):

https://codereview.chromium.org/2556983003/diff/80001/components/nacl/rendere...
components/nacl/renderer/nexe_load_manager.h:71: void
close_trusted_plugin_channel();
On 2016/12/08 23:20:00, bbudge wrote:
> nit: not a getter or setter, so use camel case and move decl with other
regular
> methods.

Done.

https://codereview.chromium.org/2556983003/diff/80001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private.h (right):

https://codereview.chromium.org/2556983003/diff/80001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private.h:208: 
On 2016/12/08 23:20:00, bbudge wrote:
> remove stray line

Done.

https://codereview.chromium.org/2556983003/diff/80001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private.h:285: /* Terminate the NaCl loader
process associated with the instance */
On 2016/12/08 23:20:00, bbudge wrote:
> nit: Terminates

Done.

https://codereview.chromium.org/2556983003/diff/80001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/2556983003/diff/80001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:889: auto* load_mgr =
GetNexeLoadManager(instance);
On 2016/12/08 23:20:00, bbudge wrote:
> nit: is * necessary?

It's not necessary but it's good practice to use auto* or auto& when applicable
to avoid accidentally copying objects (especially after a refactor, etc).

[commit-bot: I haz the power, 2016-12-09 00:11:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[bbudge, 2016-12-09 00:19:44]

lgtm Thanks for fixing this!

https://codereview.chromium.org/2556983003/diff/80001/components/nacl/rendere...
File components/nacl/renderer/ppb_nacl_private_impl.cc (right):

https://codereview.chromium.org/2556983003/diff/80001/components/nacl/rendere...
components/nacl/renderer/ppb_nacl_private_impl.cc:889: auto* load_mgr =
GetNexeLoadManager(instance);
On 2016/12/09 00:11:34, Derek Schuff wrote:
> On 2016/12/08 23:20:00, bbudge wrote:
> > nit: is * necessary?
> 
> It's not necessary but it's good practice to use auto* or auto& when
applicable
> to avoid accidentally copying objects (especially after a refactor, etc).

OK

[commit-bot: I haz the power, 2016-12-09 00:22:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-09 00:22:03]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Derek Schuff, 2016-12-09 00:24:00]

The CQ bit was checked by dschuff@chromium.org

[commit-bot: I haz the power, 2016-12-09 00:24:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-09 01:41:29]

CQ is committing da patch.

Bot data: {"patchset_id": 100001, "attempt_start_ts": 1481243040131600,
"parent_rev": "02770645232c596921f3e5756609ce3d97efdbd0", "commit_rev":
"9b5697018f5308d7661710b163c0ae4fc875f27a"}

[commit-bot: I haz the power, 2016-12-09 01:42:10]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2016-12-09 01:45:43]

Description was changed from

==========
NaCl plugin: Make ServiceRuntime::Shutdown() kill the NaCl loader

Previously, Shutdown would kill the loader by closing the SRPC command
channel. After SRPC removal, it became a no-op. But this caused a
deadlock in some cases where surfaway or embed element removal would
cause the PNaCl translator to block waiting to join the translation
thread while the translator nexe was waiting for more data. This change
makes the original shutdown sequence work as intended by making
ServiceRuntime::Shutdown() close the trusted plugin IPC channel, which
causes the service runtime to immediately exit.

1. This pokes a sort of arbitrary hole in the PPBNaClPrivate interface
but that interface is vestigial anyway.
2. Even aside from PPBNaClPrvate, there is a lot that could be simplified
or refactored about shutdown post-SRPC, but this is a minimal change
to fix the deadlock that users are experiencing, so we can get it
on a branch.

R=bbudge
BUG=645688
==========

to

==========
NaCl plugin: Make ServiceRuntime::Shutdown() kill the NaCl loader

Previously, Shutdown would kill the loader by closing the SRPC command
channel. After SRPC removal, it became a no-op. But this caused a
deadlock in some cases where surfaway or embed element removal would
cause the PNaCl translator to block waiting to join the translation
thread while the translator nexe was waiting for more data. This change
makes the original shutdown sequence work as intended by making
ServiceRuntime::Shutdown() close the trusted plugin IPC channel, which
causes the service runtime to immediately exit.

1. This pokes a sort of arbitrary hole in the PPBNaClPrivate interface
but that interface is vestigial anyway.
2. Even aside from PPBNaClPrvate, there is a lot that could be simplified
or refactored about shutdown post-SRPC, but this is a minimal change
to fix the deadlock that users are experiencing, so we can get it
on a branch.

R=bbudge
BUG=645688

Committed: https://crrev.com/61ef841a46679445e532906eba407e4ba7ecda8e
Cr-Commit-Position: refs/heads/master@{#437418}
==========

[commit-bot: I haz the power, 2016-12-09 01:45:44]

Patchset 6 (id:??) landed as
https://crrev.com/61ef841a46679445e532906eba407e4ba7ecda8e
Cr-Commit-Position: refs/heads/master@{#437418}