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

Issue 804508 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Jan 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 2
Type: Bug



Sign in to add a comment

mash: DCHECK in content::CrossProcessFrameConnector::SetView() cc_param_traits.cc p.is_valid()

Project Member Reported by rjkroege@chromium.org, Jan 22 2018

Issue description

Run Chrome build with DCHECKs on and run on device with the --mash flag as of ToT refs/heads/master@{#530869}

This happens because of a failed DCHECK. However fsamuel@ informed me offline that this DCHECK is detecting a race condition in mus that would otherwise lead to flaky incorrect behaviour.

fsamuel@ maybe you could expand on this?

[5339:5339:0122/142316.403683:FATAL:cc_param_traits.cc(600)] Check failed: p.is_valid(). 
#0 0x5b964dcbd17c base::debug::StackTrace::StackTrace()
#1 0x5b964dcdbd91 logging::LogMessage::~LogMessage()
#2 0x5b964f595011 IPC::ParamTraits<>::Write()
#3 0x5b964bcfc796 content::CrossProcessFrameConnector::SetView()
#4 0x5b964bd53cb2 content::RenderFrameHostManager::SetRWHViewForInnerContents()
#5 0x5b964c0a6ca1 content::WebContentsImpl::ReattachToOuterWebContentsFrame()
#6 0x5b964c0a6b70 content::WebContentsImpl::AttachToOuterWebContentsFrame()
#7 0x5b964feea50b _ZN4base8internal7InvokerINS0_9BindStateIZN10guest_view22GuestViewMessageFilter23OnAttachToEmbedderFrameEiiiRKNS_15DictionaryValueEE3$_0JPN7content11WebContentsESB_PNS9_15RenderFrameHostEiEEEFvvEE7RunOnceEPNS
0_13BindStateBaseE
#8 0x5b964fee5551 guest_view::GuestViewBase::WillAttach()
#9 0x5b964fee9efa guest_view::GuestViewMessageFilter::OnAttachToEmbedderFrame()
#10 0x5b964fee9b8d _ZN3IPC8MessageTI43GuestViewHostMsg_AttachToEmbedderFrame_MetaNSt3__15tupleIJiiiN4base15DictionaryValueEEEEvE8DispatchIN10guest_view22GuestViewMessageFilterESA_vMSA_FviiiRKS5_EEEbPKNS_7MessageEPT_PT0_PT1_T2_
#11 0x5b964fee97d5 guest_view::GuestViewMessageFilter::OnMessageReceived()
#12 0x5b964c3517f3 extensions::ExtensionsGuestViewMessageFilter::OnMessageReceived()
#13 0x5b964ba9b619 content::BrowserMessageFilter::Internal::DispatchMessage()
#14 0x5b964ba9b748 _ZN4base8internal7InvokerINS0_9BindStateINS0_18IgnoreResultHelperIMN7content20BrowserMessageFilter8InternalEFbRKN3IPC7MessageEEEEJ13scoped_refptrIS6_ES8_EEEFvvEE7RunOnceEPNS0_13BindStateBaseE
#15 0x5b964dcbe098 base::debug::TaskAnnotator::RunTask()
#16 0x5b964dd872e6 base::internal::IncomingTaskQueue::RunTask()
#17 0x5b964dce32bd base::MessageLoop::RunTask()
#18 0x5b964dce36c4 base::MessageLoop::DeferOrRunPendingTask()
#19 0x5b964dce3982 base::MessageLoop::DoWork()
#20 0x5b964dce5d29 base::MessagePumpLibevent::Run()
#21 0x5b964dce2b2c base::MessageLoop::Run()
#22 0x5b964dd11b06 base::RunLoop::Run()
#23 0x5b964d8dc327 ChromeBrowserMainParts::MainMessageLoopRun()
#24 0x5b964bbade04 content::BrowserMainLoop::RunMainMessageLoopParts()
#25 0x5b964bbb1403 content::BrowserMainRunnerImpl::Run()
#26 0x5b964bba984e content::BrowserMain()
#27 0x5b964d8c2b44 content::RunNamedProcessTypeMain()
#28 0x5b964d8c3ad8 content::ContentMainRunnerImpl::Run()
#29 0x5b964d8d0109 service_manager::Main()
#30 0x5b964d8c1fa1 content::ContentMain()
#31 0x5b964b0f2be7 ChromeMain
#32 0x73a5efd94736 __libc_start_main
#33 0x5b964b0f2839 _start

 

Comment 1 by fsamuel@google.com, Jan 22 2018

Yea, in mus, the FrameSinkId comes from the child, not from the browser and is then propagated to the browser on UpdateResizeParams. We seem to be getting into a state where the RWHV changes and the new one doesn't have a FrameSinkId yet. We need to skip ViewChanged IPC in that case. I'm not sure if we need to send that IPC or not after we find out about the FrameSinkId...

Comment 2 by sky@chromium.org, Jan 23 2018

Cc: -fsam...@chromium.org sky@chromium.org
Owner: fsam...@chromium.org
I'm not going to be able to get to this for a couple of days, and given this breaks a login test it would be good to resolve soon. Fady, any chance you can poke at it? If not, pass it back to me.
Summary: mash: DCHECK in content::CrossProcessFrameConnector::SetView() cc_param_traits.cc p.is_valid() (was: --mash DCHECK triggering on device)
Something similar reproduces on the mojo FYI bots in browser_tests, so there might be a repro on linux-chromeos:

https://ci.chromium.org/buildbot/chromium.fyi/Mojo%20ChromiumOS/
https://ci.chromium.org/buildbot/chromium.fyi/Mojo%20ChromiumOS/26252

It's been broken for a week, but if you go back you might be able to find a regression range.

[30587:30587:0123/092351.693233:FATAL:cc_param_traits.cc(600)] Check failed: p.is_valid(). 
#0 0x000003e8080c base::debug::StackTrace::StackTrace()
#1 0x000003e9ae06 logging::LogMessage::~LogMessage()
#2 0x000005f1a414 IPC::ParamTraits<>::Write()
#3 0x000002822b16 content::CrossProcessFrameConnector::SetView()
#4 0x00000286ce0a content::RenderFrameHostManager::CommitPending()
#5 0x00000286c48c content::RenderFrameHostManager::CommitPendingIfNecessary()
#6 0x00000286c36e content::RenderFrameHostManager::DidNavigateFrame()
#7 0x00000284a855 content::NavigatorImpl::DidNavigate()
#8 0x00000285b604 content::RenderFrameHostImpl::DidCommitProvisionalLoad()
#9 0x000001f0d53a content::mojom::FrameHostStubDispatch::Accept()
#10 0x0000052237eb mojo::InterfaceEndpointClient::HandleValidatedMessage()
#11 0x00000523ac66 mojo::FilterChain::Accept()

Retitling to make this more searchable.

Cc: fsam...@chromium.org jamescook@chromium.org sadrul@chromium.org
Owner: rjkroege@chromium.org
Status: Assigned (was: Untriaged)
Blocking: -804052
Labels: -Pri-2 Pri-3
Owner: sky@chromium.org
This particular DCHECK probably indicates an incorrect use of the compositor that would cause flaky behaviour. I fixed the login screen issue a different way so this isn't blocking so back to sky@ for triage.


Labels: -Pri-3 Pri-2
This is causing hundreds of mash_browser_tests crashes on the mojo FYI bot, so I think it's more than a P3.

https://ci.chromium.org/buildbot/chromium.fyi/Mojo%20ChromiumOS/26294

I would like to switch the main chromium waterfall to use the mojo FYI bot's browser_test blacklist, but we can't do that until this is fixed.

Comment 7 by sky@chromium.org, Jan 25 2018

Status: Started (was: Assigned)
This was caused by Jon's change here: https://chromium-review.googlesource.com/c/chromium/src/+/839782 . In mus we effectively ignore the FrameSinkId in FrameMsg_ViewChanged (see https://chromium.googlesource.com/chromium/src/+/master/content/renderer/render_frame_proxy.cc#449 ). I'm inclined to make the FrameSinkId param in FrameMsg_ViewChanged  optional.
Project Member

Comment 9 by bugdroid1@chromium.org, Jan 27 2018

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

commit 0f6bb0905776c57284646f20b188fdc1d03beb1a
Author: Scott Violet <sky@chromium.org>
Date: Sat Jan 27 19:43:17 2018

change FrameMsg_ViewChanged to take a struct

the viz::FrameSinkId is not used when mus is hosting viz (the
current code DCHECKs in mus as we end up supplying an invalid
FrameSinkId, which triggers a DCHECK). To address this address a
struct with param traits to ensure the FrameSinkId is set when
appropriate.

BUG= 804508 
TEST=covered by tests

Change-Id: I1949d546d7aa7db39dc0ad2116694b614b3b875f
Reviewed-on: https://chromium-review.googlesource.com/887619
Commit-Queue: Scott Violet <sky@chromium.org>
Reviewed-by: Jonathan Ross <jonross@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Cr-Commit-Position: refs/heads/master@{#532245}
[modify] https://crrev.com/0f6bb0905776c57284646f20b188fdc1d03beb1a/content/browser/frame_host/cross_process_frame_connector.cc
[modify] https://crrev.com/0f6bb0905776c57284646f20b188fdc1d03beb1a/content/common/BUILD.gn
[modify] https://crrev.com/0f6bb0905776c57284646f20b188fdc1d03beb1a/content/common/content_param_traits.cc
[modify] https://crrev.com/0f6bb0905776c57284646f20b188fdc1d03beb1a/content/common/content_param_traits.h
[add] https://crrev.com/0f6bb0905776c57284646f20b188fdc1d03beb1a/content/common/frame_message_structs.cc
[add] https://crrev.com/0f6bb0905776c57284646f20b188fdc1d03beb1a/content/common/frame_message_structs.h
[modify] https://crrev.com/0f6bb0905776c57284646f20b188fdc1d03beb1a/content/common/frame_messages.h
[modify] https://crrev.com/0f6bb0905776c57284646f20b188fdc1d03beb1a/content/renderer/render_frame_proxy.cc
[modify] https://crrev.com/0f6bb0905776c57284646f20b188fdc1d03beb1a/content/renderer/render_frame_proxy.h

Comment 10 by sky@chromium.org, Jan 27 2018

Status: Fixed (was: Started)
Cc: msi...@igalia.com toniki...@chromium.org
Components: -Internals>MUS Internals>Services>WindowService

Sign in to add a comment