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

Issue 862582 link

Starred by 1 user
Status: Fixed
Owner:
w...@chromium.org
Closed: Jul 12
Cc:
█ kbr@chromium.org
robliao@chromium.org
gab@chromium.org
fdoray@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac , Fuchsia
Pri: 1
Type: Bug



Sign in to add a comment

TaskSchedulerSingleThreadTaskRunnerManagerStartTest.PostTaskBeforeStart flaky on Fuchsia x64

Project Member Reported by w...@chromium.org, Jul 11 
In try https://ci.chromium.org/p/chromium/builders/luci.chromium.try/fuchsia_x64/58619 this flaked with:

[ RUN      ] TaskSchedulerSingleThreadTaskRunnerManagerStartTest.PostTaskBeforeStart
[80448:6893521:0709/183824.522289:46254667:FATAL:scheduler_worker.cc(92)] Check failed: !should_exit_.IsSet().
#00: base::debug::StackTrace::StackTrace(unsigned long) at ../../base/debug/stack_trace_fuchsia.cc:173
#01: endl<char, std::__2::char_traits<char> > at ../../buildtools/third_party/libc++/trunk/include/ostream:1001
      (inlined by) operator<< at ../../buildtools/third_party/libc++/trunk/include/ostream:195
      (inlined by) ~LogMessage at ../../base/logging.cc:593
#02: WakeUp at ../../base/task_scheduler/scheduler_worker.cc:93
#03: Release at ../../base/memory/ref_counted.h:385
      (inlined by) Release at ../../base/memory/scoped_refptr.h:284
      (inlined by) ~scoped_refptr at ../../base/memory/scoped_refptr.h:208
      (inlined by) Start at ../../base/task_scheduler/scheduler_single_thread_task_runner_manager.cc:430
#04: TestBody at ../../base/task_scheduler/scheduler_single_thread_task_runner_manager_unittest.cc:655
#05: os_stack_trace_getter at ../../third_party/googletest/src/googletest/src/gtest.cc:5332
      (inlined by) Run at ../../third_party/googletest/src/googletest/src/gtest.cc:2498
#06: os_stack_trace_getter at ../../third_party/googletest/src/googletest/src/gtest.cc:5332
      (inlined by) Run at ../../third_party/googletest/src/googletest/src/gtest.cc:2671
#07: Run at ../../third_party/googletest/src/googletest/src/gtest.cc:2784
#08: RunAllTests at ../../third_party/googletest/src/googletest/src/gtest.cc:5046
#09: Run at ./../../third_party/googletest/src/googletest/src/gtest.cc:?
#10: RUN_ALL_TESTS at ../../third_party/googletest/src/googletest/include/gtest/gtest.h:2329
      (inlined by) Run at ../../base/test/test_suite.cc:277
#11: Run at ../../base/callback.h:99
      (inlined by) LaunchUnitTestsInternal at ../../base/test/launcher/unit_test_launcher.cc:225
#12: LaunchUnitTests at ../../base/test/launcher/unit_test_launcher.cc:576
#13: main at ../../base/test/run_all_base_unittests.cc:12
#14: pc 0x6f412a940e0e (libc.so,0x19e0e)
[00046.315] pkgsvr: 2018/07/09 18:38:24 pkgfs:unsupported(/packages/base_unittests/0): dir unlink "app.log"

[00046.325] pkgsvr: 2018/07/09 18:38:24 pkgfs:unsupported(/packages/base_unittests/0): dir unlink "app.log"
[1653/3112] TaskSchedulerSingleThreadTaskRunnerManagerStartTest.PostTaskBeforeStart (CRASHED)

Note that there are already test-ordering-dependent ASAN reports in  issue 763055  and  issue 763100 , and an Android+ASAN report in issue 754676, although those look to have (in principle) been resolved by robliao@'s https://chromium-review.googlesource.com/773299.

Comment 1 by w...@chromium.org, Jul 11

Owner: w...@chromium.org
Status: Started (was: Untriaged)
Looks like what's happening here is:

1. SingleThreadTaskRunner is created and added to the manager's |workers_|.
2. Task is posted to the new TaskRunner. The posted task takes a reference to the SingleThreadTaskRunner on which it will run.
3. Start() is called on the manager. There is one entry in |workers_|, for which:
4. Manager calls Start() on the worker, causing a new thread to be created and started.
5. This new thread immediately starts running tasks. Once the task posted above has run, the final reference to the SingleThreadTaskRunner is dropped, and the CleanUp() is called on its |worker_|, resulting in |should_exit_.Set()|.
6. Manager calls WakeUp() on the worker, which sees |should_exit_.IsSet()|. Kaboom.

Since Start() will create the worker thread, which will call GetWork() before falling-back to waiting for work to do, the call to WakeUp() is redundant, and as described above, is actively harmful, so we should remove it.

Comment 2 by w...@chromium.org, Jul 11

Cc: kbr@chromium.org
Labels: OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
Project Member

Comment 3 by bugdroid1@chromium.org, Jul 12 

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

commit 291f0a739f2c952d402f76d3750b9a2340269c18
Author: Wez <wez@chromium.org>
Date: Thu Jul 12 19:42:09 2018

Fix race-condition in SchedulerSingleThreadTaskRunnerManager::Start().

SchedulerSingleThreadTaskRunnerManager allows SingleThreadTaskRunners to
be created and tasks posted to them before the worker threads are
actually started. The call to Start() the required workers was calling
both Start() and WakeUp() on each of them, the latter being unnecessary
since if a worker needs to process work then it must already have been
woken, when the work was PostTask()d into it.

Besides removing an unnecessary wake-up, this resolves a subtle race-
condition between wake-up and a potential worker clean-up (see bug).

Bug:  862582 
Change-Id: I9c4246cec4b1bda6d728e5e1656bb2c5ca7c0f0a
Reviewed-on: https://chromium-review.googlesource.com/1133279
Commit-Queue: Wez <wez@chromium.org>
Reviewed-by: Robert Liao <robliao@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#574681}
[modify] https://crrev.com/291f0a739f2c952d402f76d3750b9a2340269c18/base/task_scheduler/scheduler_single_thread_task_runner_manager.cc

Comment 4 by w...@chromium.org, Jul 12

Status: Fixed (was: Started)
►

Sign in to add a comment