Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2851473005:
zygote: Don't CHECK if browser gone during zygote startup (Closed)
|
Created:
3 years, 7 months ago by James Cook Modified:
3 years, 7 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, jln+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
Descriptionzygote: Don't CHECK if browser gone during zygote startup
When the zygote starts it sends a "boot message" to the browser process
across a socket. If the browser has already exited or crashed the
sendmsg() will fail. We've seen this happen in the Chrome OS autotest
lab.
Instead of crashing the zygote, just log an error and cleanly exit.
BUG=692227
TEST=add sleep to zygote startup, kill browser before zygote boot
message is sent, verify zygote shuts down cleanly
Review-Url: https://codereview.chromium.org/2851473005
Cr-Commit-Position: refs/heads/master@{#468027}
Committed: https://chromium.googlesource.com/chromium/src/+/facab14158a0d26aa4ad59b5b547f5da90fb7c2a
Patch Set 1 #
Total comments: 3
Patch Set 2 : review comments #Messages
Total messages: 22 (14 generated)
jamescook@chromium.org changed reviewers: + rockot@chromium.org
rockot, please take a look. https://codereview.chromium.org/2851473005/diff/1/content/zygote/zygote_main_... File content/zygote/zygote_main_linux.cc (right): https://codereview.chromium.org/2851473005/diff/1/content/zygote/zygote_main_... content/zygote/zygote_main_linux.cc:627: linux_sandbox->set_exiting_before_initialize_sandbox(true); If I don't do something like this I hit the CHECK(initialize_sandbox_ran_) in ~SandboxLinux(). SandboxLinux is a singleton cleaned up by the AtExitManager. Alternately I could _exit(1), but this seemed a little better.
lgtm
jamescook@chromium.org changed reviewers: + mdempsky@chromium.org
mdempsky, please take a look.
The CQ bit was checked by jamescook@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...
https://codereview.chromium.org/2851473005/diff/1/content/zygote/zygote_main_... File content/zygote/zygote_main_linux.cc (right): https://codereview.chromium.org/2851473005/diff/1/content/zygote/zygote_main_... content/zygote/zygote_main_linux.cc:627: linux_sandbox->set_exiting_before_initialize_sandbox(true); Since this is security sensitive code, I'd prefer we err on the side of simplicity and just _exit(1).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by jamescook@chromium.org to run a CQ dry run
mdempsky, please take another look. https://codereview.chromium.org/2851473005/diff/1/content/zygote/zygote_main_... File content/zygote/zygote_main_linux.cc (right): https://codereview.chromium.org/2851473005/diff/1/content/zygote/zygote_main_... content/zygote/zygote_main_linux.cc:627: linux_sandbox->set_exiting_before_initialize_sandbox(true); On 2017/04/27 22:50:45, mdempsky wrote: > Since this is security sensitive code, I'd prefer we err on the side of > simplicity and just _exit(1). Sounds good. Done.
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: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by jamescook@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org Link to the patchset: https://codereview.chromium.org/2851473005/#ps20001 (title: "review comments")
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": 1493398018869070,
"parent_rev": "91d96fbdff12d8751743e7b51df93afeeb33c823", "commit_rev":
"facab14158a0d26aa4ad59b5b547f5da90fb7c2a"}
Message was sent while issue was closed.
Description was changed from ========== zygote: Don't CHECK if browser gone during zygote startup When the zygote starts it sends a "boot message" to the browser process across a socket. If the browser has already exited or crashed the sendmsg() will fail. We've seen this happen in the Chrome OS autotest lab. Instead of crashing the zygote, just log an error and cleanly exit. BUG=692227 TEST=add sleep to zygote startup, kill browser before zygote boot message is sent, verify zygote shuts down cleanly ========== to ========== zygote: Don't CHECK if browser gone during zygote startup When the zygote starts it sends a "boot message" to the browser process across a socket. If the browser has already exited or crashed the sendmsg() will fail. We've seen this happen in the Chrome OS autotest lab. Instead of crashing the zygote, just log an error and cleanly exit. BUG=692227 TEST=add sleep to zygote startup, kill browser before zygote boot message is sent, verify zygote shuts down cleanly Review-Url: https://codereview.chromium.org/2851473005 Cr-Commit-Position: refs/heads/master@{#468027} Committed: https://chromium.googlesource.com/chromium/src/+/facab14158a0d26aa4ad59b5b547... ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001) as https://chromium.googlesource.com/chromium/src/+/facab14158a0d26aa4ad59b5b547... |
