Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2827783003:
Initialize RenderSandboxHostLinux in --no-zygote mode to not crash. (Closed)
|
Created:
3 years, 8 months ago by zoeclifford Modified:
3 years, 8 months ago CC:
chromium-reviews, jam, darin-cc_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionInitialize RenderSandboxHostLinux in --no-zygote mode to not crash.
http://crrev.com/2384163002 skipped initializing RenderSandboxHostLinux
If the sandbox and zygote are both disabled. But the RendererSocket is
used even in this case by the Linux ChildProcessLauncherHelper, so it
lead to a crash.
Instead initialize the RenderSandboxHostLinux unconditionally. This
appears to be what some code expects in multi-process code.
Tested:
cr run chrome --no-zygote --no-sandbox http://www.google.com
BUG=712779
Review-Url: https://codereview.chromium.org/2827783003
Cr-Commit-Position: refs/heads/master@{#466168}
Committed: https://chromium.googlesource.com/chromium/src/+/9dc1413a9f8a803109530c02aeb4429a8856e525
Patch Set 1 #Patch Set 2 : Don't crash in --no-zygote mode. #Patch Set 3 : Disable the test under TSan. #
Messages
Total messages: 28 (15 generated)
Description was changed from ========== Initialize RenderSandboxHostLinux in --no-zygote mode to not crash. http://crrev.com/2384163002 skipped initializing RenderSandboxHostLinux If the sandbox and zygote are both disabled. But the RendererSocket is used even in this case by the Linux ChildProcessLauncherHelper, so it lead to a crash. Instead initialize the RenderSandboxHostLinux unconditionally. This appears to be what some code expects in multi-process code. BUG=712779 ========== to ========== Initialize RenderSandboxHostLinux in --no-zygote mode to not crash. http://crrev.com/2384163002 skipped initializing RenderSandboxHostLinux If the sandbox and zygote are both disabled. But the RendererSocket is used even in this case by the Linux ChildProcessLauncherHelper, so it lead to a crash. Instead initialize the RenderSandboxHostLinux unconditionally. This appears to be what some code expects in multi-process code. Tested: cr run chrome --no-zygote --no-sandbox http://www.google.com BUG=712779 ==========
zoeclifford@chromium.org changed reviewers: + skyostil@chromium.org
Does this change look reasonable? Running it by skyostil@ (who wrote http://crrev.com/2384163002) before adding owner reviewers.
lgtm, thanks for tracking this down!
zoeclifford@chromium.org changed reviewers: + creis@chromium.org
RS LGTM. Is this something we could include a test for, to prevent regressions if this code changes in the future?
On 2017/04/19 17:15:56, Charlie Reis wrote: > RS LGTM. > > Is this something we could include a test for, to prevent regressions if this > code changes in the future? Test added. Testing this was a lot more straightforward than I thought it would be. Does it look OK?
Yep-- if that crashes without your fix, then that's perfect. Thanks! LGTM.
The CQ bit was checked by zoeclifford@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: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/04/19 19:18:46, Charlie Reis wrote: > Yep-- if that crashes without your fix, then that's perfect. Thanks! > > LGTM. Yeah it crashes without the fix. Unfortunately the trybots show that it also fails with the fix in TSan mode :(. There is a reported data race over gfx::(anonymous namespace)::device_scale_factor_. Between gfx::GetFontRenderParams on the sandbox IPC thread, and "[failed to restore the stack]" on the browser thread. Would it be OK to mark this test as TSAN disabled for now? Actually I was planning to try and make the renderer's font code not use the sandbox IPC when zygote is disabled in a follow up change (see bug for why).
On 2017/04/19 23:57:06, zoeclifford wrote: > Would it be OK to mark this test as TSAN disabled for now? Actually I was > planning to try and make the renderer's font code not use the sandbox IPC when > zygote is disabled in a follow up change (see bug for why). This seems reasonable since it sounds like the race won't be an issue anymore after that change, right?
On 2017/04/20 10:15:28, Sami wrote: > On 2017/04/19 23:57:06, zoeclifford wrote: > > Would it be OK to mark this test as TSAN disabled for now? Actually I was > > planning to try and make the renderer's font code not use the sandbox IPC when > > zygote is disabled in a follow up change (see bug for why). > > This seems reasonable since it sounds like the race won't be an issue anymore > after that change, right? Sure. If you can, list a bug number for the font code change when marking the test as TSAN disabled. Thanks!
On 2017/04/20 16:56:22, Charlie Reis wrote: > On 2017/04/20 10:15:28, Sami wrote: > > On 2017/04/19 23:57:06, zoeclifford wrote: > > > Would it be OK to mark this test as TSAN disabled for now? Actually I was > > > planning to try and make the renderer's font code not use the sandbox IPC > when > > > zygote is disabled in a follow up change (see bug for why). > > > > This seems reasonable since it sounds like the race won't be an issue anymore > > after that change, right? > > Sure. If you can, list a bug number for the font code change when marking the > test as TSAN disabled. Thanks! I've disabled this test, added a bug number, and mentioned the data race in the bug. After the follow up change it shouldn't get triggered (tested locally). Basically I'm thinking of making RendererBlinkPlatformImpl::sandboxEnabled return false in no-zygote mode. Of course there's a chance the reviewer won't be happy with that approach! By the way, creis@, would you be a good person to review the follow-up CL? It looks like it doesn't get touched too frequently, so I'm not sure who a good reviewer would be.
The CQ bit was checked by zoeclifford@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...
On 2017/04/20 21:16:20, zoeclifford wrote: > On 2017/04/20 16:56:22, Charlie Reis wrote: > > On 2017/04/20 10:15:28, Sami wrote: > > > On 2017/04/19 23:57:06, zoeclifford wrote: > > > > Would it be OK to mark this test as TSAN disabled for now? Actually I was > > > > planning to try and make the renderer's font code not use the sandbox IPC > > when > > > > zygote is disabled in a follow up change (see bug for why). > > > > > > This seems reasonable since it sounds like the race won't be an issue > anymore > > > after that change, right? > > > > Sure. If you can, list a bug number for the font code change when marking the > > test as TSAN disabled. Thanks! > > I've disabled this test, added a bug number, and mentioned > the data race in the bug. > > After the follow up change it shouldn't get triggered (tested locally). > Basically I'm thinking of making RendererBlinkPlatformImpl::sandboxEnabled > return false in no-zygote mode. Of course there's a chance the reviewer > won't be happy with that approach! > > By the way, creis@, would you be a good person to review the follow-up > CL? It looks like it doesn't get touched too frequently, so I'm not > sure who a good reviewer would be. I don't work on the sandbox myself, so I'm not a good reviewer here. You might start with rsesek@ or kerrnel@, and they can probably recommend someone if it's beyond what they know. Hope that helps!
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 zoeclifford@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from skyostil@chromium.org, creis@chromium.org Link to the patchset: https://codereview.chromium.org/2827783003/#ps40001 (title: "Disable the test under TSan.")
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": 40001, "attempt_start_ts": 1492727452039290,
"parent_rev": "466e48c8e7f8fbfc3ba625000881094dd864a8da", "commit_rev":
"9dc1413a9f8a803109530c02aeb4429a8856e525"}
Message was sent while issue was closed.
Description was changed from ========== Initialize RenderSandboxHostLinux in --no-zygote mode to not crash. http://crrev.com/2384163002 skipped initializing RenderSandboxHostLinux If the sandbox and zygote are both disabled. But the RendererSocket is used even in this case by the Linux ChildProcessLauncherHelper, so it lead to a crash. Instead initialize the RenderSandboxHostLinux unconditionally. This appears to be what some code expects in multi-process code. Tested: cr run chrome --no-zygote --no-sandbox http://www.google.com BUG=712779 ========== to ========== Initialize RenderSandboxHostLinux in --no-zygote mode to not crash. http://crrev.com/2384163002 skipped initializing RenderSandboxHostLinux If the sandbox and zygote are both disabled. But the RendererSocket is used even in this case by the Linux ChildProcessLauncherHelper, so it lead to a crash. Instead initialize the RenderSandboxHostLinux unconditionally. This appears to be what some code expects in multi-process code. Tested: cr run chrome --no-zygote --no-sandbox http://www.google.com BUG=712779 Review-Url: https://codereview.chromium.org/2827783003 Cr-Commit-Position: refs/heads/master@{#466168} Committed: https://chromium.googlesource.com/chromium/src/+/9dc1413a9f8a803109530c02aeb4... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/9dc1413a9f8a803109530c02aeb4... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
