Issue metadata
Sign in to add a comment
|
DCHECK(MainFrameImpl()) hit on a remote-to-local main frame navigation when accessibility is enabled |
||||||||||||||||||||||||
Issue descriptionWhat steps will reproduce the problem? (1) Start a debug build of chrome with --enable-browser-side-navigation --site-per-process --force-renderer-accessibility (2) Go to http://csreis.github.io/tests/window-open.html (3) Click "Open simple window" (4) Navigate new tab to https://csreis.github.io (5) Go back to first tab and also navigate it to https://csreis.github.io (6) Observe DCHECK: [1:1:0906/223400.627205:FATAL:WebViewImpl.cpp(3442)] Check failed: MainFrameImpl(). #0 0x7f630e97f88d base::debug::StackTrace::StackTrace() #1 0x7f630e97dc5c base::debug::StackTrace::StackTrace() #2 0x7f630ea0eb2a logging::LogMessage::~LogMessage() #3 0x7f62fcfeb5c3 blink::WebViewImpl::ResizeAfterLayout() #4 0x7f62fda10e29 blink::ChromeClientImpl::ResizeAfterLayout() #5 0x7f62fd811f7b blink::LayoutView::UpdateAfterLayout() #6 0x7f62fd6a9307 blink::LayoutBlockFlow::UpdateBlockLayout() #7 0x7f62fd69115c blink::LayoutBlock::UpdateLayout() #8 0x7f62fd80eb82 blink::LayoutView::LayoutContent() #9 0x7f62fd80f75a blink::LayoutView::UpdateLayout() #10 0x7f62fd06ec13 blink::LocalFrameView::PerformLayout() #11 0x7f62fd06b3ae blink::LocalFrameView::UpdateLayout() #12 0x7f62fd07bc87 blink::LocalFrameView::UpdateStyleAndLayoutIfNeededRecursiveInternal() #13 0x7f62fd07968e blink::LocalFrameView::UpdateStyleAndLayoutIfNeededRecursive() #14 0x7f62fd0781a9 blink::LocalFrameView::UpdateLifecyclePhasesInternal() #15 0x7f62fd078a2a blink::LocalFrameView::UpdateLifecycleToCompositingCleanPlusScrolling() #16 0x7f62fa23b33a blink::WebAXObject::UpdateLayoutAndCheckValidity() #17 0x7f6309578f8d content::RenderAccessibilityImpl::SendPendingAccessibilityEvents() #18 0x7f6306fdda4f _ZN4base8internal13FunctorTraitsIMN7content13URLLoaderImplEFvvEvE6InvokeINS_7WeakPtrIS3_EEJEEEvS5_OT_DpOT0_ #19 0x7f630958606a _ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRKMN7content23RenderAccessibilityImplEFvvERKNS_7WeakPtrIS5_EEJEEEvOT_OT0_DpOT1_ #20 0x7f6309586000 _ZN4base8internal7InvokerINS0_9BindStateIMN7content23RenderAccessibilityImplEFvvEJNS_7WeakPtrIS4_EEEEEFvvEE7RunImplIRKS6_RKNSt3__15tupleIJS8_EEEJLm0EEEEvOT_OT0_NSF_16integer_sequenceImJXspT1_EEEE #21 0x7f6309585f4c _ZN4base8internal7InvokerINS0_9BindStateIMN7content23RenderAccessibilityImplEFvvEJNS_7WeakPtrIS4_EEEEEFvvEE3RunEPNS0_13BindStateBaseE #22 0x7f630e92ade1 _ZNO4base12OnceCallbackIFvvEE3RunEv #23 0x7f630e984585 base::debug::TaskAnnotator::RunTask() #24 0x7f62fb665682 blink::scheduler::TaskQueueManager::ProcessTaskFromWorkQueue() #25 0x7f62fb66087a blink::scheduler::TaskQueueManager::DoWork() #26 0x7f62fb66cb87 _ZN4base8internal13FunctorTraitsIMN5blink9scheduler16TaskQueueManagerEFvbEvE6InvokeIRKNS_7WeakPtrIS4_EEJRKbEEEvS6_OT_DpOT0_ #27 0x7f62fb66cae5 _ZN4base8internal12InvokeHelperILb1EvE8MakeItSoIRKMN5blink9scheduler16TaskQueueManagerEFvbERKNS_7WeakPtrIS6_EEJRKbEEEvOT_OT0_DpOT1_ #28 0x7f62fb66ca5d _ZN4base8internal7InvokerINS0_9BindStateIMN5blink9scheduler16TaskQueueManagerEFvbEJNS_7WeakPtrIS5_EEbEEEFvvEE7RunImplIRKS7_RKNSt3__15tupleIJS9_bEEEJLm0ELm1EEEEvOT_OT0_NSG_16integer_sequenceImJXspT1_EEEE #29 0x7f62fb66c96c _ZN4base8internal7InvokerINS0_9BindStateIMN5blink9scheduler16TaskQueueManagerEFvbEJNS_7WeakPtrIS5_EEbEEEFvvEE3RunEPNS0_13BindStateBaseE #30 0x7f630e92ade1 _ZNO4base12OnceCallbackIFvvEE3RunEv #31 0x7f630e984585 base::debug::TaskAnnotator::RunTask() #32 0x7f630ea333ff base::internal::IncomingTaskQueue::RunTask() #33 0x7f630ea38774 base::MessageLoop::RunTask() #34 0x7f630ea389f7 base::MessageLoop::DeferOrRunPendingTask() #35 0x7f630ea396e4 base::MessageLoop::DoWork() #36 0x7f630ea4003a base::MessagePumpDefault::Run() #37 0x7f630ea37f34 base::MessageLoop::Run() #38 0x7f630eaeeb8d base::RunLoop::Run() #39 0x7f6309847fcb content::RendererMain() #40 0x7f6309d1793c content::RunZygote() #41 0x7f6309d18609 content::RunNamedProcessTypeMain() #42 0x7f6309d1b16e content::ContentMainRunnerImpl::Run() #43 0x7f6309d15c3d content::ContentServiceManagerMainDelegate::RunEmbedderProcess() #44 0x7f630f2828b5 service_manager::Main() #45 0x7f6309d172df content::ContentMain() #46 0x55b8e59672de ChromeMain #47 0x55b8e59671f2 main #48 0x7f62f5841f45 __libc_start_main #49 0x55b8e59670d4 <unknown> The DCHECK fires because even though LayoutView::UpdateAfterLayout checked that this is only done for a main frame, the main frame is provisional and not swapped into the tree yet (that will happen on commit). So, when WebViewImpl::ResizeAfterLayout tries to DCHECK MainFrameImpl(), this returns null because it tries to get the local main frame from Page, and Page doesn't know about the pending navigation yet and its main frame still pointing at a RemoteFrame. Basically, any main frame navigation that follows the provisional (remote-to-local) path will hit this. I hit this in an Android telemetry_perf_unittest (benchmarks.system_health_smoke_test.SystemHealthBenchmarkSmokeTest.system_health.memory_mobile.browse:chrome:omnibox) on https://chromium-review.googlesource.com/c/chromium/src/+/636786/), where this failed because the test happened to enable accessibility and then do a cross-site navigation, which followed the remote-to-local path due to changes in that CL. That CL will make this scenario a lot more common. Here's how we get to SendPendingAccessibilityEvents. After creating the main frame's RFH but before navigating, in WCI::RenderFrameCreated, we call UpdateAccessibilityMode which sends FrameMsg_SetAccessibilityMode. RenderFrameImpl handles this, sees that AX is enabled, and triggers creation of RenderAccessibilityImpl. (At this point it's still sitting at the initial blank document.) RenderAccessibilityImpl's constructor calls: HandleAXEvent(WebAXObject::FromWebDocument(document), ui::AX_EVENT_LAYOUT_COMPLETE); which is what triggers the SendPendingAccessibilityEvents. It seems weird to get into layout and resize on the provisional frame before it commits anything (it's hidden and not swapped into the frame tree at this point). A few potential approaches to fix this: 1) Early return for provisional frames in WebViewImpl::ResizeAfterLayout and WebViewImpl::LayoutUpdated? Or earlier before starting layout at all? (The frame should do it properly if/when it commits) 2) Fix this path by plumbing the frame into WebViewImpl::ResizeAfterLayout() from LayoutView::UpdateAfterLayout(), so that WebViewImpl::ResizeAfterLayout doesn't have to look it up from Page. (ResizeAfterLayout needs the main frame for the should_auto_resize_ path.) Same for WebViewImpl::LayoutUpdated(). I'm not sure this is correct to do if the provisional frame never commits: it doesn't seem like we should be applying sizing changes to WebViewImpl before commit, but maybe it doesn't matter since the WebViewImpl with the current main frame is in another process. 3) Avoid doing a SendPendingAccessibilityEvents until the new frame commits. I'm not sure why the AX_EVENT_LAYOUT_COMPLETE is generated; there's a comment there that I don't really understand: // It's possible that the webview has already loaded a webpage without // accessibility being enabled. Initialize the browser's cached // accessibility tree by sending it a notification. Interestingly, in release builds things seem to work in practice. E.g., for the test where I hit this the should_auto_resize_ path wasn't followed and the rest of the work in ResizeAfterLayout() is independent of the main frame. dmazzoni@ and dcheng@: any thoughts?
,
Sep 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/430766263a0619edb0770affead7a675e7275a94 commit 430766263a0619edb0770affead7a675e7275a94 Author: Alex Moshchuk <alexmos@chromium.org> Date: Fri Sep 08 21:20:44 2017 Don't send accessibility events for provisional frames. When accessibility is enabled, a RenderFrame created for a remote-to-local navigation will try to dispatch an accessibility event (AX_EVENT_LAYOUT_COMPLETE) before it navigates/commits. This leads to layout being performed on the provisional frame which is still not in the tree, which, for main frames, hits a DCHECK in WebViewImpl::ResizeAfterLayout when it to find a local main frame from Page, because Page won't know about this frame until commit. To fix this, avoid sending AX events for these provisional frames until they commit, at which point they should trigger layout and dispatch them. Bug: 762824 Change-Id: Ic71c36e8c7a54c74ccc4009c8cbd796ded45b52d Reviewed-on: https://chromium-review.googlesource.com/656398 Commit-Queue: Alex Moshchuk <alexmos@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org> Cr-Commit-Position: refs/heads/master@{#500677} [modify] https://crrev.com/430766263a0619edb0770affead7a675e7275a94/content/browser/accessibility/site_per_process_accessibility_browsertest.cc [modify] https://crrev.com/430766263a0619edb0770affead7a675e7275a94/content/renderer/accessibility/render_accessibility_impl.cc [modify] https://crrev.com/430766263a0619edb0770affead7a675e7275a94/content/renderer/render_frame_impl.cc [modify] https://crrev.com/430766263a0619edb0770affead7a675e7275a94/content/renderer/render_frame_impl.h
,
Sep 8 2017
|
|||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||
Comment 1 by dmazz...@chromium.org
, Sep 7 2017