New issue
Advanced search Search tips

Issue 885329 link

Starred by 1 user

Issue metadata

Status: Assigned
Owner:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

DisplayLinkMac::GetAllDisplayLinks is being called on the wrong thread?

Project Member Reported by ellyjo...@chromium.org, Sep 18

Issue description

This function does the following:

DisplayLinkMac::DisplayLinkMap& DisplayLinkMac::GetAllDisplayLinks() {
  DCHECK_EQ(base::ThreadTaskRunnerHandle::Get(), GetMainThreadTaskRunner());
  static base::NoDestructor<DisplayLinkMac::DisplayLinkMap> all_display_links;
  return *all_display_links;
}

and then there's:

scoped_refptr<base::SingleThreadTaskRunner>
DisplayLinkMac::GetMainThreadTaskRunner() {
  static scoped_refptr<base::SingleThreadTaskRunner> task_runner =
      base::ThreadTaskRunnerHandle::Get();
  return task_runner;
}

sometimes when running the full suite the DCHECK_EQ in GetAllDisplayLinks will fail. Unfortunately thread handles are fairly opaque :(.
 
(lldb) bt
* thread #1, queue = 'com.apple.main-thread', stop reason = EXC_BREAKPOINT (code=EXC_I386_BPT, subcode=0x0)
    frame #0: 0x000000010ef1d921 libbase.dylib`base::debug::BreakDebugger() at debugger_posix.cc:272 [opt]
    frame #1: 0x000000010ee1aa95 libbase.dylib`logging::LogMessage::~LogMessage(this=0x00007ffeefbfe300) at logging.cc:856 [opt]
  * frame #2: 0x0000000116c5dc6a libaccelerated_widget_mac.dylib`ui::DisplayLinkMac::GetAllDisplayLinks() at display_link_mac.cc:197 [opt]
    frame #3: 0x0000000116c5d864 libaccelerated_widget_mac.dylib`ui::DisplayLinkMac::GetForDisplay(display_id=<unavailable>) at display_link_mac.cc:44 [opt]
    frame #4: 0x000000010eb98546 libviews.dylib`views::BridgedNativeWidgetHostImpl::OnWindowDisplayChanged(this=0x000000011f241ed0, new_display=0x00007ffeefbfe730) at bridged_native_widget_host_impl.mm:652 [opt]
    frame #5: 0x000000010eae5681 libviews.dylib`views::BridgedNativeWidgetImpl::InitCompositorView() [inlined] views::BridgedNativeWidgetImpl::UpdateWindowDisplay(this=<unavailable>) at bridged_native_widget.mm:1277 [opt]
    frame #6: 0x000000010eae5652 libviews.dylib`views::BridgedNativeWidgetImpl::InitCompositorView(this=0x000000011f2dad30) at bridged_native_widget.mm:916 [opt]
    frame #7: 0x000000010eb96b1a libviews.dylib`views::BridgedNativeWidgetHostImpl::CreateCompositor(this=0x000000011f241ed0, params=<unavailable>) at bridged_native_widget_host_impl.mm:272 [opt]
    frame #8: 0x000000010eb76304 libviews.dylib`views::NativeWidgetMac::InitNativeWidget(this=0x000000011f2b0290, params=0x00007ffeefbfeb88) at native_widget_mac.mm:158 [opt]
    frame #9: 0x000000010eb7d62a libviews.dylib`views::Widget::Init(this=0x000000011f2b3130, in_params=<unavailable>) at widget.cc:342 [opt]
    frame #10: 0x0000000101ec0cb9 unit_tests`OmniboxViewViewsTest::SetUp(this=0x000000011f844000) at omnibox_view_views_unittest.cc:280 [opt]
    frame #11: 0x00000001020d33cd unit_tests`testing::Test::Run() [inlined] void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(object=<unavailable>, method=11 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)(), char const*) at gtest.cc:0 [opt]
    frame #12: 0x00000001020d33bf unit_tests`testing::Test::Run(this=0x000000011f844000) at gtest.cc:2518 [opt]
    frame #13: 0x00000001020d4050 unit_tests`testing::TestInfo::Run(this=0x0000000121cd5900) at gtest.cc:2698 [opt]
    frame #14: 0x00000001020d45c7 unit_tests`testing::TestCase::Run(this=0x0000000121cd57f0) at gtest.cc:2816 [opt]
    frame #15: 0x00000001020e0557 unit_tests`testing::internal::UnitTestImpl::RunAllTests(this=0x000000011f601bc0) at gtest.cc:5182 [opt]
    frame #16: 0x00000001020e01cd unit_tests`testing::UnitTest::Run() [inlined] bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(object=0x000000011f601bc0, method=00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00)(), char const*) at gtest.cc:0 [opt]
    frame #17: 0x00000001020e01c0 unit_tests`testing::UnitTest::Run(this=<unavailable>) at gtest.cc:4791 [opt]
    frame #18: 0x0000000102844a76 unit_tests`base::TestSuite::Run() [inlined] RUN_ALL_TESTS() at gtest.h:2333 [opt]
    frame #19: 0x0000000102844a69 unit_tests`base::TestSuite::Run(this=0x0000000121d11690) at test_suite.cc:295 [opt]
    frame #20: 0x000000010285443e unit_tests`base::(anonymous namespace)::LaunchUnitTestsInternal(base::OnceCallback<int ()>, unsigned long, int, bool, base::OnceCallback<void ()>) [inlined] base::OnceCallback<int ()>::Run(this=<unavailable>) && at callback.h:99 [opt]
    frame #21: 0x00000001028543dc unit_tests`base::(anonymous namespace)::LaunchUnitTestsInternal(run_test_suite=base::RunTestSuiteCallback @ 0x00007ffeefbff720, parallel_jobs=1, default_batch_limit=10, use_job_objects=true, gtest_init=base::OnceClosure @ 0x00007ffeefbff718)>, unsigned long, int, bool, base::OnceCallback<void ()>) at unit_test_launcher.cc:225 [opt]
    frame #22: 0x0000000102854291 unit_tests`base::LaunchUnitTests(argc=2, argv=<unavailable>, run_test_suite=<unavailable>)>) at unit_test_launcher.cc:576 [opt]
    frame #23: 0x000000010283543b unit_tests`main(argc=2, argv=0x00007ffeefbff980) at run_all_unittests.cc:30 [opt]
    frame #24: 0x00007fff50e48015 libdyld.dylib`start + 1

Owner: ccameron@chromium.org
Status: Assigned (was: Untriaged)
I figured this one out: the static variable in DisplayLinkMac::GetMainThreadTaskRunner() is not reset between tests, so the first test in a suite runs properly, and then in the second test that static is already initialized - C++ has no way to know it should be reinitialized. That leads to a DCHECK because ThreadTaskRunnerHandle's state *is* reset. That reset is accomplished via ~ViewsTestBase, which calls ~ScopedTaskEnvironment, which destroys message_loop_.

I guess we should probably just delete this DCHECK, or have it DCHECK instead that we're on the main thread by PlatformThreadRef.

Over to ccameron@ :)
Project Member

Comment 3 by bugdroid1@chromium.org, Sep 25

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

commit 61e34c2a024cb9849ac2112c233b44fe4ed384e0
Author: Christopher Cameron <ccameron@chromium.org>
Date: Tue Sep 25 22:37:15 2018

Replace DCHECK in DisplayLinkMac with comments

Move some of the static methods in DisplayLinkMac to be anonymous
namespace methods in the cc file, and add comments clarifying their
usage.

Remove the DCHECK ensuring that the initially-captured main thread task
runner be equal to the current main thread task runner, and add a
comment indicating that this may not always be the case.

Bug: 885329
Change-Id: Idfdeb3ba1d20840161f26669e19bdee42aa155c5
Reviewed-on: https://chromium-review.googlesource.com/1234229
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#594129}
[modify] https://crrev.com/61e34c2a024cb9849ac2112c233b44fe4ed384e0/ui/accelerated_widget_mac/display_link_mac.cc
[modify] https://crrev.com/61e34c2a024cb9849ac2112c233b44fe4ed384e0/ui/accelerated_widget_mac/display_link_mac.h

Sign in to add a comment