Issue metadata
Sign in to add a comment
|
NoSessionRestoreTest.LocalStorageClearedOnExit flakilly DCHECKs in TaskScheduler code |
||||||||||||||||||||||
Issue descriptionSomewhat infrequent this browser test fails with the following assertion/stack trace: [22192:22216:0531/102034.488463:FATAL:task_tracker.cc(413)] Check failed: IsPostingBlockShutdownTaskAfterShutdownAllowed(). #0 0x7f0a44d0fc1b base::debug::StackTrace::StackTrace() #1 0x7f0a44d0e91c base::debug::StackTrace::StackTrace() #2 0x7f0a44d824c3 logging::LogMessage::~LogMessage() #3 0x7f0a44edd2cb base::internal::TaskTracker::BeforePostTask() #4 0x7f0a44edcffa base::internal::TaskTracker::WillPostTask() #5 0x7f0a44ec764f base::internal::SchedulerSingleThreadTaskRunnerManager::SchedulerSingleThreadTaskRunner::PostDelayedTask() #6 0x7f0a44eb3f6d base::TaskRunner::PostTask() #7 0x7f0a44d39735 base::(anonymous namespace)::FilePathWatcherImpl::OnFilePathChanged() #8 0x7f0a44d3913f base::(anonymous namespace)::InotifyReader::OnInotifyEvent() #9 0x7f0a44d384aa base::(anonymous namespace)::InotifyReaderCallback() #10 0x7f0a44d38bd4 _ZN4base8internal13FunctorTraitsIPFvPNS_12_GLOBAL__N_113InotifyReaderEiEvE6InvokeIJS4_iEEEvS6_DpOT_ #11 0x7f0a44d38add _ZN4base8internal12InvokeHelperILb0EvE8MakeItSoIPFvPNS_12_GLOBAL__N_113InotifyReaderEiEJS6_iEEEvOT_DpOT0_ #12 0x7f0a44d38a63 _ZN4base8internal7InvokerINS0_9BindStateIPFvPNS_12_GLOBAL__N_113InotifyReaderEiEJS5_iEEEFvvEE7RunImplIS7_St5tupleIJS5_iEEJLm0ELm1EEEEvOT_OT0_NS_13In dexSequenceIJXspT1_EEEE #13 0x7f0a44d3895e _ZN4base8internal7InvokerINS0_9BindStateIPFvPNS_12_GLOBAL__N_113InotifyReaderEiEJS5_iEEEFvvEE7RunOnceEPNS0_13BindStateBaseE #14 0x7f0a44ccb3fe _ZNO4base8CallbackIFvvELNS_8internal8CopyModeE0ELNS2_10RepeatModeE0EE3RunEv #15 0x7f0a44d1586e base::debug::TaskAnnotator::RunTask() #16 0x7f0a44dac32d base::MessageLoop::RunTask() #17 0x7f0a44dac5b7 base::MessageLoop::DeferOrRunPendingTask() #18 0x7f0a44dac80d base::MessageLoop::DoWork() #19 0x7f0a44dbe928 base::MessagePumpDefault::Run() #20 0x7f0a44dabd68 base::MessageLoop::Run() #21 0x7f0a44e5578d base::RunLoop::Run() #22 0x7f0a44f05614 base::Thread::Run() #23 0x7f0a44f05e7a base::Thread::ThreadMain() #24 0x7f0a44eec66a base::(anonymous namespace)::ThreadFunc() #25 0x7f0a45199184 start_thread #26 0x7f0a2b7a2bed clone I wonder how realistic it is to even have a check for "has shutdown completed" in PostTask. Returning false seems reasonable, but as long as not every task is managed by the Task Scheduler it doesn't seem reasonable for it to try to assert that nothing posts shutdown blocking tasks after all the task scheduler managed shutdown blocking tasks have completed. I haven't looked too deeply into this occurence of this dcheck, but some other place I've ran across it what basically is happening is that code on the UI thread that happens to run after the task scheduler is shut down is trying to post a task to a task scheduler managed task runner, which dchecks if the task scheduler happened to have managed to flush all its tasks already. So since PostTask already has the contract to return false when the task is never going to be executed, it seems a bit odd that TaskRunners managed by the TaskScheduler actually DCHECK rather than returning false...
,
May 31 2017
If there is a flake, that suggests a bug: Sometimes the task gets to run, sometimes it doesn't. That could argue that the task shouldn't be blocking shutdown or some important work is not being done.
,
May 31 2017
Ah, yeah looking at this specific stack trace this is a case of a missing (desired) piece in our current API. The callbacks from FilePathWatcher really shouldn't be BLOCK_SHUTDOWN (nothing can guarantee them before shutdown already). But because sequences can only have one shutdown mode today, if the destination sequence is BLOCK_SHUTDOWN: so is every non-delayed task posted on it... What we would like is to allow multiple "post" sequences funnel into one "execution" sequence. The "post" sequences could then have different shutdown properties while still maintaining sequencing of execution from various callers. Until we have that I guess we have to disable that DCHECK.
,
May 31 2017
Ah yes, good point that posting shutdown blocking tasks after shutdown is probably a bug. Being able to specify task traits separate from the traits for the task runner the tasks are posting on (or something like that) does sounds like a sensible solution, and would also help with my other domstorage related issue (where one particular task gets posted from a destructor, and I don't care at all if that task runs before shutdown or not, but since the overall dom storage task runners are shutdown blocking that task is also).
,
May 31 2017
Actually this particular issue is a dupe of internal issue 698140, we knew this could theoretically trigger in the wild but hadn't heard of actual devs hitting this DCHECK in the wild. @robliao: that issue is marked as Started with you as owner, I forget what we had in mind?
,
May 31 2017
We were going to stop the service thread, but we ran into some lifetime and shutdown ordering issues, so we punted on that. I'll update the status to "Available".
,
May 31 2017
fdoray@ pointed out this is FilePathWatcher, not FileDescriptorWatcher. FilePathWatcher has its own thread and probably should be brought into the task scheduler.
,
May 31 2017
Separate from this issue or any other specific issue I think there are always going to be cases (even after everything is task-schedulerized) where you want to post non-shutdown-blocking tasks to a shutdown blocking task runner (or at least to a task runner sequenced with another sequenced task runner). (like this issue I really care about is in some new code I'm trying to land...)
,
May 31 2017
Right, I'll disable the DCHECK until we have a solution for that.
,
Jun 2 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b2b72df4bdc0217e7ed7cc321648028a429edf61 commit b2b72df4bdc0217e7ed7cc321648028a429edf61 Author: gab <gab@chromium.org> Date: Fri Jun 02 21:32:23 2017 Disable DCHECK for no BLOCK_SHUTDOWN posted after TaskScheduler shutdown. Until it's possible for async observation services to post non-BLOCK_SHUTDOWN tasks to otherwise BLOCK_SHUTDOWN sequences, this DCHECK will be flaky. clang-format off is required or I couldn't find a way to make it happy with the comment (either inside or outside the #if) BUG=728235, 698140 Review-Url: https://codereview.chromium.org/2916673003 Cr-Commit-Position: refs/heads/master@{#476799} [modify] https://crrev.com/b2b72df4bdc0217e7ed7cc321648028a429edf61/base/task_scheduler/task_tracker.cc [modify] https://crrev.com/b2b72df4bdc0217e7ed7cc321648028a429edf61/base/task_scheduler/task_tracker_unittest.cc
,
Jun 7 2017
,
May 3 2018
Once we have sequence funnelling, FilePathWatcher/FileDescriptorWatcher should be reporting back to a non-BLOCK_SHUTDOWN child sequence.
,
May 4 2018
Actually, I think we should just stop the service thread on shutdown and prevent such tasks from even being posted. Issue 698140 is tracking this, I don't recall why it's non-trivial... |
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by gab@chromium.org
, May 31 2017