Bug 135514 – Layers need to be already updated before we call adjustViewSize()

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

In FrameView::layout(), we currently call adjustViewSize() before updateLayerPositionsAfterLayout(). This is bad because adjustViewSize() can result in scrolling and painting, so layers need to have been fully updated at this point. In some cases we rely on updateLayerPositionsAfterLayout() to have been called to update RenderLayer pointers (e.g. for m_enclosingPaginationLayer) so it's really bad to not have done this by adjustViewSize time.

[Simon Fraser (smfr)]

Here's another instance of this.

Hit-test is forcing layout. Before we've updated layers, FrameView::adjustViewSize() is causing scrolling, which does FrameView::updateCompositingLayersAfterScrolling() even though we haven't updated layers after the layout yet.

(lldb) bt
* thread #1: tid = 0x61bcd, 0x000000010bbfad2c WebCore`WebCore::RenderGeometryMap::mapToContainer(this=0x00007fff5d9a2008, rect=0x00007fff5d9a1480, container=0x0000000000000000) const + 988 at RenderGeometryMap.cpp:143, queue = 'com.apple.main-thread', stop reason = breakpoint 8.1
  * frame #0: 0x000000010bbfad2c WebCore`WebCore::RenderGeometryMap::mapToContainer(this=0x00007fff5d9a2008, rect=0x00007fff5d9a1480, container=0x0000000000000000) const + 988 at RenderGeometryMap.cpp:143
    frame #1: 0x000000010bc2f43e WebCore`WebCore::RenderGeometryMap::absoluteRect(this=0x00007fff5d9a2008, rect=0x00007fff5d9a1480) const + 62 at RenderGeometryMap.h:90
    frame #2: 0x000000010bc7f245 WebCore`WebCore::RenderLayerCompositor::computeCompositingRequirements(this=0x00007ff8bbe04680, ancestorLayer=0x00007ff8c2932bd0, layer=0x00007ff8c2932e30, overlapMap=0x00007fff5d9a1fd0, compositingState=0x00007fff5d9a1608, layersChanged=0x00007fff5d9a2447, descendantHas3DTransform=0x00007fff5d9a15f7) + 453 at RenderLayerCompositor.cpp:1233
    frame #3: 0x000000010bc7f655 WebCore`WebCore::RenderLayerCompositor::computeCompositingRequirements(this=0x00007ff8bbe04680, ancestorLayer=0x00007ff8c2932aa0, layer=0x00007ff8c2932bd0, overlapMap=0x00007fff5d9a1fd0, compositingState=0x00007fff5d9a17b8, layersChanged=0x00007fff5d9a2447, descendantHas3DTransform=0x00007fff5d9a17a7) + 1493 at RenderLayerCompositor.cpp:1317
    frame #4: 0x000000010bc7f655 WebCore`WebCore::RenderLayerCompositor::computeCompositingRequirements(this=0x00007ff8bbe04680, ancestorLayer=0x00007ff8c2932970, layer=0x00007ff8c2932aa0, overlapMap=0x00007fff5d9a1fd0, compositingState=0x00007fff5d9a1968, layersChanged=0x00007fff5d9a2447, descendantHas3DTransform=0x00007fff5d9a1957) + 1493 at RenderLayerCompositor.cpp:1317
    frame #5: 0x000000010bc7f655 WebCore`WebCore::RenderLayerCompositor::computeCompositingRequirements(this=0x00007ff8bbe04680, ancestorLayer=0x00007ff8b8fa83b0, layer=0x00007ff8c2932970, overlapMap=0x00007fff5d9a1fd0, compositingState=0x00007fff5d9a1b18, layersChanged=0x00007fff5d9a2447, descendantHas3DTransform=0x00007fff5d9a1b07) + 1493 at RenderLayerCompositor.cpp:1317
    frame #6: 0x000000010bc7f655 WebCore`WebCore::RenderLayerCompositor::computeCompositingRequirements(this=0x00007ff8bbe04680, ancestorLayer=0x00007ff8b8f09040, layer=0x00007ff8b8fa83b0, overlapMap=0x00007fff5d9a1fd0, compositingState=0x00007fff5d9a1cc8, layersChanged=0x00007fff5d9a2447, descendantHas3DTransform=0x00007fff5d9a1cb7) + 1493 at RenderLayerCompositor.cpp:1317
    frame #7: 0x000000010bc7f734 WebCore`WebCore::RenderLayerCompositor::computeCompositingRequirements(this=0x00007ff8bbe04680, ancestorLayer=0x00007ff8bbe03890, layer=0x00007ff8b8f09040, overlapMap=0x00007fff5d9a1fd0, compositingState=0x00007fff5d9a1e78, layersChanged=0x00007fff5d9a2447, descendantHas3DTransform=0x00007fff5d9a1e67) + 1716 at RenderLayerCompositor.cpp:1323
    frame #8: 0x000000010bc7f734 WebCore`WebCore::RenderLayerCompositor::computeCompositingRequirements(this=0x00007ff8bbe04680, ancestorLayer=0x0000000000000000, layer=0x00007ff8bbe03890, overlapMap=0x00007fff5d9a1fd0, compositingState=0x00007fff5d9a2448, layersChanged=0x00007fff5d9a2447, descendantHas3DTransform=0x00007fff5d9a2446) + 1716 at RenderLayerCompositor.cpp:1323
    frame #9: 0x000000010bc7ea70 WebCore`WebCore::RenderLayerCompositor::updateCompositingLayers(this=0x00007ff8bbe04680, updateType=CompositingUpdateOnScroll, updateRoot=0x00007ff8bbe03890) + 960 at RenderLayerCompositor.cpp:729
    frame #10: 0x000000010adcc3db WebCore`WebCore::FrameView::updateCompositingLayersAfterScrolling(this=0x00007ff8bf00b200) + 139 at FrameView.cpp:2166
    frame #11: 0x000000010bf1c95a WebCore`WebCore::ScrollView::scrollTo(this=0x00007ff8bf00b200, newOffset=0x00007fff5d9a2610) + 186 at ScrollView.cpp:478
    frame #12: 0x000000010adcf971 WebCore`WebCore::FrameView::scrollTo(this=0x00007ff8bf00b200, newOffset=0x00007fff5d9a2610) + 81 at FrameView.cpp:3235
    frame #13: 0x000000010bf1c84a WebCore`WebCore::ScrollView::setScrollOffset(this=0x00007ff8bf00b200, offset=0x00007fff5d9a2a58) + 1002 at ScrollView.cpp:457
    frame #14: 0x000000010bf1c88f WebCore`non-virtual thunk to WebCore::ScrollView::setScrollOffset(this=0x00007ff8bf00b238, offset=0x00007fff5d9a2a58) + 47 at ScrollView.cpp:458
    frame #15: 0x000000010bed101d WebCore`WebCore::ScrollableArea::scrollPositionChanged(this=0x00007ff8bf00b238, position=0x00007fff5d9a2a58) + 93 at ScrollableArea.cpp:156
    frame #16: 0x000000010bed0f8f WebCore`WebCore::ScrollableArea::notifyScrollPositionChanged(this=0x00007ff8bf00b238, position=0x00007fff5d9a2a58) + 47 at ScrollableArea.cpp:148
    frame #17: 0x000000010a5c394e WebCore`WebCore::AsyncScrollingCoordinator::updateScrollPositionAfterAsyncScroll(this=0x00007ff8b8d020a0, scrollingNodeID=3, scrollPosition=0x00007fff5d9a2ae8, programmaticScroll=true, scrollingLayerPositionAction=SetScrollingLayerPosition) + 302 at AsyncScrollingCoordinator.cpp:269
    frame #18: 0x000000010a5c3745 WebCore`WebCore::AsyncScrollingCoordinator::requestScrollPositionUpdate(this=0x00007ff8b8d020a0, frameView=0x00007ff8bf00b200, scrollPosition=0x00007fff5d9a2c18) + 309 at AsyncScrollingCoordinator.cpp:180
    frame #19: 0x000000010adccab3 WebCore`WebCore::FrameView::requestScrollPositionUpdate(this=0x00007ff8bf00b200, position=0x00007fff5d9a2c18) + 259 at FrameView.cpp:2202
    frame #20: 0x000000010adccb0f WebCore`non-virtual thunk to WebCore::FrameView::requestScrollPositionUpdate(this=0x00007ff8bf00b238, position=0x00007fff5d9a2c18) + 47 at FrameView.cpp:2209
    frame #21: 0x000000010bed1382 WebCore`WebCore::ScrollableArea::setScrollOffsetFromAnimation(this=0x00007ff8bf00b238, offset=0x00007fff5d9a2c18) + 50 at ScrollableArea.cpp:208
    frame #22: 0x000000010bed46d1 WebCore`WebCore::ScrollAnimator::notifyPositionChanged(this=0x00007ff8b8c3b820, delta=0x00007fff5d9a2c98) + 65 at ScrollAnimator.cpp:170
    frame #23: 0x000000010bedc846 WebCore`WebCore::ScrollAnimatorMac::notifyPositionChanged(this=0x00007ff8b8c3b820, delta=0x00007fff5d9a2c98) + 70 at ScrollAnimatorMac.mm:786
    frame #24: 0x000000010bedc2da WebCore`WebCore::ScrollAnimatorMac::immediateScrollTo(this=0x00007ff8b8c3b820, newPosition=0x00007fff5d9a2eb8) + 282 at ScrollAnimatorMac.mm:765
    frame #25: 0x000000010bedc1b3 WebCore`WebCore::ScrollAnimatorMac::scrollToOffsetWithoutAnimation(this=0x00007ff8b8c3b820, offset=0x00007fff5d9a2eb8) + 67 at ScrollAnimatorMac.mm:727
    frame #26: 0x000000010bed0e6c WebCore`WebCore::ScrollableArea::scrollToOffsetWithoutAnimation(this=0x00007ff8bf00b238, offset=0x00007fff5d9a2eb8) + 60 at ScrollableArea.cpp:135
    frame #27: 0x000000010bf1b272 WebCore`WebCore::ScrollView::updateScrollbars(this=0x00007ff8bf00b200, desiredOffset=0x00007fff5d9a3010) + 5170 at ScrollView.cpp:734
    frame #28: 0x000000010bf1bd45 WebCore`WebCore::ScrollView::setContentsSize(this=0x00007ff8bf00b200, newSize=0x00007fff5d9a30e0) + 165 at ScrollView.cpp:385
    frame #29: 0x000000010adc3960 WebCore`WebCore::FrameView::setContentsSize(this=0x00007ff8bf00b200, size=0x00007fff5d9a30e0) + 112 at FrameView.cpp:558
    frame #30: 0x000000010adc49d5 WebCore`WebCore::FrameView::adjustViewSize(this=0x00007ff8bf00b200) + 357 at FrameView.cpp:591
    frame #31: 0x000000010adc6e96 WebCore`WebCore::FrameView::layout(this=0x00007ff8bf00b200, allowSubtree=true) + 3462 at FrameView.cpp:1339
    frame #32: 0x000000010aa2710d WebCore`WebCore::Document::updateLayout(this=0x00007ff8bf000000) + 349 at Document.cpp:1868
    frame #33: 0x000000010be12ba3 WebCore`WebCore::RenderView::hitTest(this=0x00007ff8bbe03f60, request=0x00007fff5d9a3760, location=0x00007fff5d9a3418, result=0x00007fff5d9a3418) + 51 at RenderView.cpp:177
    frame #34: 0x000000010be12b64 WebCore`WebCore::RenderView::hitTest(this=0x00007ff8bbe03f60, request=0x00007fff5d9a3760, result=0x00007fff5d9a3418) + 68 at RenderView.cpp:172
    frame #35: 0x000000010aa2fecb WebCore`WebCore::Document::prepareMouseEvent(this=0x00007ff8bf000000, request=0x00007fff5d9a3760, documentPoint=0x00007fff5d9a3590, event=0x00007fff5d9a39e8) + 203 at Document.cpp:3071
    frame #36: 0x000000010ac02f7f WebCore`WebCore::EventHandler::prepareMouseEvent(this=0x00007ff8b8c14790, request=0x00007fff5d9a3760, mouseEvent=0x00007fff5d9a39e8) + 191 at EventHandler.cpp:2356
    frame #37: 0x000000010ac034d6 WebCore`WebCore::EventHandler::handleMouseMoveEvent(this=0x00007ff8b8c14790, platformMouseEvent=0x00007fff5d9a39e8, hoveredNode=0x00007fff5d9a3800, onlyUpdateScrollbars=false) + 710 at EventHandler.cpp:1933
    frame #38: 0x000000010ac03056 WebCore`WebCore::EventHandler::mouseMoved(this=0x00007ff8b8c14790, event=0x00007fff5d9a39e8) + 198 at EventHandler.cpp:1847

[Brent Fulgham]

*** Bug 140455 has been marked as a duplicate of this bug. ***

[Brent Fulgham]

<rdar://problem/17873488>

[Brent Fulgham]

Created attachment 244633 [details]
Patch

[zalan]

Comment on attachment 244633 [details]
Patch

So why don't call updateLayerPositionsAfterLayout() in FrameView::adjustViewSize()?

[Brent Fulgham]

Created attachment 244651 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 244651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244651&action=review

> Source/WebCore/page/FrameView.cpp:1159
> +    TemporaryChange<bool> changeDelayScrollUpdateDuringLayout(m_delayScrollUpdateDuringLayout, true);

I think this scope is too long, because it's going to be set for all of the postLayoutTasks code.

This is a good argument for splitting up FrameView::layout() into a few functions with more explicit purposes.

> Source/WebCore/page/FrameView.cpp:1419
> +void FrameView::performTasksDeferredDuringLayout()
> +{
> +    ScrollView::adjustContentsSizeAfterLayout();
> +}

Not sure if there's much point in adding a new function with "deferred tasks" in the name just to call other function. We might as well just call ScrollView::adjustContentsSizeAfterLayout() directly.

> Source/WebCore/page/FrameView.cpp:3826
> +    ASSERT(m_layoutPhase == InPostLayout || m_layoutPhase == OutsideLayout);

This one is wrong; we need to assert that updateLayerPositionsAfterLayout() has happened, which implies a new layout phase. Ideally these layout phases would correlate with some new layout-related function names.

> Source/WebCore/platform/ScrollView.h:422
> +    bool m_delayScrollUpdateDuringLayout; // protected so we can use it with TemporaryChange.

It's a bit gross to have this layout-related flag here in platform code. Maybe make a virtual bool shouldDeferScrollUpdateAfterContentSizeChange(), and void handleDeferredScrollUpdateAfterContentSizeChange()?

[Brent Fulgham]

Comment on attachment 244651 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244651&action=review

>> Source/WebCore/page/FrameView.cpp:1159
>> +    TemporaryChange<bool> changeDelayScrollUpdateDuringLayout(m_delayScrollUpdateDuringLayout, true);
> 
> I think this scope is too long, because it's going to be set for all of the postLayoutTasks code.
> 
> This is a good argument for splitting up FrameView::layout() into a few functions with more explicit purposes.

Yes; I had to move it further down to get the right behavior.

>> Source/WebCore/page/FrameView.cpp:1419
>> +}
> 
> Not sure if there's much point in adding a new function with "deferred tasks" in the name just to call other function. We might as well just call ScrollView::adjustContentsSizeAfterLayout() directly.

Yeah, I thought there might be other instances of this pattern, but I'll just use the direct function call until we discover some other case that needs this.

>> Source/WebCore/page/FrameView.cpp:3826
>> +    ASSERT(m_layoutPhase == InPostLayout || m_layoutPhase == OutsideLayout);
> 
> This one is wrong; we need to assert that updateLayerPositionsAfterLayout() has happened, which implies a new layout phase. Ideally these layout phases would correlate with some new layout-related function names.

Ok. I'll add a new phase, but I don't feel comfortable refactoring the layout function in this patch.

>> Source/WebCore/platform/ScrollView.h:422
>> +    bool m_delayScrollUpdateDuringLayout; // protected so we can use it with TemporaryChange.
> 
> It's a bit gross to have this layout-related flag here in platform code. Maybe make a virtual bool shouldDeferScrollUpdateAfterContentSizeChange(), and void handleDeferredScrollUpdateAfterContentSizeChange()?

Will do.

[Brent Fulgham]

Created attachment 244659 [details]
Patch

[Brent Fulgham]

Comment on attachment 244659 [details]
Patch

Ugh. The indentation change made a mess of this patch.

[Brent Fulgham]

Created attachment 244663 [details]
Patch

[Brent Fulgham]

Created attachment 244664 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 244664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244664&action=review

> Source/WebCore/page/FrameView.h:545
> +        InPostLayerPositionsUpdatedAfterLayout,

At some point we should change these to describe states better.

I don't think we set the state back to OutsideLayout at the end of layout(), and probably should.

[Brent Fulgham]

Tests on Windows pass with this change (at least locally), with two exceptions:

1. fast/html/marquee-scroll.html

--- /cygdrive/c/Projects/WebKit/OpenSource/WebKitBuild/Release/bin32/layout-test-results/fast/html/marquee-scroll-expected.txt
+++ /cygdrive/c/Projects/WebKit/OpenSource/WebKitBuild/Release/bin32/layout-test-results/fast/html/marquee-scroll-actual.txt
@@ -17,7 +17,7 @@
           text run at (0,18) width 557: "It should scroll until the text, \"The marquee scroll test passed\" is visible and left justified."
       RenderBlock (anonymous) at (0,86) size 784x18
         RenderText {#text} at (0,0) size 0x0
-layer at (8,94) size 392x18 scrollX -392
+layer at (8,94) size 392x18 scrollX -384
   RenderBlock {MARQUEE} at (0,0) size 392x18 [bgcolor=#00FFFF]
     RenderText {#text} at (0,0) size 192x18
       text run at (0,0) width 192: "The marquee scroll test passed"

2. fast/inline-block/003.html

--- /cygdrive/c/Projects/WebKit/OpenSource/WebKitBuild/Release/bin32/layout-test-results/fast/inline-block/003-expected.txt
+++ /cygdrive/c/Projects/WebKit/OpenSource/WebKitBuild/Release/bin32/layout-test-results/fast/inline-block/003-actual.txt
@@ -22,11 +22,11 @@
           RenderText {#text} at (0,0) size 0x0
           RenderText {#text} at (0,0) size 0x0
         RenderText {#text} at (0,0) size 0x0
-layer at (8,66) size 784x22 clip at (10,68) size 780x18 scrollX -780
+layer at (8,66) size 784x22 clip at (10,68) size 780x18 scrollX -765
   RenderBlock {MARQUEE} at (0,18) size 784x22 [border: (2px solid #008000)]
     RenderText {#text} at (2,2) size 97x18
       text run at (2,2) width 97: "This is div one."
-layer at (8,88) size 784x22 clip at (10,90) size 780x18 scrollX -780
+layer at (8,88) size 784x22 clip at (10,90) size 780x18 scrollX -765
   RenderBlock {MARQUEE} at (0,40) size 784x22 [border: (2px solid #008000)]
     RenderText {#text} at (2,2) size 98x18
       text run at (2,2) width 98: "This is div two."

In both cases, the amount of scroll that took place during the test is slightly different.

Since we now block Windows from performing draw events on the scroll bars (especially during recursive layout), it may be that we are no longer hitting the scroll code multiple times.

All of the dedicated scrolling tests pass.

[Brent Fulgham]

Comment on attachment 244664 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244664&action=review

>> Source/WebCore/page/FrameView.h:545
>> +        InPostLayerPositionsUpdatedAfterLayout,
> 
> At some point we should change these to describe states better.
> 
> I don't think we set the state back to OutsideLayout at the end of layout(), and probably should.

I don't think we need to do anything here. You do this at the start of layout:

    TemporaryChange<LayoutPhase> layoutPhaseRestorer(m_layoutPhase, InPreLayout);

Assuming we always enter this method in the proper state of OutOfLayout, we should leave any recursive set of layout calls with the proper state at the end.

It's too bad we can't assert at the entry to this function that we are in OutOfLayout, but since we hit it recursively it won't work.

[Build Bot]

Comment on attachment 244664 [details]
Patch

Attachment 244664 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5259024069033984

New failing tests:
fast/html/marquee-scroll.html
pageoverlay/overlay-large-document-scrolled.html
compositing/regions/transformed-layer-inside-transformed-layer.html
compositing/regions/z-index-update.html
fast/inline-block/003.html
compositing/regions/propagate-region-box-shadow-border-padding.html
pageoverlay/overlay-installation.html
pageoverlay/overlay-large-document.html
compositing/regions/z-index.html

[Build Bot]

Created attachment 244673 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Brent Fulgham]

It looks like I'm deferring the scroll calculations too soon. We want to perform many of the calculations, but avoid actually forcing a draw event.

[Brent Fulgham]

Created attachment 244678 [details]
Patch

[Brent Fulgham]

This latest patch seems to correct things locally, though the compositing tests are skipped on Windows at the moment.

Let's see what the EWS says about this approach.

[Build Bot]

Comment on attachment 244678 [details]
Patch

Attachment 244678 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6259547974926336

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 244681 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Brent Fulgham]

Created attachment 244696 [details]
Patch

[Brent Fulgham]

The previous patch always called setScrollOffset, even when no scrolling had happened. In this case, the deferred scroll offset was an empty IntPoint, which caused the final layout to 'undo' whatever scrolling had happened.

I've revised the patch to do the delay closer defer only the part that triggered painting (i.e., moved the defer point deeper into the layout) so that more calculations are being done as they were before this change.

I also check that there is an offset to scroll to, rather than calling scrollTo with 0, 0 every time layout is called.

I'm wondering if this should be represented as a unique_ptr in case there ever are times when scrollTo is supposed to be called with an empty offset? That way we could check for nullptr and bail out if no work was to be done.

Let's see if EWS likes this version.

[zalan]

Comment on attachment 244696 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244696&action=review

> Source/WebCore/platform/ScrollView.cpp:463
> +    if (m_deferredScrollDelta.isEmpty())

I think it should check against isZero() as the delta might be negative (scrolling upwards).

[Brent Fulgham]

Created attachment 244698 [details]
Patch

[Brent Fulgham]

Comment on attachment 244698 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244698&action=review

> Source/WebCore/platform/ScrollView.cpp:490
> +        ASSERT(m_deferredScrollDelta.isZero());

I want to make sure we don't ever hit this in a nested context, such that I should be tracking a stack of these deferred scroll deltas.

[Simon Fraser (smfr)]

Comment on attachment 244698 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244698&action=review

> Source/WebCore/platform/ScrollView.cpp:489
> +    if (shouldDeferScrollUpdateAfterContentSizeChange()) {

I think it would be good to add a comment here explaining why the deferral is necessary.

[Brent Fulgham]

Committed r178531: <http://trac.webkit.org/changeset/178531>

[Joseph Pecoraro]

Looks like there are a lot of LayoutTest ASSERTS / crashes on the Yosemite debug bot after this change:
https://build.webkit.org/builders/Apple%20Yosemite%20Debug%20WK1%20%28Tests%29/builds/1561

Tests:

> Regressions: Unexpected crashes (50)
>   compositing/columns/composited-in-paginated-rl.html [ Crash ]
>   compositing/columns/composited-rl-paginated-repaint.html [ Crash ]
>   compositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html [ Crash ]
>   compositing/rtl/rtl-absolute-overflow-scrolled.html [ Crash ]
>   compositing/rtl/rtl-absolute-overflow.html [ Crash ]
>   compositing/rtl/rtl-overflow-invalidation.html [ Crash ]
>   editing/execCommand/delete-no-scroll.html [ Crash ]
>   ...


ASSERT looks like:

> stderr:
> ASSERTION FAILED: m_layoutPhase >= InPostLayout || m_layoutPhase == OutsideLayout
> /Volumes/Data/slave/yosemite-debug/build/Source/WebCore/page/FrameView.cpp(2193) : virtual void WebCore::FrameView::updateCompositingLayersAfterScrolling()
> 1   0x1058561c0 WTFCrash
> 2   0x109c1b98c WebCore::FrameView::updateCompositingLayersAfterScrolling()
> 3   0x109c1b86d WebCore::FrameView::scrollPositionChangedViaPlatformWidget(WebCore::IntPoint const&, WebCore::IntPoint const&)
> 4   0x11086551a -[WebHTMLView(WebPrivate) _frameOrBoundsChanged]
> 5   0x7fff8c772cbc __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__
> 6   0x7fff8c6641b4 _CFXNotificationPost
> 7   0x7fff8923fea1 -[NSNotificationCenter postNotificationName:object:userInfo:]
> 8   0x7fff84df26c8 -[NSView _postBoundsChangeNotification]
> 9   0x7fff84df24da -[NSView translateOriginToPoint:]
> 10  0x7fff84ccf431 -[NSClipView _immediateScrollToPoint:]
> 11  0x1107dd238 -[WebClipView _immediateScrollToPoint:]
> 12  0x7fff84ccebab -[NSClipView scrollToPoint:]
> 13  0x7fff84df194e -[NSScrollView scrollClipView:toPoint:]
> 14  0x11080ce1e -[WebDynamicScrollBarsView scrollClipView:toPoint:]
> 15  0x7fff84cd37ea -[NSClipView _scrollTo:animateScroll:flashScrollerKnobs:]
> 16  0x7fff84cd2f70 -[NSClipView _reflectDocumentViewFrameChange]
> 17  0x7fff84c9ca01 -[NSView _postFrameChangeNotification]
> 18  0x7fff84ca4826 -[NSView setFrameSize:]
> 19  0x7fff84c96bdd -[NSControl setFrameSize:]
> 20  0x10ae11982 WebCore::ScrollView::platformSetContentsSize()
> 21  0x10ae09306 WebCore::ScrollView::setContentsSize(WebCore::IntSize const&)
> 22  0x109c12be0 WebCore::FrameView::setContentsSize(WebCore::IntSize const&)
> 23  0x109c13c95 WebCore::FrameView::adjustViewSize()
> 24  0x109c1616a WebCore::FrameView::layout(bool)
> 25  0x109c220a5 WebCore::FrameView::forceLayout(bool)
> 26  0x11087087e -[WebHTMLView layoutToMinimumPageWidth:height:originalPageWidth:originalPageHeight:maximumShrinkRatio:adjustingViewSize:]
> 27  0x1108708fc -[WebHTMLView layout]
> 28  0x11080d7a5 -[WebDynamicScrollBarsView(WebInternal) updateScrollers]
> 29  0x11080e7a9 -[WebDynamicScrollBarsView(WebInternal) setScrollingModes:vertical:andLock:]
> 30  0x10ae10a3e WebCore::ScrollView::platformSetScrollbarModes()
> 31  0x10ae073ef WebCore::ScrollView::setScrollbarModes(WebCore::ScrollbarMode, WebCore::ScrollbarMode, bool, bool)

I'm going to roll out to prevent the bot being red.

[Joseph Pecoraro]

Rolled out: <http://trac.webkit.org/changeset/178565>

[Brent Fulgham]

(In reply to comment #30)
> Looks like there are a lot of LayoutTest ASSERTS / crashes on the Yosemite
> debug bot after this change:
> https://build.webkit.org/builders/
> Apple%20Yosemite%20Debug%20WK1%20%28Tests%29/builds/1561
> 
> Tests:
> 
> > Regressions: Unexpected crashes (50)
> >   compositing/columns/composited-in-paginated-rl.html [ Crash ]
> >   compositing/columns/composited-rl-paginated-repaint.html [ Crash ]
> >   compositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html [ Crash ]
> >   compositing/rtl/rtl-absolute-overflow-scrolled.html [ Crash ]
> >   compositing/rtl/rtl-absolute-overflow.html [ Crash ]
> >   compositing/rtl/rtl-overflow-invalidation.html [ Crash ]
> >   editing/execCommand/delete-no-scroll.html [ Crash ]
> >   ...
> 
> 
> ASSERT looks like:
> 
> > stderr:
> > ASSERTION FAILED: m_layoutPhase >= InPostLayout || m_layoutPhase == OutsideLayout
> > /Volumes/Data/slave/yosemite-debug/build/Source/WebCore/page/FrameView.cpp(2193) : virtual void WebCore::FrameView::updateCompositingLayersAfterScrolling()
> > 1   0x1058561c0 WTFCrash
> > 2   0x109c1b98c WebCore::FrameView::updateCompositingLayersAfterScrolling()
> > 3   0x109c1b86d WebCore::FrameView::scrollPositionChangedViaPlatformWidget(WebCore::IntPoint const&, WebCore::IntPoint const&)
> > 4   0x11086551a -[WebHTMLView(WebPrivate) _frameOrBoundsChanged]
> > 5   0x7fff8c772cbc __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__
> > 6   0x7fff8c6641b4 _CFXNotificationPost
> > 7   0x7fff8923fea1 -[NSNotificationCenter postNotificationName:object:userInfo:]
> > 8   0x7fff84df26c8 -[NSView _postBoundsChangeNotification]
> > 9   0x7fff84df24da -[NSView translateOriginToPoint:]
> > 10  0x7fff84ccf431 -[NSClipView _immediateScrollToPoint:]
> > 11  0x1107dd238 -[WebClipView _immediateScrollToPoint:]
> > 12  0x7fff84ccebab -[NSClipView scrollToPoint:]
> > 13  0x7fff84df194e -[NSScrollView scrollClipView:toPoint:]
> > 14  0x11080ce1e -[WebDynamicScrollBarsView scrollClipView:toPoint:]
> > 15  0x7fff84cd37ea -[NSClipView _scrollTo:animateScroll:flashScrollerKnobs:]
> > 16  0x7fff84cd2f70 -[NSClipView _reflectDocumentViewFrameChange]
> > 17  0x7fff84c9ca01 -[NSView _postFrameChangeNotification]
> > 18  0x7fff84ca4826 -[NSView setFrameSize:]
> > 19  0x7fff84c96bdd -[NSControl setFrameSize:]
> > 20  0x10ae11982 WebCore::ScrollView::platformSetContentsSize()
> > 21  0x10ae09306 WebCore::ScrollView::setContentsSize(WebCore::IntSize const&)
> > 22  0x109c12be0 WebCore::FrameView::setContentsSize(WebCore::IntSize const&)
> > 23  0x109c13c95 WebCore::FrameView::adjustViewSize()
> > 24  0x109c1616a WebCore::FrameView::layout(bool)
> > 25  0x109c220a5 WebCore::FrameView::forceLayout(bool)
> > 26  0x11087087e -[WebHTMLView layoutToMinimumPageWidth:height:originalPageWidth:originalPageHeight:maximumShrinkRatio:adjustingViewSize:]
> > 27  0x1108708fc -[WebHTMLView layout]
> > 28  0x11080d7a5 -[WebDynamicScrollBarsView(WebInternal) updateScrollers]
> > 29  0x11080e7a9 -[WebDynamicScrollBarsView(WebInternal) setScrollingModes:vertical:andLock:]
> > 30  0x10ae10a3e WebCore::ScrollView::platformSetScrollbarModes()
> > 31  0x10ae073ef WebCore::ScrollView::setScrollbarModes(WebCore::ScrollbarMode, WebCore::ScrollbarMode, bool, bool)
> 
> I'm going to roll out to prevent the bot being red.

Yes -- these are saying that the FrameView is in the wrong 'mode' for drawing. This is likely just a matter of things not being reset properly when going through this direct-to-painting operation.

[Brent Fulgham]

The problem is that the failing tests take the alternative platformWidget case, which needs the same protection as the non-platform case.

I think my original approach was right, but was mistakenly setting the offset to zero scroll offset regardless of whether we meant to defer anything.

[Brent Fulgham]

After debugging further, the problems was that we hit the platformWidget code path in ScrollView::setContentsSize. This code path also prompts drawing during layout.

There were two cases I saw during debugging the various layout tests:

1. For some platform widgets, we would pass through 'FrameView::scrollPositionChangedViaPlatformWidget', which was the point where paint operations could be triggered. This could be handled the same way as ScrollView::scrollTo by retaining some state and deferring the operation.

2. A handful of cases did not pass through the method (1) above, landing in FrameView::willPaintContents with the following stack trace:

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x00000001063b41ca WTFCrash + 42
1   com.apple.WebCore             	0x000000010a77b98c WebCore::FrameView::updateCompositingLayersAfterScrolling() + 92 (FrameView.cpp:2193)
2   com.apple.WebCore             	0x000000010a77b86d WebCore::FrameView::scrollPositionChangedViaPlatformWidget(WebCore::IntPoint const&, WebCore::IntPoint const&) + 45 (FrameView.cpp:2113)
3   com.apple.WebKitLegacy        	0x00000001113c251a -[WebHTMLView(WebPrivate) _frameOrBoundsChanged] + 538 (WebHTMLView.mm:1324)
4   com.apple.CoreFoundation      	0x00007fff8c772cbc __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ + 12
5   com.apple.CoreFoundation      	0x00007fff8c6641b4 _CFXNotificationPost + 3140
6   com.apple.Foundation          	0x00007fff8923fea1 -[NSNotificationCenter postNotificationName:object:userInfo:] + 66
7   com.apple.AppKit              	0x00007fff84df26c8 -[NSView _postBoundsChangeNotification] + 186
8   com.apple.AppKit              	0x00007fff84df24da -[NSView translateOriginToPoint:] + 287
9   com.apple.AppKit              	0x00007fff84ccf431 -[NSClipView _immediateScrollToPoint:] + 2015
10  com.apple.WebKitLegacy        	0x000000011133a238 -[WebClipView _immediateScrollToPoint:] + 152 (WebClipView.mm:112)
11  com.apple.AppKit              	0x00007fff84ccebab -[NSClipView scrollToPoint:] + 241
12  com.apple.AppKit              	0x00007fff84df194e -[NSScrollView scrollClipView:toPoint:] + 75
13  com.apple.WebKitLegacy        	0x0000000111369e1e -[WebDynamicScrollBarsView scrollClipView:toPoint:] + 142 (WebDynamicScrollBarsView.mm:219)
14  com.apple.AppKit              	0x00007fff84cd37ea -[NSClipView _scrollTo:animateScroll:flashScrollerKnobs:] + 1682
15  com.apple.AppKit              	0x00007fff84cd2f70 -[NSClipView _reflectDocumentViewFrameChange] + 128
16  com.apple.AppKit              	0x00007fff84c9ca01 -[NSView _postFrameChangeNotification] + 206
17  com.apple.AppKit              	0x00007fff84ca4826 -[NSView setFrameSize:] + 2059
18  com.apple.AppKit              	0x00007fff84c96bdd -[NSControl setFrameSize:] + 77
19  com.apple.WebCore             	0x000000010b971982 WebCore::ScrollView::platformSetContentsSize() + 770 (ScrollViewMac.mm:189)
20  com.apple.WebCore             	0x000000010b969306 WebCore::ScrollView::setContentsSize(WebCore::IntSize const&) + 134 (ScrollView.cpp:383)

In this second case, there is no convenient spot to head off painting operations. If I try to avoid painting in WebCore::ScrollView::platformSetContentsSize, the content size of the various widgets are not updated, which causes layout problems.

The best solution I could come up with was to do the same as in FrameView::updateLayerPositionsAfterScrolling and check if our layout phase is InViewSizeAdjust and bail out early in FrameView::paintContents.

[Brent Fulgham]

It also appears that we can hit this in a nested context (at least in the case of "fast/transforms/selection-bounds-in-transformed-view.html"). This means we need to retain a stack of these deferred scroll operations so that we can unwind them properly.

[zalan]

(In reply to comment #35)
> It also appears that we can hit this in a nested context (at least in the
> case of "fast/transforms/selection-bounds-in-transformed-view.html"). This
> means we need to retain a stack of these deferred scroll operations so that
> we can unwind them properly.
Is it actually a valid state? (nested context)

[Brent Fulgham]

Created attachment 244769 [details]
Stacked Scroll State Patch

[Brent Fulgham]

I get what appears to be a nested call to the scroll logic in the test case 'fast/transforms/selection-bounds-in-transformed-view.html':

#0	0x0000000107d3726a in WTFCrash at /Volumes/Data/Projects/WebKit/OpenSource/Source/WTF/wtf/Assertions.cpp:321
#1	0x000000010c10494f in WebCore::FrameView::scrollPositionChangedViaPlatformWidget(WebCore::IntPoint const&, WebCore::IntPoint const&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:2113
#2	0x0000000112d5fcda in -[WebHTMLView(WebPrivate) _frameOrBoundsChanged] at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit/mac/WebView/WebHTMLView.mm:1323
#3	0x00007fff94edbb0c in __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ ()
#4	0x00007fff94dccfb4 in _CFXNotificationPost ()
#5	0x00007fff96532751 in -[NSNotificationCenter postNotificationName:object:userInfo:] ()
#6	0x00007fff8eae41d8 in -[NSView _postBoundsChangeNotification] ()
#7	0x00007fff8eaf629a in -[NSView translateOriginToPoint:] ()
#8	0x00007fff8eadb11a in -[NSClipView _selfBoundsChanged] ()
#9	0x00007fff8eadac8c in -[NSClipView setFrameSize:] ()
#10	0x00007fff8ea2bf03 in -[NSView setFrame:] ()
#11	0x00007fff8eada67f in -[NSScrollView _setContentViewFrame:] ()
#12	0x00007fff8ead9da7 in -[NSScrollView _applyContentAreaLayout:] ()
#13	0x00007fff8ead88ba in -[NSScrollView tile] ()
#14	0x0000000112d07159 in -[WebDynamicScrollBarsView(WebInternal) tile] at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit/mac/WebView/WebDynamicScrollBarsView.mm:238
#15	0x00007fff8ead7c7d in -[NSScrollView _tileWithoutRecursing] ()
#16	0x00007fff8ead7bfc in -[NSScrollView _update] ()
#17	0x0000000112d07d03 in -[WebDynamicScrollBarsView(WebInternal) updateScrollers] at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit/mac/WebView/WebDynamicScrollBarsView.mm:382
#18	0x0000000112d08224 in -[WebDynamicScrollBarsView(WebInternal) reflectScrolledClipView:] at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit/mac/WebView/WebDynamicScrollBarsView.mm:434
#19	0x00007fff8eaf3d5a in -[NSClipView _scrollTo:animateScroll:flashScrollerKnobs:] ()
#20	0x00007fff8eaf33c4 in -[NSClipView _reflectDocumentViewFrameChange] ()
#21	0x00007fff8ea2d661 in -[NSView _postFrameChangeNotification] ()
#22	0x00007fff8ea2cf2b in -[NSView setFrameSize:] ()
#23	0x00007fff8ea2c71a in -[NSControl setFrameSize:] ()
#24	0x000000010d2fd3c2 in WebCore::ScrollView::platformSetContentsSize() at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/platform/mac/ScrollViewMac.mm:189
#25	0x000000010d2f4966 in WebCore::ScrollView::setContentsSize(WebCore::IntSize const&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/platform/ScrollView.cpp:383
#26	0x000000010c0fb900 in WebCore::FrameView::setContentsSize(WebCore::IntSize const&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:567
#27	0x000000010c0fc9b5 in WebCore::FrameView::adjustViewSize() at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:600
#28	0x000000010c0fee8a in WebCore::FrameView::layout(bool) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:1348
#29	0x000000010cdf0429 in WebCore::Page::setPageScaleFactor(float, WebCore::IntPoint const&, bool) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/page/Page.cpp:815
#30	0x0000000112e2e784 in -[WebView(WebPrivate) _scaleWebView:atOrigin:] at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit/mac/WebView/WebView.mm:4211
#31	0x00000001070ac8d3 in resetWebViewToConsistentStateBeforeTesting() at /Volumes/Data/Projects/WebKit/OpenSource/Tools/DumpRenderTree/mac/DumpRenderTree.mm:1688
#32	0x00000001070aa606 in runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) at /Volumes/Data/Projects/WebKit/OpenSource/Tools/DumpRenderTree/mac/DumpRenderTree.mm:1929
#33	0x00000001070a8c0a in runTestingServerLoop() at /Volumes/Data/Projects/WebKit/OpenSource/Tools/DumpRenderTree/mac/DumpRenderTree.mm:1071
#34	0x00000001070a839a in dumpRenderTree(int, char const**) at /Volumes/Data/Projects/WebKit/OpenSource/Tools/DumpRenderTree/mac/DumpRenderTree.mm:1179
#35	0x00000001070aaa46 in DumpRenderTreeMain(int, char const**) at /Volumes/Data/Projects/WebKit/OpenSource/Tools/DumpRenderTree/mac/DumpRenderTree.mm:1296
#36	0x00000001070fa622 in main at /Volumes/Data/Projects/WebKit/OpenSource/Tools/DumpRenderTree/mac/DumpRenderTreeMain.mm:30
#37	0x00007fff8b3585c9 in start ()

At this point (where I put the assert) we have the following:

1. We are entering FrameView::scrollPositionChangedViaPlatformWidget with:
oldPosition = (0, 1515)
newPosition = (0, 1500)

2. We already have a deferred position change of:
oldPosition = (0, 1728)
newPosition = (0, 1515)

The stack trace doesn't show this being recursive, so I'll see if I can figure out when the first element in the stack gets created:

1. Adding the first scroll content to the stack:

Thread 1Queue : com.apple.main-thread (serial)
#0	0x000000010aa886fa in WebCore::FrameView::scrollPositionChangedViaPlatformWidget(WebCore::IntPoint const&, WebCore::IntPoint const&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:2112
#1	0x00000001116decda in -[WebHTMLView(WebPrivate) _frameOrBoundsChanged] at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit/mac/WebView/WebHTMLView.mm:1323
#2	0x00007fff94edbb0c in __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ ()
#3	0x00007fff94dccfb4 in _CFXNotificationPost ()
#4	0x00007fff96532751 in -[NSNotificationCenter postNotificationName:object:userInfo:] ()
#5	0x00007fff8eae41d8 in -[NSView _postBoundsChangeNotification] ()
#6	0x00007fff8eaf629a in -[NSView translateOriginToPoint:] ()
#7	0x00007fff8eaf4ebf in -[NSClipView _immediateScrollToPoint:] ()
#8	0x0000000111655c88 in -[WebClipView _immediateScrollToPoint:] at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit/mac/WebView/WebClipView.mm:110
#9	0x00007fff8eaf3eae in -[NSClipView scrollToPoint:] ()
#10	0x00007fff8ebd337e in -[NSScrollView scrollClipView:toPoint:] ()
#11	0x0000000111685c0e in -[WebDynamicScrollBarsView scrollClipView:toPoint:] at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit/mac/WebView/WebDynamicScrollBarsView.mm:218
#12	0x00007fff8eaf3c3d in -[NSClipView _scrollTo:animateScroll:flashScrollerKnobs:] ()
#13	0x00007fff8eaf33c4 in -[NSClipView _reflectDocumentViewFrameChange] ()
#14	0x00007fff8ea2d661 in -[NSView _postFrameChangeNotification] ()
#15	0x00007fff8ea2cf2b in -[NSView setFrameSize:] ()
#16	0x00007fff8ea2c71a in -[NSControl setFrameSize:] ()
#17	0x000000010bc813c2 in WebCore::ScrollView::platformSetContentsSize() at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/platform/mac/ScrollViewMac.mm:189
#18	0x000000010bc78966 in WebCore::ScrollView::setContentsSize(WebCore::IntSize const&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/platform/ScrollView.cpp:383
#19	0x000000010aa7f900 in WebCore::FrameView::setContentsSize(WebCore::IntSize const&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:567
#20	0x000000010aa809b5 in WebCore::FrameView::adjustViewSize() at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:600
#21	0x000000010aa82e8a in WebCore::FrameView::layout(bool) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:1348
#22	0x000000010b774429 in WebCore::Page::setPageScaleFactor(float, WebCore::IntPoint const&, bool) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/page/Page.cpp:815
#23	0x00000001117ad784 in -[WebView(WebPrivate) _scaleWebView:atOrigin:] at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit/mac/WebView/WebView.mm:4211
#24	0x0000000105a288d3 in resetWebViewToConsistentStateBeforeTesting() at /Volumes/Data/Projects/WebKit/OpenSource/Tools/DumpRenderTree/mac/DumpRenderTree.mm:1688
#25	0x0000000105a26606 in runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) at /Volumes/Data/Projects/WebKit/OpenSource/Tools/DumpRenderTree/mac/DumpRenderTree.mm:1929
#26	0x0000000105a24c0a in runTestingServerLoop() at /Volumes/Data/Projects/WebKit/OpenSource/Tools/DumpRenderTree/mac/DumpRenderTree.mm:1071
#27	0x0000000105a2439a in dumpRenderTree(int, char const**) at /Volumes/Data/Projects/WebKit/OpenSource/Tools/DumpRenderTree/mac/DumpRenderTree.mm:1179
#28	0x0000000105a26a46 in DumpRenderTreeMain(int, char const**) at /Volumes/Data/Projects/WebKit/OpenSource/Tools/DumpRenderTree/mac/DumpRenderTree.mm:1296
#29	0x0000000105a76622 in main at /Volumes/Data/Projects/WebKit/OpenSource/Tools/DumpRenderTree/mac/DumpRenderTreeMain.mm:30
#30	0x00007fff8b3585c9 in start ()

Before we return from the above stack, we hit this code again:

#0	0x000000010aa886fa in WebCore::FrameView::scrollPositionChangedViaPlatformWidget(WebCore::IntPoint const&, WebCore::IntPoint const&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:2112
#1	0x00000001116decda in -[WebHTMLView(WebPrivate) _frameOrBoundsChanged] at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit/mac/WebView/WebHTMLView.mm:1323
#2	0x00007fff94edbb0c in __CFNOTIFICATIONCENTER_IS_CALLING_OUT_TO_AN_OBSERVER__ ()
#3	0x00007fff94dccfb4 in _CFXNotificationPost ()
#4	0x00007fff96532751 in -[NSNotificationCenter postNotificationName:object:userInfo:] ()
#5	0x00007fff8eae41d8 in -[NSView _postBoundsChangeNotification] ()
#6	0x00007fff8eaf629a in -[NSView translateOriginToPoint:] ()
#7	0x00007fff8eadb11a in -[NSClipView _selfBoundsChanged] ()
#8	0x00007fff8eadac8c in -[NSClipView setFrameSize:] ()
#9	0x00007fff8ea2bf03 in -[NSView setFrame:] ()
#10	0x00007fff8eada67f in -[NSScrollView _setContentViewFrame:] ()
#11	0x00007fff8ead9da7 in -[NSScrollView _applyContentAreaLayout:] ()
#12	0x00007fff8ead88ba in -[NSScrollView tile] ()
#13	0x0000000111686159 in -[WebDynamicScrollBarsView(WebInternal) tile] at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit/mac/WebView/WebDynamicScrollBarsView.mm:238
#14	0x00007fff8ead7c7d in -[NSScrollView _tileWithoutRecursing] ()
#15	0x00007fff8ead7bfc in -[NSScrollView _update] ()
#16	0x0000000111686d03 in -[WebDynamicScrollBarsView(WebInternal) updateScrollers] at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit/mac/WebView/WebDynamicScrollBarsView.mm:382
#17	0x0000000111687224 in -[WebDynamicScrollBarsView(WebInternal) reflectScrolledClipView:] at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit/mac/WebView/WebDynamicScrollBarsView.mm:434
#18	0x00007fff8eaf3d5a in -[NSClipView _scrollTo:animateScroll:flashScrollerKnobs:] ()
#19	0x00007fff8eaf33c4 in -[NSClipView _reflectDocumentViewFrameChange] ()
#20	0x00007fff8ea2d661 in -[NSView _postFrameChangeNotification] ()
#21	0x00007fff8ea2cf2b in -[NSView setFrameSize:] ()
#22	0x00007fff8ea2c71a in -[NSControl setFrameSize:] ()
#23	0x000000010bc813c2 in WebCore::ScrollView::platformSetContentsSize() at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/platform/mac/ScrollViewMac.mm:189
#24	0x000000010bc78966 in WebCore::ScrollView::setContentsSize(WebCore::IntSize const&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/platform/ScrollView.cpp:383
#25	0x000000010aa7f900 in WebCore::FrameView::setContentsSize(WebCore::IntSize const&) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:567
#26	0x000000010aa809b5 in WebCore::FrameView::adjustViewSize() at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:600
#27	0x000000010aa82e8a in WebCore::FrameView::layout(bool) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/page/FrameView.cpp:1348
#28	0x000000010b774429 in WebCore::Page::setPageScaleFactor(float, WebCore::IntPoint const&, bool) at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebCore/page/Page.cpp:815
#29	0x00000001117ad784 in -[WebView(WebPrivate) _scaleWebView:atOrigin:] at /Volumes/Data/Projects/WebKit/OpenSource/Source/WebKit/mac/WebView/WebView.mm:4211
#30	0x0000000105a288d3 in resetWebViewToConsistentStateBeforeTesting() at /Volumes/Data/Projects/WebKit/OpenSource/Tools/DumpRenderTree/mac/DumpRenderTree.mm:1688
#31	0x0000000105a26606 in runTest(std::__1::basic_string<char, std::__1::char_traits<char>, std::__1::allocator<char> > const&) at /Volumes/Data/Projects/WebKit/OpenSource/Tools/DumpRenderTree/mac/DumpRenderTree.mm:1929
#32	0x0000000105a24c0a in runTestingServerLoop() at /Volumes/Data/Projects/WebKit/OpenSource/Tools/DumpRenderTree/mac/DumpRenderTree.mm:1071
#33	0x0000000105a2439a in dumpRenderTree(int, char const**) at /Volumes/Data/Projects/WebKit/OpenSource/Tools/DumpRenderTree/mac/DumpRenderTree.mm:1179
#34	0x0000000105a26a46 in DumpRenderTreeMain(int, char const**) at /Volumes/Data/Projects/WebKit/OpenSource/Tools/DumpRenderTree/mac/DumpRenderTree.mm:1296
#35	0x0000000105a76622 in main at /Volumes/Data/Projects/WebKit/OpenSource/Tools/DumpRenderTree/mac/DumpRenderTreeMain.mm:30
#36	0x00007fff8b3585c9 in start ()

So in this case, maybe we don't want to keep track of both of these. Maybe AppKit did some calculations and decided to modify the scroll position we should to be using?

[Brent Fulgham]

So the asserts I added indicate that we never hit a nested case where we have a ScrollableArea being adjusted during layout AND a PlatformWidget being adjusted.

So the stack implementation is probably not needed.

[Brent Fulgham]

Created attachment 244790 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 244790 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244790&action=review

> Source/WebCore/ChangeLog:10
> +        Defer updating scrollbars until we have finished layout. This

It's more than juste updating scrollbars.

> Source/WebCore/ChangeLog:20
> +        Modify FrameView to set its ScrollView state to block drawing
> +        scrollbar updates during layout. Also add a post-layout

"drawing scrollbar updates" isn't really accurate.

> Source/WebCore/ChangeLog:24
> +        Store the deferred scroll drawing changes in a stack so that we properly

No stack any more.

> Source/WebCore/page/FrameView.cpp:2112
> +    // is not complete. Instead, defer the scroll event until the layout finishes.

"scroll event" is ambiguous here. This didn't change anything about the way that scroll events are propagated to content (or maybe it does?)

> Source/WebCore/page/FrameView.cpp:2115
> +        m_deferredScrollPosition = std::make_unique<std::pair<IntPoint, IntPoint>>(std::make_pair(oldPosition, newPosition));

This oldPosition stuff is only used by sendWillRevealEdgeEventsIfNeeded() so I wonder if we can remove that.

> Source/WebCore/platform/ScrollView.h:456
> +    IntSize m_deferredScrollDelta;

I really don't like this tracking of state in both FrameView and ScrollView. r- for that reason.

[Brent Fulgham]

Created attachment 244812 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 244812 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244812&action=review

> Source/WebCore/platform/ScrollView.cpp:487
> +    if (deferredDelta.isZero())
> +        scrollPositionChangedViaPlatformWidgetImpl(std::get<1>(*m_deferredScrollDeltaOrPositions), std::get<2>(*m_deferredScrollDeltaOrPositions));
> +    else {
> +        updateLayerPositionsAfterScrolling();
> +        scrollContents(deferredDelta);
> +        updateCompositingLayersAfterScrolling();
> +    }

It's so hard to justify this "if this thing is zero, do platform widget stuff, otherwise do this other random set of things". Does calling scrollPositionChangedViaPlatformWidgetImpl() have the side effect of doing these things too? Does it do anything else interesting?

[Brent Fulgham]

Comment on attachment 244812 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244812&action=review

>> Source/WebCore/platform/ScrollView.cpp:487
>> +    }
> 
> It's so hard to justify this "if this thing is zero, do platform widget stuff, otherwise do this other random set of things". Does calling scrollPositionChangedViaPlatformWidgetImpl() have the side effect of doing these things too? Does it do anything else interesting?

The m_deferredScrollDeltaOrPositions unique_ptr only exists if we have deferred either platform widget stuff, or our scroll view stuff. If we deferred our scroll view stuff, we have a deferredDelta that fails the "isZero" predicate. So if deferredDelta passes 'isZero' we call the platformWidget stuff.

I guess I could wrap the three lines of our scrolling stuff from the 'ScrollView::scrollTo' in a method and call it here if that would make things any clearer.

[Brent Fulgham]

Created attachment 244822 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 244822 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=244822&action=review

I don't like this but let's get it in to unblock windows.

> Source/WebCore/platform/ScrollView.h:463
> +    std::unique_ptr<IntSize> m_deferredScrollDelta;
> +    std::unique_ptr<std::pair<IntPoint, IntPoint>> m_deferredScrollPositions;

Please add comments saying why you need both (e.g. one is used when there are platform widgets).

[Brent Fulgham]

Committed r178661: <http://trac.webkit.org/changeset/178661>

[Brent Fulgham]

This work really shows how complicated ScrollView has become. We should refactor this stuff (see Bug 140633)