Issues while using SetTaskRunner on a RepeatingTimer |
|||||
Issue description
I've tried using SetTaskRunner on a RepeatingTimer (before doing anything else with it) and it's failing consistently as soon as timer triggers for the first time with the following stack trace:
Check failed: origin_sequence_checker_.CalledOnValidSequence().
Backtrace:
base::debug::StackTrace::StackTrace [0x63E5C3B6+102] (C:\src\chrome\src\base\debug\stack_trace_win.cc:289)
base::debug::StackTrace::StackTrace [0x63E5C33B+27] (C:\src\chrome\src\base\debug\stack_trace.cc:203)
logging::LogMessage::~LogMessage [0x63EC0B24+148] (C:\src\chrome\src\base\logging.cc:591)
base::internal::TimerBase::GetCurrentDelay [0x641052F9+201] (C:\src\chrome\src\base\timer\timer.cc:99)
base::RepeatingTimer::RunUserTask [0x6410635A+74] (C:\src\chrome\src\base\timer\timer.cc:301)
base::internal::TimerBase::RunScheduledTask [0x64105C25+293] (C:\src\chrome\src\base\timer\timer.cc:232)
base::internal::BaseTimerTaskInternal::Run [0x64105A8C+60] (C:\src\chrome\src\base\timer\timer.cc:50)
base::internal::FunctorTraits<void (base::internal::BaseTimerTaskInternal::*)() __attribute__((thiscall)),void>::Invoke<void (base::internal::BaseTimerTaskInternal::*)() __attribute__((thiscall)),base::internal::BaseTimerTaskInternal *> [0x64107DBC+28] (C:\src\chrome\src\base\bind_internal.h:516)
base::internal::InvokeHelper<0,void>::MakeItSo<void (base::internal::BaseTimerTaskInternal::*)() __attribute__((thiscall)),base::internal::BaseTimerTaskInternal *> [0x64107D1F+79] (C:\src\chrome\src\base\bind_internal.h:616)
base::internal::Invoker<base::internal::BindState<void (base::internal::BaseTimerTaskInternal::*)() __attribute__((thiscall)),base::internal::OwnedWrapper<base::internal::BaseTimerTaskInternal> >,void ()>::RunImpl<void (base::internal::BaseTimerTaskIntern [0x64107C75+85] (C:\src\chrome\src\base\bind_internal.h:689)
base::internal::Invoker<base::internal::BindState<void (base::internal::BaseTimerTaskInternal::*)() __attribute__((thiscall)),base::internal::OwnedWrapper<base::internal::BaseTimerTaskInternal> >,void ()>::RunOnce [0x64107B24+84] (C:\src\chrome\src\base\bind_internal.h:658)
base::OnceCallback<void ()>::Run [0x63DF6210+80] (C:\src\chrome\src\base\callback.h:100)
base::debug::TaskAnnotator::RunTask [0x63E618B3+1075] (C:\src\chrome\src\base\debug\task_annotator.cc:101)
base::internal::TaskTracker::RunOrSkipTask [0x640C8052+2690] (C:\src\chrome\src\base\task\task_scheduler\task_tracker.cc:654)
base::internal::TaskTracker::RunAndPopNextTask [0x640C660E+638] (C:\src\chrome\src\base\task\task_scheduler\task_tracker.cc:508)
base::internal::SchedulerWorker::RunWorker [0x640994C3+2147] (C:\src\chrome\src\base\task\task_scheduler\scheduler_worker.cc:331)
base::internal::SchedulerWorker::RunBackgroundPooledWorker [0x64098A02+34] (C:\src\chrome\src\base\task\task_scheduler\scheduler_worker.cc:231)
base::internal::SchedulerWorker::ThreadMain [0x640988FA+90] (C:\src\chrome\src\base\task\task_scheduler\scheduler_worker.cc:183)
base::`anonymous namespace'::ThreadFunc [0x640DF446+310] (C:\src\chrome\src\base\threading\platform_thread_win.cc:102)
BaseThreadInitThunk [0x774E62C4+36]
RtlSubscribeWnfStateChangeNotification [0x77871F69+1081]
RtlSubscribeWnfStateChangeNotification [0x77871F34+1028]
The problem seems to be (here TR=TaskRunner's Sequence):
- I create my timer on TR1, make it runs its tasks on TR2. My timer's origin_sequence_checker_ is not set at this point (as we detach in the timer's constructor).
- I call RepeatingTimer::Start on TR1, this calls TimerBase::StartInternal which set origin_sequence_checker_ to TR1.
- When my timer fires it calls RepeatingTimer::PostNewScheduledTask(GetCurrentDelay()) on TR2. This causes the call to origin_sequence_checker_.CalledOnValidSequence() in GetCurrentDelay to fail because we're on TR2 and origin_sequence_checker_ is associated with TR1.
,
Oct 19
This seems to be the only non-test where we use RepeatingTimer::SetTaskRunner : https://cs.chromium.org/chromium/src/chrome/browser/chromeos/printing/cups_print_job_manager_impl.cc?l=267&rcl=2e48fc9c4e3c1b6b8e5fea07277a7da785abd75f
,
Oct 29
fdoray: Can you triage?
,
Dec 3
fdoray: ping?
,
Dec 6
,
Dec 10
RepeatingTimer::SetTaskRunner is racy. We should use = delete to prevent calls to this API. Re-assigning to gab@ who is working on timers.
,
Jan 4
,
Jan 18
(4 days ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5304ffc0b4d5a79db7afa308bfab498e717ad4d9 commit 5304ffc0b4d5a79db7afa308bfab498e717ad4d9 Author: Gabriel Charette <gab@chromium.org> Date: Fri Jan 18 22:36:43 2019 [LoopbackStream] Don't make an explicit but redundant call to Timer::SetTaskRunner() SetTaskRunner() is racy and the API will soon be restricted to same thread usage to mock the clock in tests only : https://chromium-review.googlesource.com/c/chromium/src/+/1394311 In this case it wasn't used racily (but the upcoming stricter API will prevent making this call before Start()'ing the Timer elsewhere). As-is this call was redundant as it's the sequence the Timer is *started* on which is bound to it (and in this case Start() is always called from |flow_task_runner_| so this was already the case). R=olka@chromium.org Bug: 587199 , 896990 Change-Id: I009819509fea8336ce426b81b932d85f5cad6d55 Reviewed-on: https://chromium-review.googlesource.com/c/1416978 Commit-Queue: Gabriel Charette <gab@chromium.org> Reviewed-by: Max Morin <maxmorin@chromium.org> Cr-Commit-Position: refs/heads/master@{#624324} [modify] https://crrev.com/5304ffc0b4d5a79db7afa308bfab498e717ad4d9/services/audio/loopback_stream.cc
,
Jan 19
(4 days ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/2282c04ec121209372e18a87f32d7f439a974a99 commit 2282c04ec121209372e18a87f32d7f439a974a99 Author: Gabriel Charette <gab@chromium.org> Date: Sat Jan 19 00:57:34 2019 [VideoFrameCompositor] Don't make an explicit but redundant call to Timer::SetTaskRunner() SetTaskRunner() is racy and the API will soon be restricted to same thread usage to mock the clock in tests only : https://chromium-review.googlesource.com/c/chromium/src/+/1394311 In this case it wasn't used racily (but the upcoming stricter API will prevent making this call before Start()'ing the Timer elsewhere). As-is this call was redundant as it's the sequence the Timer is *started* on which is bound to it (and in this case Reset() is always called from |task_runner| so this was already the case). And VideoFrameCompositor::BackgroundRender()'s check also proves this correct. Bug: 587199 , 896990 Change-Id: Iddd0353ba9ddf587c128ff1b594cd9c15a2d7662 Reviewed-on: https://chromium-review.googlesource.com/c/1415131 Reviewed-by: Matthew Wolenetz <wolenetz@chromium.org> Commit-Queue: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#624389} [modify] https://crrev.com/2282c04ec121209372e18a87f32d7f439a974a99/media/blink/video_frame_compositor.cc
,
Jan 19
(3 days ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/e92ccc00d7bbed2632e518d9d2b57edbff93d6d1 commit e92ccc00d7bbed2632e518d9d2b57edbff93d6d1 Author: Gabriel Charette <gab@chromium.org> Date: Sat Jan 19 14:13:05 2019 [base] Make Timer::SetTaskRunner sequence-affine. This is a prereq to make all of base::Timer sequence-affine. Ideally we would get rid of Timer::SetTaskRunner but it's used in many tests to mock the clock (by redirecting the delayed task to a TestSimpleTaskRunner or TestMockTimeTaskRunner explicitly managed by the test). Hence this CL bans multi-threaded use cases while still allowing same-thread use cases for old tests. Bug: 587199 , 896990 Change-Id: I1785ff404f115309ea6eec743004709048d52f07 Reviewed-on: https://chromium-review.googlesource.com/c/1394311 Commit-Queue: Gabriel Charette <gab@chromium.org> Auto-Submit: Gabriel Charette <gab@chromium.org> Reviewed-by: François Doray <fdoray@chromium.org> Cr-Commit-Position: refs/heads/master@{#624470} [modify] https://crrev.com/e92ccc00d7bbed2632e518d9d2b57edbff93d6d1/base/timer/timer.cc [modify] https://crrev.com/e92ccc00d7bbed2632e518d9d2b57edbff93d6d1/base/timer/timer.h [modify] https://crrev.com/e92ccc00d7bbed2632e518d9d2b57edbff93d6d1/base/timer/timer_unittest.cc
,
Jan 19
(3 days ago)
Fixed (by banning this use case). SetTaskRunner() is now a test-only API for an old approach to mock time (see CL). base::Timer will soon be sequence-affine. As such, to have the Timer fire on a different TaskRunner, you'll need to Start() it there. |
|||||
►
Sign in to add a comment |
|||||
Comment 1 by sebmarchand@chromium.org
, Oct 19