New issue
Advanced search Search tips

Issue 826902 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner: ----
Closed: Oct 3
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

[Blink scheduler] TaskAnnotator::DidQueueTask invoked multiple times for same PendingTask

Project Member Reported by gab@chromium.org, Mar 28 2018

Issue description

Chrome Version: ToT @ r545041

TaskAnnotator::DidQueueTask should be invoked once per PendingTask. TaskQueueImpl can invoke it multiple times for the same PendingTask.

This seems to occur for delayed tasks which are "queued" twice (once on delayed queue, and once again on immediate queue when delay expires).

Caught by https://chromium-review.googlesource.com/c/chromium/src/+/982496

Adding a DCHECK that DidQueueTask is only invoked once per task fires @

        base::debug::BreakDebugger [0x7324E5BC+12] (D:\src\chrome\src\base\debug\debugger_win.cc:21)
        logging::LogMessage::~LogMessage [0x732773DF+735] (D:\src\chrome\src\base\logging.cc:855)
        base::debug::TaskAnnotator::DidQueueTask [0x73250F29+137] (D:\src\chrome\src\base\debug\task_annotator.cc:49)
        blink::scheduler::internal::ThreadControllerImpl::DidQueueTask [0x5EA8DDD4+20] (D:\src\chrome\src\third_party\WebKit\Source\platform\scheduler\base\thread_controller_impl.cc:139)
        blink::scheduler::internal::TaskQueueImpl::PushOntoDelayedIncomingQueueFromMainThread [0x5EA7F033+45] (D:\src\chrome\src\third_party\WebKit\Source\platform\scheduler\base\task_queue_impl.cc:255)
        blink::scheduler::internal::TaskQueueImpl::ScheduleDelayedWorkTask [0x5EA7F68A+270] (D:\src\chrome\src\third_party\WebKit\Source\platform\scheduler\base\task_queue_impl.cc:295)
        base::internal::FunctorTraits<void (blink::scheduler::internal::TaskQueueImpl::*)(blink::scheduler::internal::TaskQueueImpl::Task) __attribute__((thiscall)),void>::Invoke<blink::scheduler::internal::TaskQueueImpl *,blink::scheduler::internal::TaskQueueImp [0x5EA83FDE+66] (D:\src\chrome\src\base\bind_internal.h:447)
        base::internal::Invoker<base::internal::BindState<void (blink::scheduler::internal::TaskQueueImpl::*)(blink::scheduler::internal::TaskQueueImpl::Task) __attribute__((thiscall)),base::internal::UnretainedWrapper<blink::scheduler::internal::TaskQueueImpl>,b [0x5EA83F8A+48] (D:\src\chrome\src\base\bind_internal.h:572)
        base::debug::TaskAnnotator::RunTask [0x73251274+308] (D:\src\chrome\src\base\debug\task_annotator.cc:94)
        blink::scheduler::internal::ThreadControllerImpl::DoWork [0x5EA8D063+413] (D:\src\chrome\src\third_party\WebKit\Source\platform\scheduler\base\thread_controller_impl.cc:164)
        base::internal::Invoker<base::internal::BindState<void (blink::scheduler::internal::ThreadControllerImpl::*)(blink::scheduler::internal::SequencedTaskSource::WorkType) __attribute__((thiscall)),base::WeakPtr<blink::scheduler::internal::ThreadControllerImp [0x5EA8E2CE+60] (D:\src\chrome\src\base\bind_internal.h:589)
        base::debug::TaskAnnotator::RunTask [0x73251274+308] (D:\src\chrome\src\base\debug\task_annotator.cc:94)
        base::internal::IncomingTaskQueue::RunTask [0x73285CB9+105] (D:\src\chrome\src\base\message_loop\incoming_task_queue.cc:124)
        base::MessageLoop::RunTask [0x73289B07+519] (D:\src\chrome\src\base\message_loop\message_loop.cc:355)
        base::MessageLoop::DeferOrRunPendingTask [0x7328A05D+157] (D:\src\chrome\src\base\message_loop\message_loop.cc:364)
        base::MessageLoop::DoWork [0x7328A28A+506] (D:\src\chrome\src\base\message_loop\message_loop.cc:408)
        base::MessagePumpDefault::Run [0x7328C19C+156] (D:\src\chrome\src\base\message_loop\message_pump_default.cc:38)
        base::MessageLoop::Run [0x732894A9+169] (D:\src\chrome\src\base\message_loop\message_loop.cc:308)
        base::RunLoop::Run [0x732C70FC+204] (D:\src\chrome\src\base\run_loop.cc:133)
        content::RendererMain [0x63D2BFE2+1058] (D:\src\chrome\src\content\renderer\renderer_main.cc:247)
        content::RunNamedProcessTypeMain [0x63EAF134+148] (D:\src\chrome\src\content\app\content_main_runner.cc:423)
        content::ContentMainRunnerImpl::Run [0x63EAF699+285] (D:\src\chrome\src\content\app\content_main_runner.cc:703)
        service_manager::Main [0x5D2B1C7C+699] (D:\src\chrome\src\services\service_manager\embedder\main.cc:453)
        content::ContentMain [0x63EAF045+53] (D:\src\chrome\src\content\app\content_main.cc:19)
        content::LaunchTests [0x0350A456+313] (D:\src\chrome\src\content\public\test\test_launcher.cc:615)
        LaunchChromeTests [0x04FC789B+263] (D:\src\chrome\src\chrome\test\base\chrome_test_launcher.cc:171)
        main [0x04FC748B+115] (D:\src\chrome\src\chrome\test\base\browser_tests_main.cc:36)
        __scrt_common_main_seh [0x04FCB109+248] (f:\dd\vctools\crt\vcstartup\src\startup\exe_common.inl:283)
        BaseThreadInitThunk [0x767162C4+36]
        RtlSubscribeWnfStateChangeNotification [0x77690F79+1081]
        RtlSubscribeWnfStateChangeNotification [0x77690F44+1028]
 
Status: Available (was: Assigned)
Owner: ----
Project Member

Comment 3 by bugdroid1@chromium.org, Apr 5 2018

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

commit ae4f9ce34ec9a809f683be15d7b6957070549031
Author: Gabriel Charette <gab@chromium.org>
Date: Thu Apr 05 19:00:01 2018

Move PendingTask::task_backtrace management completely to TaskAnnotator.

This fixes 3 things:
 1) No longer need a friended private variable on MessageLoop
    (this facilitates crbug.com/825327)
 2) Support backtraces that go through TaskScheduler (and eventual other
    TaskAnnotator users)
 3) Support backtraces for tasks posted when returning from a nested
    loop (MessageLoop would previously set the field to null rather than
    "previous" when returning from a task).

New TaskAnnotatorBacktraceIntegrationTests are largely based on previous
PendingTaskTests (plus testing support beyond simple MessageLoop/Thread).

Prefered making PendingTask::task_backtrace mutable to forcing all
PostTask annotations (TaskAnnotator::DidQueueTask and TaskTracker::WillPostTask)
to use a non-const pointer given this doesn't really modify the state of
the PendingTask but rather some internal tracing state.

This also revealed  issue 826902 .

Bug:  825987 , 825327,  826902 
Change-Id: Iae24c8d8745a6dadb2185f5c581fb1ff1d5b3f23
Reviewed-on: https://chromium-review.googlesource.com/982496
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: danakj <danakj@chromium.org>
Reviewed-by: kylechar <kylechar@chromium.org>
Cr-Commit-Position: refs/heads/master@{#548512}
[modify] https://crrev.com/ae4f9ce34ec9a809f683be15d7b6957070549031/base/BUILD.gn
[modify] https://crrev.com/ae4f9ce34ec9a809f683be15d7b6957070549031/base/debug/task_annotator.cc
[modify] https://crrev.com/ae4f9ce34ec9a809f683be15d7b6957070549031/base/debug/task_annotator.h
[modify] https://crrev.com/ae4f9ce34ec9a809f683be15d7b6957070549031/base/debug/task_annotator_unittest.cc
[modify] https://crrev.com/ae4f9ce34ec9a809f683be15d7b6957070549031/base/message_loop/message_loop.cc
[modify] https://crrev.com/ae4f9ce34ec9a809f683be15d7b6957070549031/base/message_loop/message_loop.h
[modify] https://crrev.com/ae4f9ce34ec9a809f683be15d7b6957070549031/base/pending_task.cc
[modify] https://crrev.com/ae4f9ce34ec9a809f683be15d7b6957070549031/base/pending_task.h
[delete] https://crrev.com/707b08d5df848b53efc5fb35616048df0444d203/base/pending_task_unittest.cc

Labels: -Pri-3 Pri-2
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 2

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

commit 2f811807dfa95df471a01e78020e776d0ab2614f
Author: Alex Clarke <alexclarke@chromium.org>
Date: Tue Oct 02 15:19:30 2018

SequenceManager: Cross thread delayed task calls WillQueueTask once

Bug:  826902 
Change-Id: Ic79292edea1d7d92935797103ded169cb114e1cf
Reviewed-on: https://chromium-review.googlesource.com/1254542
Commit-Queue: Alex Clarke <alexclarke@chromium.org>
Reviewed-by: Alexander Timin <altimin@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#595848}
[modify] https://crrev.com/2f811807dfa95df471a01e78020e776d0ab2614f/base/debug/task_annotator.cc
[modify] https://crrev.com/2f811807dfa95df471a01e78020e776d0ab2614f/base/task/sequence_manager/task_queue_impl.cc
[modify] https://crrev.com/2f811807dfa95df471a01e78020e776d0ab2614f/base/task/sequence_manager/task_queue_impl.h

Status: Fixed (was: Available)

Sign in to add a comment