Better handling of surfaway/reload in PNaCl translation

Better handling of surfaway/reload in PNaCl translation
On surfaway when the Plugin object gets destroyed, the coordinator joins the
translation thread, which could be waiting for an SRPC to return.
But the compiler or linker could be blocked (or about to block) on a
reverse service call, which could cause a deadlock. So, the reverse service
interface for the translator or linker needs to be shut down as well which
will cause the nexe's RPC to fail and break the deadlock.
Also, join the translation thread in the streaming translation object where
it was created to make sure the objects it needs are live.

R=jvoung@chromium.org,sehr@chromium.org,robertm@chromium.org
BUG=http://code.google.com/p/nativeclient/issues/detail?id=2195
TEST=nacl_integration


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=149300

[Derek Schuff, 2012-07-31 16:54:33]

PTAL.
These 2 classes can probably be integrated, or at least better organized now
that the non-streaming code is gone, but that's for a future CL.

[robertm, 2012-07-31 17:22:18]

As you said the code might benefit from some more refactoring.
The code is far from being "obviously" correct, though job control is always a
bit messy/

https://chromiumcodereview.appspot.com/10843009/diff/1/ppapi/native_client/sr...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc (right):

https://chromiumcodereview.appspot.com/10843009/diff/1/ppapi/native_client/sr...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:133: void
PnaclTranslateThread::SetSubprocessesShouldDie() {
The name of this function does not reflect the side effect
of shutting down current_rev_interface_

https://chromiumcodereview.appspot.com/10843009/diff/1/ppapi/native_client/sr...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:137: if
(current_rev_interface_) current_rev_interface_->ShutDown();
maybe also set current_rev_interface_ to zero,
assuming it is not legit to use it after shutdown.

Maybe cross reference where else the interface is shutdown
(probably as part of the destruction of the service runtime .

https://chromiumcodereview.appspot.com/10843009/diff/1/ppapi/native_client/sr...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h (right):

https://chromiumcodereview.appspot.com/10843009/diff/1/ppapi/native_client/sr...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h:85:
PluginReverseInterface *current_rev_interface_;
"*" to the left

[Derek Schuff, 2012-07-31 17:41:27]

https://chromiumcodereview.appspot.com/10843009/diff/1/ppapi/native_client/sr...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc (right):

https://chromiumcodereview.appspot.com/10843009/diff/1/ppapi/native_client/sr...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:133: void
PnaclTranslateThread::SetSubprocessesShouldDie() {
The desired effect is to cause the subprocesses to die, and shutting down
current_rev_interface_ is part of that; I'm not sure I would call it a side
effect. (it could be called KillSubprocesses or ShutdownSubprocesses but it does
not do so immediately, presumably that's why it is so named).
On 2012/07/31 17:22:18, robertm wrote:
> The name of this function does not reflect the side effect
> of shutting down current_rev_interface_

https://chromiumcodereview.appspot.com/10843009/diff/1/ppapi/native_client/sr...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:137: if
(current_rev_interface_) current_rev_interface_->ShutDown();
On 2012/07/31 17:22:18, robertm wrote:
> maybe also set current_rev_interface_ to zero,
> assuming it is not legit to use it after shutdown.
It actually does seem ok to use it, since ShutDown just sets a flag variable
internally, but still seems like a good idea to just do it once.
Also added a cross ref in the comment on RegisterReverseInterface in the header
file.

> 
> Maybe cross reference where else the interface is shutdown
> (probably as part of the destruction of the service runtime .

https://chromiumcodereview.appspot.com/10843009/diff/1/ppapi/native_client/sr...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h (right):

https://chromiumcodereview.appspot.com/10843009/diff/1/ppapi/native_client/sr...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h:85:
PluginReverseInterface *current_rev_interface_;
On 2012/07/31 17:22:18, robertm wrote:
> "*" to the left

Done.

[jvoung (off chromium), 2012-07-31 18:51:45]

http://codereview.chromium.org/10843009/diff/5/ppapi/native_client/src/truste...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc (right):

http://codereview.chromium.org/10843009/diff/5/ppapi/native_client/src/truste...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:138:
current_rev_interface_->ShutDown();
I assume that this works better than shutting down the llc_subprocess or
ld_subprocess objects.

http://codereview.chromium.org/10843009/diff/5/ppapi/native_client/src/truste...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h (right):

http://codereview.chromium.org/10843009/diff/5/ppapi/native_client/src/truste...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h:36: //
TODO(jvoung/dschuff): handle surfaway issues when coordinator/plugin
is this TODO() obsolete, w/ the joining?

http://codereview.chromium.org/10843009/diff/5/ppapi/native_client/src/truste...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h:69: // plugin is
shutdown. The reverse service pointer  must be available on the
extra spaces: "pointer  must be"

http://codereview.chromium.org/10843009/diff/5/ppapi/native_client/src/truste...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h:70: // main
thread because the translation thread could be blocked on SRPC
for a future CL: would it be possible to make it more clear which code in this
class runs on the main thread and which runs on the separate thread?

[Derek Schuff, 2012-07-31 18:57:50]

http://codereview.chromium.org/10843009/diff/5/ppapi/native_client/src/truste...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc (right):

http://codereview.chromium.org/10843009/diff/5/ppapi/native_client/src/truste...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:138:
current_rev_interface_->ShutDown();
Well, this was simpler than trying to own those objects outside their current
local scopes (in this case the only thing we expose externally is the bit we
need). 
On 2012/07/31 18:51:45, jvoung (chromium) wrote:
> I assume that this works better than shutting down the llc_subprocess or
> ld_subprocess objects.

http://codereview.chromium.org/10843009/diff/5/ppapi/native_client/src/truste...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h (right):

http://codereview.chromium.org/10843009/diff/5/ppapi/native_client/src/truste...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h:36: //
TODO(jvoung/dschuff): handle surfaway issues when coordinator/plugin
On 2012/07/31 18:51:45, jvoung (chromium) wrote:
> is this TODO() obsolete, w/ the joining?

Done.

http://codereview.chromium.org/10843009/diff/5/ppapi/native_client/src/truste...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h:69: // plugin is
shutdown. The reverse service pointer  must be available on the
On 2012/07/31 18:51:45, jvoung (chromium) wrote:
> extra spaces: "pointer  must be"

Done.

http://codereview.chromium.org/10843009/diff/5/ppapi/native_client/src/truste...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.h:70: // main
thread because the translation thread could be blocked on SRPC
Yeah, actually i think all the code is main thread and the translation thread
code is in pnacl_streaming_translate_thread which is decently labelled in that
regard. But a clean up CL is in the works.
On 2012/07/31 18:51:45, jvoung (chromium) wrote:
> for a future CL: would it be possible to make it more clear which code in this
> class runs on the main thread and which runs on the separate thread?

[jvoung (off chromium), 2012-07-31 20:42:19]

lgtm

[commit-bot: I haz the power, 2012-07-31 20:48:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dschuff@chromium.org/10843009/7002

[commit-bot: I haz the power, 2012-07-31 22:39:19]

Change committed as 149300