New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 874387 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Aug 22
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: ----

Blocking:
issue 871924



Sign in to add a comment

SandboxTest.SpawnSandboxTarget (chrome_cleaner_unittests) is flaky

Project Member Reported by sheriff-...@appspot.gserviceaccount.com, Aug 15

Issue description

Filed by sheriff-o-matic@appspot.gserviceaccount.com on behalf of mstensho@chromium.org

chrome_cleaner_unittests failing on chromium.win/Win10 Tests x64 (dbg)

Builders failed on: 
- Win10 Tests x64 (dbg): 
  https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Win10%20Tests%20x64%20%28dbg%29

Flaky at least since: https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Win10%20Tests%20x64%20%28dbg%29/2373
(2018-08-08)

 
Owner: joenotcharles@chromium.org
Status: Assigned (was: Available)
SandboxTest.SpawnSandboxTarget was actually added on 2018-08-08, so it seems it's been flaky all along:
https://chromium-review.googlesource.com/1161283


Project Member

Comment 2 by bugdroid1@chromium.org, Aug 15

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/802385a55780c99158d6ad37d2f64a2b2772aeb2

commit 802385a55780c99158d6ad37d2f64a2b2772aeb2
Author: Morten Stenshorne <mstensho@chromium.org>
Date: Wed Aug 15 09:59:04 2018

Disable SandboxTest.SpawnSandboxTarget from chrome_cleaner_unittests

It's flaky.

TBR=joenotcharles@chromium.org

Bug:  874387 
Change-Id: I3056f49cb1879a6fbacd195f94dfe6992cfd6710
Reviewed-on: https://chromium-review.googlesource.com/1175123
Reviewed-by: Morten Stenshorne <mstensho@chromium.org>
Commit-Queue: Morten Stenshorne <mstensho@chromium.org>
Cr-Commit-Position: refs/heads/master@{#583209}
[modify] https://crrev.com/802385a55780c99158d6ad37d2f64a2b2772aeb2/chrome/chrome_cleaner/ipc/sandbox_unittest.cc

Labels: -Sheriff-Chromium
I think this code in sandbox.cc has a race condition:

  // Wait for the sandboxed process to signal it is ready, checking every now
  // and then to see if the process crashed before it could complete it's setup.
  int exit_code = -1;
  while (!init_done_event->TimedWait(base::TimeDelta::FromSeconds(1))) {
    if (process_handle.WaitForExitWithTimeout(base::TimeDelta(), &exit_code)) {
      LOG(ERROR)
          << "Sandboxed process exited before signaling it was initialized, "
             "exit code: "
          << exit_code;
      return RESULT_CODE_FAILED_TO_START_SANDBOX_PROCESS;
    }
  }

In tests the sandboxed process signals the event and then exits almost immediately. If the parent returns from TimedWait, then the sandbox process signals the event and exits before the call to process_handle.WaitForExitWithTimeout, the parent process will miss the signal.

Solution is to add both the event and process handles to WaitForMultipleObjects and then check which one was signaled. (See http://shortn/_I3Ua6xmMXA for a test that does this in the internal repo.)
Components: Services>Safebrowsing>ChromeCleanup
Cc: joenotcharles@chromium.org hubbe@google.com
 Issue 872440  has been merged into this issue.
Owner: csharp@chromium.org
Blocking: 871924
Labels: SafeBrowsing-Triaged
Project Member

Comment 10 by bugdroid1@chromium.org, Aug 22

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/84ed7b5aa12e9b73b9db07bc0d76420eb30b8820

commit 84ed7b5aa12e9b73b9db07bc0d76420eb30b8820
Author: Joe Mason <joenotcharles@chromium.org>
Date: Wed Aug 22 14:14:19 2018

Fix race condition in StartSandboxTarget

If the sandbox target process is very short-lived (as in some multiprocess
tests), and also takes long enough to spawn that the first TimedWait on the
init done notifier times out, then there's a race between the target process
exiting and the parent process calling TimedWait again. Instead of polling in a
loop, use WaitForMultipleObjects to wait for both the init done notifier and
process exit.

TBR=csharp
TBR_REASON=Already reviewed in internal repo.

Bug:  874387 
Change-Id: I86af661bbe43f3fc044fb5afd6bcbd9f9d7b297c
Reviewed-on: https://chromium-review.googlesource.com/1184214
Reviewed-by: Joe Mason <joenotcharles@chromium.org>
Reviewed-by: Chris Sharp <csharp@chromium.org>
Commit-Queue: Chris Sharp <csharp@chromium.org>
Cr-Commit-Position: refs/heads/master@{#585006}
[modify] https://crrev.com/84ed7b5aa12e9b73b9db07bc0d76420eb30b8820/chrome/chrome_cleaner/ipc/sandbox.cc

Status: Fixed (was: Assigned)

Sign in to add a comment