New issue
Advanced search Search tips

Issue 896990 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jan 19
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , iOS , Chrome , Mac , Fuchsia
Pri: 3
Type: Bug

Blocking:
issue 684640



Sign in to add a comment

Issues while using SetTaskRunner on a RepeatingTimer

Project Member Reported by sebmarchand@chromium.org, Oct 19

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.
 
Cc: fdoray@chromium.org
fdoray: Can you triage?
fdoray: ping?
Cc: -fdoray@chromium.org
Owner: fdoray@chromium.org
Status: Assigned (was: Untriaged)
Cc: -gab@chromium.org fdoray@chromium.org
Owner: gab@chromium.org
RepeatingTimer::SetTaskRunner is racy. We should use = delete to prevent calls to this API.

Re-assigning to gab@ who is working on timers.
Blocking: 684640
Project Member

Comment 8 by bugdroid1@chromium.org, 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

Project Member

Comment 9 by bugdroid1@chromium.org, 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

Project Member

Comment 10 by bugdroid1@chromium.org, 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

Comment 11 by gab@chromium.org, Jan 19 (3 days ago)

Status: Fixed (was: Assigned)
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