New issue
Advanced search Search tips

Issue 712429 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Apr 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

ProcessManagerTest.CreateBackgroundHostsDeferred unit test is flaky

Project Member Reported by kmarshall@chromium.org, Apr 17 2017

Issue description

The extensions unittest ProcessManagerTest.CreateBackgroundHostsDeferred seems to be flaky. It passes on the first retry.

The initial failure spews this check failure:

[ RUN      ] ProcessManagerTest.CreateBackgroundHostsDeferred
[12699:12699:0417/162628.519873:455867750224:FATAL:observer_list.h(276)] Check failed: false. Observers can only be added once!
#0 0x7fd826f4728b base::debug::StackTrace::StackTrace()
#1 0x7fd826f45f8c base::debug::StackTrace::StackTrace()
#2 0x7fd826fb8e4f logging::LogMessage::~LogMessage()
#3 0x7fd828d352de base::ObserverListBase<>::AddObserver()
#4 0x7fd828d330ae content::DevToolsAgentHost::AddObserver()
#5 0x00000115ae27 extensions::ProcessManager::ProcessManager()
#6 0x00000115a792 extensions::ProcessManager::CreateForTesting()
#7 0x00000070d877 extensions::ProcessManagerTest_CreateBackgroundHostsDeferred_Test::TestBody()
#8 0x000001a34d6e testing::internal::HandleSehExceptionsInMethodIfSupported<>()
#9 0x000001a274c2 testing::internal::HandleExceptionsInMethodIfSupported<>()
#10 0x000001a1bfe6 testing::Test::Run()
#11 0x000001a1c79d testing::TestInfo::Run()
#12 0x000001a1cd3f testing::TestCase::Run()
#13 0x000001a220ac testing::internal::UnitTestImpl::RunAllTests()
#14 0x000001a38c8e testing::internal::HandleSehExceptionsInMethodIfSupported<>()
#15 0x000001a28c32 testing::internal::HandleExceptionsInMethodIfSupported<>()
#16 0x000001a21d4f testing::UnitTest::Run()
#17 0x00000186e7f1 RUN_ALL_TESTS()
#18 0x00000186d692 base::TestSuite::Run()
#19 0x0000018bd37d content::UnitTestTestSuite::Run()
#20 0x0000005680b5 _ZN4base8internal13FunctorTraitsIMN7content17UnitTestTestSuiteEFivEvE6InvokeIPS3_JEEEiS5_OT_DpOT0_
#21 0x000000567fd1 _ZN4base8internal12InvokeHelperILb0EiE8MakeItSoIRKMN7content17UnitTestTestSuiteEFivEJPS5_EEEiOT_DpOT0_
#22 0x000000567f77 _ZN4base8internal7InvokerINS0_9BindStateIMN7content17UnitTestTestSuiteEFivEJNS0_17UnretainedWrapperIS4_EEEEEFivEE7RunImplIRKS6_RKSt5tupleIJS8_EEJLm0EEEEiOT_OT0_NS_13IndexSequenceIJXspT1_EEEE
#23 0x000000567e8c _ZN4base8internal7InvokerINS0_9BindStateIMN7content17UnitTestTestSuiteEFivEJNS0_17UnretainedWrapperIS4_EEEEEFivEE3RunEPNS0_13BindStateBaseE
#24 0x000000641abd _ZNKR4base8CallbackIFvvELNS_8internal8CopyModeE1ELNS2_10RepeatModeE1EE3RunEv
#25 0x0000018711d6 base::(anonymous namespace)::LaunchUnitTestsInternal()
#26 0x000001871044 base::LaunchUnitTests()
#27 0x000000565fca main
#28 0x7fd817165f45 __libc_start_main
#29 0x000000565e64 <unknown>

 
Cc: jamescook@chromium.org
Owner: rdevlin....@chromium.org
This should probably be owned by one of the extensions owners. I touched this code, but long ago to refactor things.

Devlin, can you suggest an owner?

Components: Platform>Extensions
Cc: rdevlin....@chromium.org
Owner: lazyboy@chromium.org
Status: Started (was: Untriaged)
Project Member

Comment 4 by bugdroid1@chromium.org, Apr 25 2017

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

commit f70b2eb9fb89130c5d8c9bdc91fc98b7e07964ad
Author: lazyboy <lazyboy@chromium.org>
Date: Tue Apr 25 20:42:28 2017

Fix ProcessManagerTest.* flakiness.

Remove ProcessManager as DevToolsAgentHost observer during the manager's
destruction.
In ProcessManagerTest-s, we create multiple ProcessManager-s
and add as DevToolsAgentHost observer. If two process managers were
created at the same address, then DevToolsAgentHost will think that we
are adding the same ProcessManager as observer twice.
Remove ProcessManager as DevToolsAgentHost observer during the manager's
destruction to avoid that. Also that is the right thing to do to avoid
potential UAF of ProcessManager from DevToolsAgentHost.

BUG= 712429 
Test=Running
./out/Release/extensions_unittests --gtest_filter=ProcessManagerTest.*
should pass without any retry.

Review-Url: https://codereview.chromium.org/2838223002
Cr-Commit-Position: refs/heads/master@{#467104}

[modify] https://crrev.com/f70b2eb9fb89130c5d8c9bdc91fc98b7e07964ad/extensions/browser/process_manager.cc

Status: Fixed (was: Started)

Sign in to add a comment