Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2517703003:
Terminate child process if sandbox::TargetProcess is destroyed. (Closed)
|
Created:
4 years, 1 month ago by jbauman Modified:
4 years ago Reviewers:
Will Harris CC:
chromium-reviews, wfh+watch_chromium.org, pennymac+watch_chromium.org, rickyz+watch_chromium.org, forshaw Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTerminate child process if sandbox::TargetProcess is destroyed.
Leaking the ipc server doesn't work as it has raw pointers to other
classes (like the Dispatcher) that will be destroyed.
BUG=663019
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Committed: https://crrev.com/14944d76e12f71f7d1a45832b303eafff0a708c7
Cr-Commit-Position: refs/heads/master@{#437093}
Patch Set 1 #
Total comments: 1
Patch Set 2 : change result code #Messages
Total messages: 20 (13 generated)
Description was changed from ========== Terminate child process if sandbox::TargetProcess is destroyed. Leaking the ipc server doesn't work as it has raw pointers to other classes (like the Dispatcher) that will be destroyed. BUG=663019 ========== to ========== Terminate child process if sandbox::TargetProcess is destroyed. Leaking the ipc server doesn't work as it has raw pointers to other classes (like the Dispatcher) that will be destroyed. BUG=663019 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
The CQ bit was checked by jbauman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
jbauman@chromium.org changed reviewers: + wfh@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2517703003/diff/1/sandbox/win/src/target_proc... File sandbox/win/src/target_process.cc (right): https://codereview.chromium.org/2517703003/diff/1/sandbox/win/src/target_proc... sandbox/win/src/target_process.cc:76: ::TerminateProcess(sandbox_process_info_.process_handle(), 2); why 2? I think that's RESULT_CODE_HUNG. If it's meant to be RESULT_CODE_HUNG then please use that code - do you mean to use RESULT_CODE_KILLED?
The CQ bit was checked by jbauman@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_x86-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
On 2016/11/23 16:44:23, Will Harris wrote: > https://codereview.chromium.org/2517703003/diff/1/sandbox/win/src/target_proc... > File sandbox/win/src/target_process.cc (right): > > https://codereview.chromium.org/2517703003/diff/1/sandbox/win/src/target_proc... > sandbox/win/src/target_process.cc:76: > ::TerminateProcess(sandbox_process_info_.process_handle(), 2); > why 2? > > I think that's RESULT_CODE_HUNG. If it's meant to be RESULT_CODE_HUNG then > please use that code - do you mean to use RESULT_CODE_KILLED? Ok, yeah. Switched it to RESULT_CODE_KILLED (though I couldn't use the actual enum value due to DEPS issues).
ya DEPS with sandbox is a pain, normally I end up adding a static_assert somewhere to make sure the value doesn't get changed. otherwise, regardless. lgtm
The CQ bit was checked by jbauman@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 20001, "attempt_start_ts": 1481146128056090,
"parent_rev": "dc19ad60b82078bb19d4aa9873d9c25d0792d46b", "commit_rev":
"6117fe9ed6e4caeecfcb2ea34c0ad7eacdd2a205"}
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Terminate child process if sandbox::TargetProcess is destroyed. Leaking the ipc server doesn't work as it has raw pointers to other classes (like the Dispatcher) that will be destroyed. BUG=663019 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Terminate child process if sandbox::TargetProcess is destroyed. Leaking the ipc server doesn't work as it has raw pointers to other classes (like the Dispatcher) that will be destroyed. BUG=663019 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Committed: https://crrev.com/14944d76e12f71f7d1a45832b303eafff0a708c7 Cr-Commit-Position: refs/heads/master@{#437093} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/14944d76e12f71f7d1a45832b303eafff0a708c7 Cr-Commit-Position: refs/heads/master@{#437093} |
