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

Issue 907177 link

Starred by 2 users
Status: Fixed
Owner:
etiennep@chromium.org
Closed: Nov 22
Cc:
█ kbr@chromium.org
rjkroege@chromium.org
etienneb@chromium.org
lgrey@chromium.org
wittman@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

Thread restriction is not valid for sampling profier

Project Member Reported by etienneb@chromium.org, Nov 20 
From Tot we are getting this error when slow-reports are activated.

[29924:43592:1120/133049.279:FATAL:thread_restrictions.cc(78)] Check failed: !g_blocking_disallowed.Get().Get(). To allow //base sync primitives in a scope where blocking is disallowed use ScopedAllowBaseSyncPrimitivesOutsideBlockingScope.


Backtrace:
    base::debug::StackTrace::StackTrace [0x00007FFCE1AB9C05+101] (C:/src/chromium/src/out/build\..\..\base\debug\stack_trace_win.cc:290)
    base::debug::StackTrace::StackTrace [0x00007FFCE1AB8BBD+29] (C:/src/chromium/src/out/build\..\..\base\debug\stack_trace.cc:203)
    logging::LogMessage::~LogMessage [0x00007FFCE1B0BD86+134] (C:/src/chromium/src/out/build\..\..\base\logging.cc:592)
    base::ScopedAllowBaseSyncPrimitives::ScopedAllowBaseSyncPrimitives [0x00007FFCE1D34A06+230] (C:/src/chromium/src/out/build\..\..\base\threading\thread_restrictions.cc:81)
    base::StackSamplingProfiler::~StackSamplingProfiler [0x00007FFCE1BFAA51+321] (C:/src/chromium/src/out/build\..\..\base\profiler\stack_sampling_profiler.cc:683)
    std::default_delete<base::StackSamplingProfiler>::operator() [0x00007FFCEFF0F9CD+45] (C:/src/chromium/src/out/build\..\..\third_party\depot_tools\win_toolchain\vs_files\3bc0ec615cf20ee342f3bc29bc991b5ad66d8d2c\VC\Tools\MSVC\14.14.26428\include\memory:2055)
    std::unique_ptr<base::StackSamplingProfiler,std::default_delete<base::StackSamplingProfiler> >::reset [0x00007FFCEFF0F354+100] (C:/src/chromium/src/out/build\..\..\third_party\depot_tools\win_toolchain\vs_files\3bc0ec615cf20ee342f3bc29bc991b5ad66d8d2c\VC\Tools\MSVC\14.14.26428\include\memory:2306)
    tracing::TracingSamplerProfiler::OnTraceLogDisabled [0x00007FFCEFF0F2DE+94] (C:/src/chromium/src/out/build\..\..\components\tracing\common\tracing_sampler_profiler.cc:207)
    base::internal::FunctorTraits<void (base::trace_event::TraceLog::AsyncEnabledStateObserver::*)(),void>::Invoke<void (base::trace_event::TraceLog::AsyncEnabledStateObserver::*)(),base::WeakPtr<base::trace_event::TraceLog::AsyncEnabledStateObserver>> [0x00007FFCE1DB7ADF+31] (C:/src/chromium/src/out/build\..\..\base\bind_internal.h:516)
    base::internal::InvokeHelper<1,void>::MakeItSo<void (base::trace_event::TraceLog::AsyncEnabledStateObserver::*)(),base::WeakPtr<base::trace_event::TraceLog::AsyncEnabledStateObserver>> [0x00007FFCE1DB7A1B+75] (C:/src/chromium/src/out/build\..\..\base\bind_internal.h:639)
    base::internal::Invoker<base::internal::BindState<void (base::trace_event::TraceLog::AsyncEnabledStateObserver::*)(),base::WeakPtr<base::trace_event::TraceLog::AsyncEnabledStateObserver> >,void ()>::RunImpl<void (base::trace_event::TraceLog::AsyncEnabledS [0x00007FFCE1DB79A9+73] (C:/src/chromium/src/out/build\..\..\base\bind_internal.h:689)
    base::internal::Invoker<base::internal::BindState<void (base::trace_event::TraceLog::AsyncEnabledStateObserver::*)(),base::WeakPtr<base::trace_event::TraceLog::AsyncEnabledStateObserver> >,void ()>::RunOnce [0x00007FFCE1DB78B6+70] (C:/src/chromium/src/out/build\..\..\base\bind_internal.h:658)
    base::OnceCallback<void ()>::Run [0x00007FFCE1A68A61+97] (C:/src/chromium/src/out/build\..\..\base\callback.h:100)
    base::debug::TaskAnnotator::RunTask [0x00007FFCE1ABBAD9+921] (C:/src/chromium/src/out/build\..\..\base\debug\task_annotator.cc:101)
    base::MessageLoopImpl::RunTask [0x00007FFCE1B3C4DF+959] (C:/src/chromium/src/out/build\..\..\base\message_loop\message_loop_impl.cc:462)
    base::MessageLoopImpl::DeferOrRunPendingTask [0x00007FFCE1B3CA43+83] (C:/src/chromium/src/out/build\..\..\base\message_loop\message_loop_impl.cc:476)
    base::MessageLoopImpl::DoWork [0x00007FFCE1B3D4DB+507] (C:/src/chromium/src/out/build\..\..\base\message_loop\message_loop_impl.cc:561)
    base::MessagePumpForIO::DoRunLoop [0x00007FFCE1B48785+37] (C:/src/chromium/src/out/build\..\..\base\message_loop\message_pump_win.cc:512)
    base::MessagePumpWin::Run [0x00007FFCE1B4608C+140] (C:/src/chromium/src/out/build\..\..\base\message_loop\message_pump_win.cc:54)
    base::MessageLoopImpl::Run [0x00007FFCE1B3BD74+532] (C:/src/chromium/src/out/build\..\..\base\message_loop\message_loop_impl.cc:416)
    base::RunLoop::Run [0x00007FFCE1C02E34+500] (C:/src/chromium/src/out/build\..\..\base\run_loop.cc:102)
    base::Thread::Run [0x00007FFCE1D24A8E+366] (C:/src/chromium/src/out/build\..\..\base\threading\thread.cc:250)
    content::BrowserProcessSubThread::IOThreadRun [0x00007FFCA609C128+56] (C:/src/chromium/src/out/build\..\..\content\browser\browser_process_sub_thread.cc:175)
    content::BrowserProcessSubThread::Run [0x00007FFCA609C011+305] (C:/src/chromium/src/out/build\..\..\content\browser\browser_process_sub_thread.cc:127)
    base::Thread::ThreadMain [0x00007FFCE1D25299+1289] (C:/src/chromium/src/out/build\..\..\base\threading\thread.cc:332)
    base::`anonymous namespace'::ThreadFunc [0x00007FFCE1D16E43+403] (C:/src/chromium/src/out/build\..\..\base\threading\platform_thread_win.cc:99)
    BaseThreadInitThunk [0x00007FFD0F601FE4+20]
    RtlUserThreadStart [0x00007FFD10D5CB31+33]

Comment 1 by etienneb@chromium.org, Nov 20

Cc: etienneb@chromium.org

Comment 2 by wittman@chromium.org, Nov 20 

I assume this is due to 50b50793ee7891ab8ff9a4bab66c0c43391aa4ad. Can we revert that change?

Comment 3 by etiennep@chromium.org, Nov 20 

Reverting 50b50793ee7891ab8ff9a4bab66c0c43391aa4ad.
Will reland with outside blocking scope allowance.
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 21 

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

commit 90927154fc2ebac2c77c0b43957bcbf55df40637
Author: Etienne Pierre-Doray <etiennep@chromium.org>
Date: Wed Nov 21 01:59:05 2018

Revert "[TaskScheduler]: Migrate off of ScopedAllowWait in /base/profiler/stack_sampling_profiler.cc"

This reverts commit 50b50793ee7891ab8ff9a4bab66c0c43391aa4ad.

Reason for revert: Thread restriction is not valid for sampling profier

Bug:  907177 

Original change's description:
> [TaskScheduler]: Migrate off of ScopedAllowWait in /base/profiler/stack_sampling_profiler.cc
> 
> base::ThreadRestrictions::ScopedAllowWait is deprecated in favor of its more
> explicit counterpart.
> 
> It should have been replaced by :
>  * base::ScopedAllowBaseSyncPrimitivesForTesting in test files.
>  * base::ScopedAllowSyncPrimitives in non-test files
>  * base::ScopedAllowSyncPrimitivesOutsideBlockingScope when it's used on threads
>    that don't allow blocking
> The last one is strongly frowned upon but this CL aims to document existing
> behavior rather than address it. Owners are encouraged to follow-up by fixing
> unnecessary waits and more particularly unnecessary waits
> outside-blocking-scope.
> 
> Note: The non-for-testing versions require friend'ing in thread_restrictions.h
> but care was taken to add these friends ahead of git cl split (since it wasn't
> possible to do a line-by-line associated CL split).
> Refer to the top-level CL if necessary :
> https://chromium-review.googlesource.com/c/chromium/src/+/1288533
> 
> Please CQ if LGTY!
> 
> This CL was uploaded by git cl split.
> 
> R=​wittman@chromium.org
> 
> Bug: 766678
> Change-Id: I2c45612b77a49ca5720da95093b0cdd8845b6f03
> Reviewed-on: https://chromium-review.googlesource.com/c/1325191
> Reviewed-by: Mike Wittman <wittman@chromium.org>
> Commit-Queue: Mike Wittman <wittman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#606478}

TBR=wittman@chromium.org,etiennep@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 766678
Change-Id: Iba9e69ad0923da8d6b6dcc2f89b9e323a30894d6
Reviewed-on: https://chromium-review.googlesource.com/c/1344577
Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
Reviewed-by: Etienne Pierre-Doray <etiennep@chromium.org>
Reviewed-by: Mike Wittman <wittman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#609892}
[modify] https://crrev.com/90927154fc2ebac2c77c0b43957bcbf55df40637/base/profiler/stack_sampling_profiler.cc

Comment 5 by etiennep@chromium.org, Nov 22

Status: Fixed (was: Untriaged)
Project Member

Comment 6 by bugdroid1@chromium.org, Nov 28 

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

commit 9bbc291e802a7f5ab37dd27af1af0b576d08219a
Author: Etienne Pierre-Doray <etiennep@chromium.org>
Date: Wed Nov 28 14:34:39 2018

Reland "[TaskScheduler]: Migrate off of ScopedAllowWait in /base/profiler/stack_sampling_profiler.cc"

This is a reland of 50b50793ee7891ab8ff9a4bab66c0c43391aa4ad

This CL changes ScopedAllowBaseSyncPrimitives to ScopedAllowBaseSyncPrimitivesOutsideBlockingScope
in StackSamplingProfiler.

Original change's description:
> [TaskScheduler]: Migrate off of ScopedAllowWait in /base/profiler/stack_sampling_profiler.cc
>
> base::ThreadRestrictions::ScopedAllowWait is deprecated in favor of its more
> explicit counterpart.
>
> It should have been replaced by :
>  * base::ScopedAllowBaseSyncPrimitivesForTesting in test files.
>  * base::ScopedAllowSyncPrimitives in non-test files
>  * base::ScopedAllowSyncPrimitivesOutsideBlockingScope when it's used on threads
>    that don't allow blocking
> The last one is strongly frowned upon but this CL aims to document existing
> behavior rather than address it. Owners are encouraged to follow-up by fixing
> unnecessary waits and more particularly unnecessary waits
> outside-blocking-scope.
>
> Note: The non-for-testing versions require friend'ing in thread_restrictions.h
> but care was taken to add these friends ahead of git cl split (since it wasn't
> possible to do a line-by-line associated CL split).
> Refer to the top-level CL if necessary :
> https://chromium-review.googlesource.com/c/chromium/src/+/1288533
>
> Please CQ if LGTY!
>
> This CL was uploaded by git cl split.
>
> R=wittman@chromium.org
>
> Bug: 766678
> Change-Id: I2c45612b77a49ca5720da95093b0cdd8845b6f03
> Reviewed-on: https://chromium-review.googlesource.com/c/1325191
> Reviewed-by: Mike Wittman <wittman@chromium.org>
> Commit-Queue: Mike Wittman <wittman@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#606478}

Bug: 766678,  907177 
Change-Id: I45b48e57040721e5ff3c2bdaac62237ed7ae429e
Reviewed-on: https://chromium-review.googlesource.com/c/1349633
Reviewed-by: Mike Wittman <wittman@chromium.org>
Reviewed-by: François Doray <fdoray@chromium.org>
Commit-Queue: Etienne Pierre-Doray <etiennep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611687}
[modify] https://crrev.com/9bbc291e802a7f5ab37dd27af1af0b576d08219a/base/profiler/stack_sampling_profiler.cc
[modify] https://crrev.com/9bbc291e802a7f5ab37dd27af1af0b576d08219a/base/threading/thread_restrictions.h

Comment 7 by wittman@chromium.org, Nov 28

Cc: kbr@chromium.org rjkroege@chromium.org lgrey@chromium.org
 Issue 907258  has been merged into this issue.
►

Sign in to add a comment