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

Issue 616976 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Aug 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

RenderWidgetHostViewChildFrame races with the ui::Compositor startup

Project Member Reported by danakj@chromium.org, Jun 3 2016

Issue description

The following flake occured for me. I don't think it's my local patch but would love to hear otherwise. I read this as

- The SurfaceManager::RegisterSurfaceIdNamespace has not been called.

- That method is called from ui::InProcessContextFactory::CreateSurfaceIdAllocator. In this test the callstack looks like:
#3 0x7f4104b847aa cc::SurfaceManager::RegisterSurfaceIdNamespace()
#4 0x7f4104b80145 cc::SurfaceIdAllocator::RegisterSurfaceIdNamespace()
#5 0x000003447177 ui::InProcessContextFactory::CreateSurfaceIdAllocator()
#6 0x7f40e9fbfceb ui::Compositor::Compositor()
#7 0x7f40e982d978 aura::WindowTreeHost::CreateCompositor()
#8 0x7f40e9832425 aura::WindowTreeHostX11::WindowTreeHostX11()
#9 0x7f40e9835c02 aura::WindowTreeHost::Create()
#10 0x0000035474f1 aura::TestScreen::CreateHostForPrimaryDisplay()
#11 0x00000353f962 aura::test::AuraTestHelper::SetUp()
#12 0x000002fb96b8 content::RenderViewHostTestHarness::SetUp()
#13 0x000001546990 content::RenderFrameHostManagerTest::SetUp()

- Or... SurfaceManager::InvalidateSurfaceIdNamespace has been called and then DidNavigateFrame happens. A callstack for that happening in this test is:
#3 0x7fdad8209971 cc::SurfaceManager::InvalidateSurfaceIdNamespace()
#4 0x7fdabd6472ad ui::Compositor::~Compositor()
#5 0x7fdabd647f8e ui::Compositor::~Compositor()
#6 0x7fdabceb781f aura::WindowTreeHostX11::~WindowTreeHostX11()
#7 0x7fdabceb78de aura::WindowTreeHostX11::~WindowTreeHostX11()
#8 0x00000354019d aura::test::AuraTestHelper::TearDown()
#9 0x000002fb9b4d content::RenderViewHostTestHarness::TearDown()
#10 0x000001546aae content::RenderFrameHostManagerTest::TearDown()

The failure looked like:

[ RUN      ] RenderFrameHostManagerTestWithSiteIsolation.ProxiesReceiveShouldEnforceStrictMixedContentChecking
[22626:22626:0602/180221:8563222924899:FATAL:surface_manager.cc(266)] Check failed: valid_surface_id_namespaces_.count(parent_namespace) == 1u (0 vs. 1)
#0 0x000000728651 __interceptor_backtrace
#1 0x7f9295177473 base::debug::StackTrace::StackTrace()
#2 0x7f92951e574c logging::LogMessage::~LogMessage()
#3 0x7f929880ce1a cc::SurfaceManager::RegisterSurfaceNamespaceHierarchy()
#4 0x7f9291855758 content::RenderWidgetHostViewChildFrame::SetCrossProcessFrameConnector()
#5 0x7f92917754bc content::CrossProcessFrameConnector::set_view()
#6 0x7f929182b783 content::RenderFrameHostManager::CommitPending()
#7 0x7f92918298e5 content::RenderFrameHostManager::CommitPendingIfNecessary()
#8 0x7f92918296e1 content::RenderFrameHostManager::DidNavigateFrame()
#9 0x000001543ba8 content::RenderFrameHostManagerTestWithSiteIsolation_ProxiesReceiveShouldEnforceStrictMixedContentChecking_Test::TestBody()
#10 0x00000490f9bc testing::Test::Run()
#11 0x000004911409 testing::TestInfo::Run()
#12 0x000004912683 testing::TestCase::Run()
#13 0x000004925096 testing::internal::UnitTestImpl::RunAllTests()
#14 0x0000049246da testing::UnitTest::Run()
#15 0x00000306f70f base::TestSuite::Run()
#16 0x00000309dd7f base::(anonymous namespace)::LaunchUnitTestsInternal()
#17 0x00000309d9ea base::LaunchUnitTests()
#18 0x000002b3a28f main
#19 0x7f9278653f45 __libc_start_main
#20 0x0000006e9a29 <unknown>


Do you think this is something I did?
 
This test fails with 100% reproduciability for me. It is also failing on the builders (see for example: https://chromium-swarm.appspot.com/user/task/2f64dd25ce8a6710).

I found that if I ran the test by itself, it would pass everytime, and if I ran the test with any of the other RenderFrameHostManagerTestWithSiteIsolation.* tests, it would fail everytime. 

The bots don't show it as failing because it initally fails with the other tests, but since it is the only test in the suite that fails, it gets re-run and then passes when it is run in isolation.

The issue is that these tests are sharing the global state in the content/test/test_render_view_host.cc (https://cs.chromium.org/chromium/src/content/test/test_render_view_host.cc?q=test_render_view_host.cc&sq=package:chromium&dr&l=32)

And the valid_surface_id_namespace_ is looking for parent (in my case usually id 3 or 4), when the only valid surface id is id 1.

I found that making a static TestRenderWidgetHostView::ResetSurfaceIdNamespace() method that reset the global state and was called in RenderFrameHostManagerTestWithSiteIsolation::SetUp did the trick for me, but I think this is likely a race condition, so I am hoping someone more familiar with this test/code can come up with a correct solution

Cc: nick@chromium.org alex...@chromium.org
Project Member

Comment 3 by bugdroid1@chromium.org, Jun 20 2016

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

commit d31c7828f22b988a63718fd9dd8735c7220aba57
Author: enne <enne@chromium.org>
Date: Mon Jun 20 22:38:15 2016

Create only one ContextFactory in content unit tests

ImageTransportFactory::InitializeForUnitTests gets called in the setup
of many tests.  However, ui::InitializeContextFactoryForTests via
RenderViewHostTestHarness::SetUp was also being called indirectly in
many unit tests as well.  This would result in two context factories,
one set on ui::aura::Env and the other being the ImageTransportFactory
instance.

Fix this problem by consolidating these two in content unittests.

This fix is in the service of fixing racy RenderWidgetHostViewChildFrame
tests that were using fake surface namespace ids that were never
registered.  That bug is fixed by registering that id with the (single)
global SurfaceManager on the context factory.

BUG= 616976 
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

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

[modify] https://crrev.com/d31c7828f22b988a63718fd9dd8735c7220aba57/content/browser/compositor/surface_utils.h
[modify] https://crrev.com/d31c7828f22b988a63718fd9dd8735c7220aba57/content/browser/frame_host/navigator_impl_unittest.cc
[modify] https://crrev.com/d31c7828f22b988a63718fd9dd8735c7220aba57/content/browser/frame_host/render_frame_host_manager_unittest.cc
[modify] https://crrev.com/d31c7828f22b988a63718fd9dd8735c7220aba57/content/browser/renderer_host/render_widget_host_view_aura_unittest.cc
[modify] https://crrev.com/d31c7828f22b988a63718fd9dd8735c7220aba57/content/browser/renderer_host/render_widget_host_view_mac_unittest.mm
[modify] https://crrev.com/d31c7828f22b988a63718fd9dd8735c7220aba57/content/public/test/test_renderer_host.cc
[modify] https://crrev.com/d31c7828f22b988a63718fd9dd8735c7220aba57/content/test/test_render_view_host.cc
[modify] https://crrev.com/d31c7828f22b988a63718fd9dd8735c7220aba57/content/test/test_render_view_host.h

Comment 4 by enne@chromium.org, Aug 30 2016

Status: Fixed (was: Assigned)

Sign in to add a comment