Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(667)

Issue 1128373005: PNaCl: Hold subprocess_mu while doing StartSrpcServices. (Closed)

Created:
5 years, 7 months ago by jvoung (off chromium)
Modified:
5 years, 7 months ago
Reviewers:
Derek Schuff
CC:
chromium-reviews
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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 Committed: https://crrev.com/7862c2537ff491d5865a451b3f9f96bb7c2f3483 Cr-Commit-Position: refs/heads/master@{#329718}

Patch Set 1 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+22 lines, -24 lines) Patch
M components/nacl/renderer/plugin/pnacl_translate_thread.h View 1 chunk +2 lines, -2 lines 0 comments Download
M components/nacl/renderer/plugin/pnacl_translate_thread.cc View 2 chunks +20 lines, -22 lines 0 comments Download

Messages

Total messages: 7 (2 generated)
jvoung (off chromium)
5 years, 7 months ago (2015-05-13 17:26:39 UTC) #2
Derek Schuff
lgtm
5 years, 7 months ago (2015-05-13 20:56:31 UTC) #3
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1128373005/1
5 years, 7 months ago (2015-05-13 21:08:48 UTC) #5
commit-bot: I haz the power
Committed patchset #1 (id:1)
5 years, 7 months ago (2015-05-13 21:16:16 UTC) #6
commit-bot: I haz the power
5 years, 7 months ago (2015-05-13 21:17:05 UTC) #7
Message was sent while issue was closed.
Patchset 1 (id:??) landed as
https://crrev.com/7862c2537ff491d5865a451b3f9f96bb7c2f3483
Cr-Commit-Position: refs/heads/master@{#329718}

Powered by Google App Engine
This is Rietveld 408576698