FrameView::updateViewportIntersectionsForSubtree() may access dirty layout |
|||||||||
Issue descriptionIn FrameView::updateLifecyclePhasesInternal(), there is a call to updateViewportIntersectionsForSubtree before updateStyleAndLayoutIfNeededRecursive(), then during geometry calculation, we may access dirty layout. For example, this bug causes failure of https://codereview.chromium.org/2636153002/. The stack trace is: [1:1:0118/021916.397150:FATAL:LayoutTable.h(256)] Check failed: !needsSectionRecalc(). #0 0x7fb909c60b0e base::debug::StackTrace::StackTrace() #1 0x7fb909c79eab logging::LogMessage::~LogMessage() #2 0x7fb90ca11491 blink::LayoutTableSection::mapToVisualRectInAncestorSpace() #3 0x7fb90c9587e6 blink::LayoutBox::mapToVisualRectInAncestorSpace() #4 0x7fb90c9587e6 blink::LayoutBox::mapToVisualRectInAncestorSpace() #5 0x7fb90c91924a blink::IntersectionGeometry::clipToRoot() #6 0x7fb90c9198b6 blink::IntersectionGeometry::computeGeometry() #7 0x7fb90c408d58 blink::IntersectionObservation::computeIntersectionObservations() #8 0x7fb90c40b0cf blink::IntersectionObserver::computeIntersectionObservations() #9 0x7fb90c40fa45 blink::IntersectionObserverController::computeTrackedIntersectionObservations() #10 0x7fb90c62fed5 blink::FrameView::updateViewportIntersectionsForSubtree() #11 0x7fb90c62ff78 blink::FrameView::updateViewportIntersectionsForSubtree() #12 0x7fb90c62ff78 blink::FrameView::updateViewportIntersectionsForSubtree() #13 0x7fb90c62ff78 blink::FrameView::updateViewportIntersectionsForSubtree() #14 0x7fb90c62f5e4 blink::FrameView::updateLifecyclePhasesInternal() #15 0x7fb90c62eed1 blink::FrameView::updateAllLifecyclePhases() #16 0x7fb90cb4f14a blink::PageAnimator::updateAllLifecyclePhases() #17 0x7fb90bf47428 blink::WebViewImpl::updateAllLifecyclePhases()
,
Jan 18 2017
,
Jan 18 2017
Thanks mstensho@opera.com for the experiment. This exposed multiple issues. Filed bug 682364 . The following crash is also related to viewport intersection though not related to frame throttling: [563/5020] WebFrameSwapTest.WindowOpenOnRemoteFrame (49 ms) [ RUN ] WebFrameVisibilityChangeTest.RemoteFrameVisibilityChange [9171:9171:0118/052934.905635:10615419408:FATAL:LayoutBox.cpp(2332)] Check failed: !needsLayout(). #0 0x000001e565be base::debug::StackTrace::StackTrace() #1 0x000001e6340b logging::LogMessage::~LogMessage() #2 0x0000032101e0 blink::LayoutBox::mapToVisualRectInAncestorSpace() #3 0x000002f3714f blink::RemoteFrameView::updateRemoteViewportIntersection() #4 0x000002f36dde blink::RemoteFrameView::frameRectsChanged() #5 0x000002ee671c blink::FrameView::addChild() #6 0x000002fa4e08 blink::HTMLFrameOwnerElement::UpdateSuspendScope::performDeferredWidgetTreeOperations() #7 0x000002fa5710 blink::HTMLFrameOwnerElement::UpdateSuspendScope::~UpdateSuspendScope() #8 0x000002c694d3 blink::Document::updateStyle() #9 0x000002c64d95 blink::Document::updateStyleAndLayoutTree() #10 0x000002ef9044 blink::FrameView::updateStyleAndLayoutIfNeededRecursiveInternal() #11 0x000002ef6b31 blink::FrameView::updateStyleAndLayoutIfNeededRecursive() #12 0x000002ef5b39 blink::FrameView::updateLifecyclePhasesInternal() #13 0x000002ef5941 blink::FrameView::updateAllLifecyclePhases() #14 0x00000340efda blink::PageAnimator::updateAllLifecyclePhases() #15 0x000001e15c88 blink::WebViewImpl::updateAllLifecyclePhases() #16 0x0000007dd761 blink::WebFrameVisibilityChangeTest_RemoteFrameVisibilityChange_Test::TestBody()
,
Jan 19 2017
,
Jan 19 2017
+kenrb in case he is the right person to tackle this (based on issue 615156 )
,
Jan 19 2017
The stack in comment 3 is related to my CL, and I could investigate addressing that, but I am not the right person to handle the frame throttling issue that this bug initially describes.
,
Apr 10 2017
Over to szager as per comment 6.
,
May 2 2017
Is this on your radar szager?
,
May 2 2017
This was totally not on my radar, but it is now. I have a speculative fix: https://codereview.chromium.org/2859543003/
,
May 2 2017
Thanks!
,
May 2 2017
Hmm, on further thought, that patch is not right. It's definitely bogus that mapToVisualRectInAncestorSpace is called when layout is dirty. From the stack traces, I see two separate issues: 1. FrameView::updateViewportIntersectionsForSubtree should not run IntersectionObservers if the FrameView needs layout. That seems pretty straightforward. 2. RemoteFrameView::updateRemoteViewportIntersection should not run when the LocalFrame tree is dirty. I don't think it's safe or correct to run that method from RemoteFrameView::frameRectsChanged. Instead, it needs to run during UpdateAllLifecyclePhases for the RemoteFrameView's FrameView parent. Probably, FrameView::updateViewportIntersectionsForSubtree should loop through its RemoteFrameView children and call updateRemoteViewportIntersection on each. @kenrb, does #2 sound right to you?
,
May 2 2017
szager@: Yes. In fact I wrote a small patch last week that changes FrameView::updateViewportIntersectionsForSubtree in exactly the way you just suggested, because it fixes it a separate bug (frame throttling of subframes in an OOPIF process is sometimes broken). I have been having a bit of trouble getting a layout test working, so it isn't uploaded yet. I can also add the change that removes the updateRemoteViewportIntersection call from RemoteFrameView::frameRectsChanged.
,
May 3 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/edf83955248f5f88792cec8a4e2618e2670a33cc commit edf83955248f5f88792cec8a4e2618e2670a33cc Author: szager <szager@chromium.org> Date: Wed May 03 21:12:23 2017 Don't run IntersectionObserver lifecycle in frames with dirty layout. IntersectionObserver calls into layout geometry code, and it's bogus to run that code while layout is dirty. BUG= 682307 Review-Url: https://codereview.chromium.org/2859543003 Cr-Commit-Position: refs/heads/master@{#469124} [modify] https://crrev.com/edf83955248f5f88792cec8a4e2618e2670a33cc/third_party/WebKit/Source/core/frame/FrameView.cpp
,
May 4 2017
kenrb, I'm going to reassign this to you for the updateViewportIntersection part.
,
May 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/321a171abcca381e3ef72d01996e4ae4dd860212 commit 321a171abcca381e3ef72d01996e4ae4dd860212 Author: kenrb <kenrb@chromium.org> Date: Mon May 08 14:13:32 2017 Propagate viewport intersection across OOPIFs at the correct time This corrects a couple of issues with how viewport intersection rects were being sent to OOPIF processes from their parents, which was causing frame throttling bugs: 1. The update is now triggered from the parent FrameView's UpdateViewportIntersectionsForSubtree, rather than the RemoteFrameView's frameRectsChanged. Formerly, this would cause intersection calculations in a dirty layout tree, and also fail to propagate intersection rect changes in the case of nested OOPIFs. 2. Viewport intersections are now updated properly when the iframe is scrolled off of screen. Previously it would return early when an OOPIF had no viewport intersection, which was wrong. BUG= 682307 , 712320 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2860903003 Cr-Commit-Position: refs/heads/master@{#469977} [add] https://crrev.com/321a171abcca381e3ef72d01996e4ae4dd860212/third_party/WebKit/LayoutTests/http/tests/intersection-observer/cross-origin-iframe-with-nesting.html [add] https://crrev.com/321a171abcca381e3ef72d01996e4ae4dd860212/third_party/WebKit/LayoutTests/http/tests/intersection-observer/resources/nested-cross-origin-subframe.html [add] https://crrev.com/321a171abcca381e3ef72d01996e4ae4dd860212/third_party/WebKit/LayoutTests/http/tests/intersection-observer/resources/nesting-cross-origin-subframe.html [modify] https://crrev.com/321a171abcca381e3ef72d01996e4ae4dd860212/third_party/WebKit/Source/core/frame/FrameView.cpp [modify] https://crrev.com/321a171abcca381e3ef72d01996e4ae4dd860212/third_party/WebKit/Source/core/frame/RemoteFrameView.cpp [modify] https://crrev.com/321a171abcca381e3ef72d01996e4ae4dd860212/third_party/WebKit/Source/core/frame/RemoteFrameView.h
,
May 8 2017
,
May 9 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c4999457b7f0e7121f4067d9bdf86a93299a8c94 commit c4999457b7f0e7121f4067d9bdf86a93299a8c94 Author: Charles Reis <creis@chromium.org> Date: Tue May 09 23:05:45 2017 Propagate viewport intersection across OOPIFs at the correct time This corrects a couple of issues with how viewport intersection rects were being sent to OOPIF processes from their parents, which was causing frame throttling bugs: 1. The update is now triggered from the parent FrameView's UpdateViewportIntersectionsForSubtree, rather than the RemoteFrameView's frameRectsChanged. Formerly, this would cause intersection calculations in a dirty layout tree, and also fail to propagate intersection rect changes in the case of nested OOPIFs. 2. Viewport intersections are now updated properly when the iframe is scrolled off of screen. Previously it would return early when an OOPIF had no viewport intersection, which was wrong. BUG= 682307 , 712320 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2860903003 Cr-Commit-Position: refs/heads/master@{#469977} (cherry picked from commit 321a171abcca381e3ef72d01996e4ae4dd860212) Review-Url: https://codereview.chromium.org/2873083002 . Cr-Commit-Position: refs/branch-heads/3071@{#489} Cr-Branched-From: a106f0abbf69dad349d4aaf4bcc4f5d376dd2377-refs/heads/master@{#464641} [add] https://crrev.com/c4999457b7f0e7121f4067d9bdf86a93299a8c94/third_party/WebKit/LayoutTests/http/tests/intersection-observer/cross-origin-iframe-with-nesting.html [add] https://crrev.com/c4999457b7f0e7121f4067d9bdf86a93299a8c94/third_party/WebKit/LayoutTests/http/tests/intersection-observer/resources/nested-cross-origin-subframe.html [add] https://crrev.com/c4999457b7f0e7121f4067d9bdf86a93299a8c94/third_party/WebKit/LayoutTests/http/tests/intersection-observer/resources/nesting-cross-origin-subframe.html [modify] https://crrev.com/c4999457b7f0e7121f4067d9bdf86a93299a8c94/third_party/WebKit/Source/core/frame/FrameView.cpp [modify] https://crrev.com/c4999457b7f0e7121f4067d9bdf86a93299a8c94/third_party/WebKit/Source/core/frame/RemoteFrameView.cpp [modify] https://crrev.com/c4999457b7f0e7121f4067d9bdf86a93299a8c94/third_party/WebKit/Source/core/frame/RemoteFrameView.h |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by msten...@opera.com
, Jan 18 2017