Commit 7862c253 authored by jvoung's avatar jvoung Committed by Commit bot

PNaCl: Hold subprocess_mu while doing StartSrpcServices.

StartSrpcServices will require using the service_runtime pointer,
to set up the srpc_client pointer. So we cannot destroy the
service_runtime at the same time.

E.g., if I add a sleep() to induce the race, I get:

../../native_client/src/include/nacl_scoped_ptr.h:96: C *nacl::scoped_ptr<plugin::SelLdrLauncherChrome>::operator->() const [C = plugin::SelLdrLauncherChrome]: Assertion `ptr_ != __null' failed.
Received signal 6
#0 0x7f687f4aef7e base::debug::StackTrace::StackTrace()
#1 0x7f687f4aeabf base::debug::(anonymous namespace)::StackDumpSignalHandler()
#2 0x7f6879dee340 <unknown>
#3 0x7f6876504cc9 gsignal
#4 0x7f68765080d8 abort
#5 0x7f68764fdb86 <unknown>
#6 0x7f68764fdc32 __assert_fail
#7 0x7f688e2887f2 nacl::scoped_ptr<>::operator->()
#8 0x7f688e288487 plugin::ServiceRuntime::SetupAppChannel()
#9 0x7f688e28cb76 plugin::NaClSubprocess::StartSrpcServices()
#10 0x7f688e281810 plugin::PnaclTranslateThread::DoLink()
#11 0x7f688e27fe7d plugin::PnaclTranslateThread::DoLinkThread()
#12 0x7f6879de6182 start_thread
#13 0x7f68765c847d clone

Should have checked this in previous refactoring:
https://codereview.chromium.org/1128943003

BUG=473474

Review URL: https://codereview.chromium.org/1128373005

Cr-Commit-Position: refs/heads/master@{#329718}
parent 9d137bfc
...@@ -190,21 +190,20 @@ void WINAPI PnaclTranslateThread::DoCompileThread(void* arg) { ...@@ -190,21 +190,20 @@ void WINAPI PnaclTranslateThread::DoCompileThread(void* arg) {
} }
void PnaclTranslateThread::DoCompile() { void PnaclTranslateThread::DoCompile() {
// If the main thread asked us to exit in between starting the thread
// and now, just leave now.
{ {
nacl::MutexLocker ml(&subprocess_mu_); nacl::MutexLocker ml(&subprocess_mu_);
// If the main thread asked us to exit in between starting the thread
// and now, just leave now.
if (!compiler_subprocess_active_) if (!compiler_subprocess_active_)
return; return;
} // Now that we are in helper thread, we can do the the blocking
// StartSrpcServices operation.
// Now that we are in helper thread, we can do the the blocking if (!compiler_subprocess_->StartSrpcServices()) {
// StartSrpcServices operation. TranslateFailed(
if (!compiler_subprocess_->StartSrpcServices()) { PP_NACL_ERROR_SRPC_CONNECTION_FAIL,
TranslateFailed( "SRPC connection failure for " + compiler_subprocess_->description());
PP_NACL_ERROR_SRPC_CONNECTION_FAIL, return;
"SRPC connection failure for " + compiler_subprocess_->description()); }
return;
} }
SrpcParams params; SrpcParams params;
...@@ -338,21 +337,20 @@ void WINAPI PnaclTranslateThread::DoLinkThread(void* arg) { ...@@ -338,21 +337,20 @@ void WINAPI PnaclTranslateThread::DoLinkThread(void* arg) {
} }
void PnaclTranslateThread::DoLink() { void PnaclTranslateThread::DoLink() {
// If the main thread asked us to exit in between starting the thread
// and now, just leave now.
{ {
nacl::MutexLocker ml(&subprocess_mu_); nacl::MutexLocker ml(&subprocess_mu_);
// If the main thread asked us to exit in between starting the thread
// and now, just leave now.
if (!ld_subprocess_active_) if (!ld_subprocess_active_)
return; return;
} // Now that we are in helper thread, we can do the the blocking
// StartSrpcServices operation.
// Now that we are in helper thread, we can do the the blocking if (!ld_subprocess_->StartSrpcServices()) {
// StartSrpcServices operation. TranslateFailed(
if (!ld_subprocess_->StartSrpcServices()) { PP_NACL_ERROR_SRPC_CONNECTION_FAIL,
TranslateFailed( "SRPC connection failure for " + ld_subprocess_->description());
PP_NACL_ERROR_SRPC_CONNECTION_FAIL, return;
"SRPC connection failure for " + ld_subprocess_->description()); }
return;
} }
SrpcParams params; SrpcParams params;
......
...@@ -113,9 +113,9 @@ class PnaclTranslateThread { ...@@ -113,9 +113,9 @@ class PnaclTranslateThread {
// subprocesses. The *_subprocess_active flags indicate which subprocesses // subprocesses. The *_subprocess_active flags indicate which subprocesses
// are active to ensure the subprocesses don't get shutdown more than once. // are active to ensure the subprocesses don't get shutdown more than once.
// The subprocess_mu_ must be held when shutting down the subprocesses // The subprocess_mu_ must be held when shutting down the subprocesses
// or checking if it's already shut down (via the active flags). // or otherwise accessing the service_runtime component of the subprocess.
// There are some accesses to the subprocesses without locks held // There are some accesses to the subprocesses without locks held
// (invoking SRPC methods client). // (invoking srpc_client methods -- in contrast to using the service_runtime).
NaClSubprocess* compiler_subprocess_; NaClSubprocess* compiler_subprocess_;
NaClSubprocess* ld_subprocess_; NaClSubprocess* ld_subprocess_;
bool compiler_subprocess_active_; bool compiler_subprocess_active_;
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment