Bug 108255

Summary: Scrollbar and scroll corner composited layers positioned incorrectly
Product: WebKit Reporter: James Robinson <jamesr>
Component: New BugsAssignee: James Robinson <jamesr>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, sam, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch none

Description James Robinson 2013-01-29 16:50:51 PST
Originally reported as https://code.google.com/p/chromium/issues/detail?id=170264

Open this repro case in Chrome with --force-compositing-mode or a recent WebKit nightlies:

<!DOCTYPE html>
<html>
 <body>
  <div style="position:relative; float:right; width:40px; height:20px; right:-20px; background-color:#090"></div>
  <div style="height:400px; background-color:#900;"></div>
 </body>
</html>


data:text/html;charset=utf-8,<!DOCTYPE%20html>%0A<html>%0A%20<body>%0A%20%20<div%20style%3D"position%3Arelative%3B%20float%3Aright%3B%20width%3A40px%3B%20height%3A20px%3B%20right%3A-20px%3B%20background-color%3A%23090"><%2Fdiv>%0A%20%20<div%20style%3D"height%3A400px%3B%20background-color%3A%23900%3B"><%2Fdiv>%0A%20<%2Fbody>%0A<%2Fhtml>

and resize the window to be close to the size of the red square.  At certain sizes, the page goes nearly white.  The scroll corner layer on the root frame is positioned at 15x15 and occupies the rest of the frame.  Somehow the overflow control layer positioning logic is running before the scrollbars get correctly sized.
Comment 1 James Robinson 2013-01-29 17:07:48 PST
When we end up doing multiple passes through ScrollView::updateScrollbars(), we sometimes end up not calling positionScrollbarLayers() at all since it's inside this check:

http://code.google.com/searchframe#OAMlx_jo-ck/src/third_party/WebKit/Source/WebCore/platform/ScrollView.cpp&exact_package=chromium&q=ScrollView.cpp&type=cs&l=608

Just unconditionally calling positionScrollbarLayers() fixes this, but that's probably not ideal.

+cc a few other ScrollView folks
Comment 2 James Robinson 2013-01-29 18:49:23 PST
We end up recursively calling updateScrollbars repeatedly on this page up to cMaxUpdateScrollbarPass.  On the first call, the ScrollView has horizontal and vertical scrollbars.  Layout results in no overflow, so updateScrollbars is called to remove them.  After a few rounds of this, we end up calling positionScrollbarLayers() with this callstack:

#0  WebCore::ScrollView::positionScrollbarLayers (this=0x79ac6800) at Source/WebCore/platform/ScrollView.cpp:940
#1  0x0c89576a in WebCore::RenderLayerCompositor::updateOverflowControlsLayers (this=0x79471db0) at Source/WebCore/rendering/RenderLayerCompositor.cpp:2411
#2  0x0c894614 in WebCore::RenderLayerCompositor::frameViewDidChangeSize (this=0x79471db0) at Source/WebCore/rendering/RenderLayerCompositor.cpp:1175
#3  0x0d7dbb2f in WebCore::FrameView::visibleContentsResized (this=0x79ac6800) at Source/WebCore/page/FrameView.cpp:2009
#4  0x0aca7354 in WebCore::ScrollView::updateScrollbars (this=0x79ac6800, desiredOffset=@0xbfff0a70) at Source/WebCore/platform/ScrollView.cpp:474
#5  0x0aca9670 in WebCore::ScrollView::setContentsSize (this=0x79ac6800, newSize=@0xbfff0b40) at Source/WebCore/platform/ScrollView.cpp:308
#6  0x0d7d2734 in WebCore::FrameView::setContentsSize (this=0x79ac6800, size=@0xbfff0b40) at Source/WebCore/page/FrameView.cpp:557
#7  0x0d7d2a01 in WebCore::FrameView::adjustViewSize (this=0x79ac6800) at Source/WebCore/page/FrameView.cpp:586
#8  0x0d7d5a42 in WebCore::FrameView::layout (this=0x79ac6800, allowSubtree=true) at Source/WebCore/page/FrameView.cpp:1231
#9  0x0d7dbae9 in WebCore::FrameView::visibleContentsResized (this=0x79ac6800) at Source/WebCore/page/FrameView.cpp:2004
#10 0x0aca7aac in WebCore::ScrollView::updateScrollbars (this=0x79ac6800, desiredOffset=@0xbfff1030) at Source/WebCore/platform/ScrollView.cpp:547
#11 0x0aca9670 in WebCore::ScrollView::setContentsSize (this=0x79ac6800, newSize=@0xbfff1100) at Source/WebCore/platform/ScrollView.cpp:308
#12 0x0d7d2734 in WebCore::FrameView::setContentsSize (this=0x79ac6800, size=@0xbfff1100) at Source/WebCore/page/FrameView.cpp:557
#13 0x0d7d2a01 in WebCore::FrameView::adjustViewSize (this=0x79ac6800) at Source/WebCore/page/FrameView.cpp:586
#14 0x0d7d5a42 in WebCore::FrameView::layout (this=0x79ac6800, allowSubtree=true) at Source/WebCore/page/FrameView.cpp:1231
#15 0x0d7dbae9 in WebCore::FrameView::visibleContentsResized (this=0x79ac6800) at Source/WebCore/page/FrameView.cpp:2004
#16 0x0aca7aac in WebCore::ScrollView::updateScrollbars (this=0x79ac6800, desiredOffset=@0xbfff15f0) at Source/WebCore/platform/ScrollView.cpp:547
#17 0x0aca9670 in WebCore::ScrollView::setContentsSize (this=0x79ac6800, newSize=@0xbfff16c0) at Source/WebCore/platform/ScrollView.cpp:308
#18 0x0d7d2734 in WebCore::FrameView::setContentsSize (this=0x79ac6800, size=@0xbfff16c0) at Source/WebCore/page/FrameView.cpp:557
#19 0x0d7d2a01 in WebCore::FrameView::adjustViewSize (this=0x79ac6800) at Source/WebCore/page/FrameView.cpp:586
#20 0x0d7d5a42 in WebCore::FrameView::layout (this=0x79ac6800, allowSubtree=true) at Source/WebCore/page/FrameView.cpp:1231
#21 0x0d7e0d5b in WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive (this=0x79ac6800) at Source/WebCore/page/FrameView.cpp:3418

where we get a non-sensical scrollCornerRect():

  m_location = {
    m_x = 15, 
    m_y = 15
  }, 
  m_size = {
    m_width = 769, 
    m_height = 413
  }

since the scrollbars are inconsistent.  The scrollbars get fixed up as the stack unwinds, but since the ScrollView ends up with both vertical and horizontal scrollbars (just like it started with), it doesn't update the layer geometry again:

    if (hasHorizontalScrollbar != (m_horizontalScrollbar != 0) || hasVerticalScrollbar != (m_verticalScrollbar != 0)) {
        // FIXME: Is frameRectsChanged really necessary here? Have any frame rects changed?
        frameRectsChanged();
        positionScrollbarLayers();
        updateScrollCorner();
Comment 3 James Robinson 2013-01-29 21:14:16 PST
Created attachment 185387 [details]
Patch
Comment 4 James Robinson 2013-01-29 21:15:12 PST
I've had no luck creating a layout test.  The problem only seems to manifest on a Widget resize and I can't make it happen inside an iframe.

I am not 100% convinced this is the right patch but it fixes this bug and does not regress any other layout tests, so it's something for consideration.
Comment 5 James Robinson 2013-01-29 21:45:58 PST
Comment on attachment 185387 [details]
Patch

After staring at it a bit more I'm pretty sure this is correct.  This should only cause us to do the update in more cases than we currently do, never less, and repositioning/etc more often should never produce incorrect behavior.
Comment 6 WebKit Review Bot 2013-01-29 22:33:15 PST
Comment on attachment 185387 [details]
Patch

Clearing flags on attachment: 185387

Committed r141226: <http://trac.webkit.org/changeset/141226>
Comment 7 WebKit Review Bot 2013-01-29 22:33:18 PST
All reviewed patches have been landed.  Closing bug.