Kill pnacl translation processes immediately on coordinator error and destruction

Kill pnacl translation processes immediately on coordinator error and destruction

On errors and destruction, use the service_runtime object in the subprocess
to kill the translator processes immediately rather than just signaling the
translation thread, which may be blocked on an RPC.
Any pending or new RPC calls will fail immediately, and the translation thread
can simply bail.
This makes destruction/surfaway much faster and more responsive and simplifies
error handling and object cleanup. The only caveat is that now the translation
thread and NaClSubprocess must be careful not to race service runtime operations
(which are protected by subprocess_mu_) with RPCs (which are called
without a mutex because they block). This is currently already ensured because
srpc_client is a separate object from service_runtime in NaClSubprocess.

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


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

[jvoung (off chromium), 2012-08-02 23:24:10]

one question but otherwise looks pretty good

https://chromiumcodereview.appspot.com/10830149/diff/2001/ppapi/native_client...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc (right):

https://chromiumcodereview.appspot.com/10830149/diff/2001/ppapi/native_client...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:288: if
(llc_subprocess_ != NULL) llc_subprocess_->service_runtime()->Shutdown();
Since set llc_subprocess is not set to null after, and AbortSubprocess() is
called from more than one destructor, is it okay to
service_runtime()->Shutdown() more than once?

https://chromiumcodereview.appspot.com/10830149/diff/2001/ppapi/native_client...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:300:
AbortSubprocesses();
AbortSubprocesses() is called from multiple destructors -- just wanted to check
if that was idempotent.  If it is idempotent, I guess it might be better to nuke
the subprocesses from orbit with all nukes available.  Otherwise,  maybe just
nuke it from this destructor and not the PnaclCoordinator one?

[Derek Schuff, 2012-08-02 23:46:45]

https://chromiumcodereview.appspot.com/10830149/diff/2001/ppapi/native_client...
File ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc (right):

https://chromiumcodereview.appspot.com/10830149/diff/2001/ppapi/native_client...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:288: if
(llc_subprocess_ != NULL) llc_subprocess_->service_runtime()->Shutdown();
I *think* so, but it actually goes pretty deep into SRPC destructors and such,
so just to be safe I went ahead and added a guard to avoid calling Shutdown()
more than once.
We do have to wait to destroy the object until after the RPCs are done, however.
On 2012/08/02 23:24:10, jvoung (chromium) wrote:
> Since set llc_subprocess is not set to null after, and AbortSubprocess() is
> called from more than one destructor, is it okay to
> service_runtime()->Shutdown() more than once?

https://chromiumcodereview.appspot.com/10830149/diff/2001/ppapi/native_client...
ppapi/native_client/src/trusted/plugin/pnacl_translate_thread.cc:300:
AbortSubprocesses();
Other than potentially service_runtime Shutdown (addressed above), it's fine.
And I think it's better to keep it idempotent and just use it wherever
necessary.
On 2012/08/02 23:24:10, jvoung (chromium) wrote:
> AbortSubprocesses() is called from multiple destructors -- just wanted to
check
> if that was idempotent.  If it is idempotent, I guess it might be better to
nuke
> the subprocesses from orbit with all nukes available.  Otherwise,  maybe just
> nuke it from this destructor and not the PnaclCoordinator one?

[jvoung (off chromium), 2012-08-02 23:54:42]

lgtm

[Derek Schuff, 2012-08-03 22:35:59]

On 2012/08/02 23:54:42, jvoung (chromium) wrote:
> lgtm

OK, this should be the same as PS3, can you take a quick look at the wrappers?

[jvoung (off chromium), 2012-08-03 22:42:15]

On 2012/08/03 22:35:59, dschuff wrote:
> On 2012/08/02 23:54:42, jvoung (chromium) wrote:
> > lgtm
> 
> OK, this should be the same as PS3, can you take a quick look at the wrappers?

yes, re-re-rebased PS7 LGTM

[commit-bot: I haz the power, 2012-08-03 22:45:10]

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

[commit-bot: I haz the power, 2012-08-04 00:17:28]

Change committed as 149982