108255 2013-01-29 16:50:51 -0800 Scrollbar and scroll corner composited layers positioned incorrectly 2013-01-29 22:33:18 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 jamesr jamesr bdakin sam simon.fraser webkit.review.bot oldest_to_newest 819499 0 jamesr 2013-01-29 16:50:51 -0800 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. 819531 1 jamesr 2013-01-29 17:07:48 -0800 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 819677 2 jamesr 2013-01-29 18:49:23 -0800 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(); 819755 3 185387 jamesr 2013-01-29 21:14:16 -0800 Created attachment 185387 Patch 819757 4 jamesr 2013-01-29 21:15:12 -0800 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. 819782 5 185387 jamesr 2013-01-29 21:45:58 -0800 Comment on attachment 185387 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. 819808 6 185387 webkit.review.bot 2013-01-29 22:33:15 -0800 Comment on attachment 185387 Patch Clearing flags on attachment: 185387 Committed r141226: <http://trac.webkit.org/changeset/141226> 819809 7 webkit.review.bot 2013-01-29 22:33:18 -0800 All reviewed patches have been landed. Closing bug. 185387 2013-01-29 21:14:16 -0800 2013-01-29 22:33:14 -0800 Patch bug-108255-20130129211107.patch text/plain 2428 jamesr U3VidmVyc2lvbiBSZXZpc2lvbjogMTQwNTQ3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNDUyYTIzNWYwOTA1NDI2 NzllMDEwNGIzNTAxZDhiMDM0NmJiZWE4MC4uNjg3NmQ0NTlkZmQxOTY3MzJiMDZjMzA2NGIwODY2 ZGNiYTk2Mzc2YyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIzIEBACisyMDEzLTAxLTI5ICBKYW1l cyBSb2JpbnNvbiAgPGphbWVzckBjaHJvbWl1bS5vcmc+CisKKyAgICAgICAgU2Nyb2xsYmFyIGFu ZCBzY3JvbGwgY29ybmVyIGNvbXBvc2l0ZWQgbGF5ZXJzIHBvc2l0aW9uZWQgaW5jb3JyZWN0bHkK KyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEwODI1NQor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFNjcm9sbFZp ZXc6OnVwZGF0ZVNjcm9sbGJhcnMoKSBuZWVkcyB0byB1cGRhdGUgdGhlIG92ZXJmbG93IGNvbnRy b2xzIGNvbXBvc2l0ZWQgbGF5ZXJzIGlmIHNjcm9sbGJhcnMgYXJlIGFkZGVkCisgICAgICAgIG9y IHJlbW92ZWQuIEl0IHdhcyBkb2luZyB0aGlzIGJ5IHJlY29yZGluZyBvbiBlbnRyeSB0byB0aGUg ZnVuY3Rpb24gaWYgaXQgaGFkIGhvcml6b250YWwgb3IgdmVydGljYWwgc2Nyb2xsYmFycworICAg ICAgICBhbmQgdGhlbiBjb21wYXJpbmcgdGhhdCB0byBtX2hvcml6b250YWwvdmVydGljYWxTY3Jv bGxiYXIgb24gZXhpdC4gVW5mb3J0dW5hdGVseSB1cGRhdGVTY3JvbGxiYXJzIGlzIHJlY3Vyc2l2 ZQorICAgICAgICBhbmQgZXhpdHMgd2l0aG91dCBydW5uaW5nIHRoZSBwb3N0YW1ibGUgY29kZSB3 aGVuIG5lc3RlZCBvbiB0aGUgY2FsbHN0YWNrLiBBcyBhIHJlc3VsdCwgc2Nyb2xsYmFycyBtYXkg YmUKKyAgICAgICAgYWRkZWQgb3IgcmVtb3ZlZCBzZXZlcmFsIHRpbWVzIGR1cmluZyB0aGUgcmVj dXJzaW9uLCBwb3NzaWJseSBsZWF2aW5nIHRoZSBvdmVyZmxvdyBjb250cm9sIGxheWVycyBpbiBh bgorICAgICAgICBpbmNvbnNpc3RlbnQgc3RhdGUsIHdoaWxlIGVuZGluZyB1cCB3aXRoIHRoZSBz YW1lIHNldCBvZiBzY3JvbGxiYXJzLgorCisgICAgICAgIFRoaXMgY2hhbmdlcyB0aGUgImhhcyBh bnl0aGluZyBjaGFuZ2VkIiBsb2dpYyB0byBvbmx5IGNvbXBhcmUgbG9jYWwgc3RhdGUgKGhhc1hY WFNjcm9sbGJhciB2cworICAgICAgICBuZXdIYXNYWFhTY3JvbGxiYXIpIHNvIGNoYW5nZXMgaW4g cmVjdXJzaXZlIGNhbGxzIGFyZSBub3QgY29uc2lkZXJlZC4KKworICAgICAgICAqIHBsYXRmb3Jt L1Njcm9sbFZpZXcuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U2Nyb2xsVmlldzo6dXBkYXRlU2Ny b2xsYmFycyk6CisKIDIwMTMtMDEtMjMgIE5vYW0gUm9zZW50aGFsICA8bm9hbUB3ZWJraXQub3Jn PgogCiAgICAgICAgIFJFR1JFU1NJT04ocjE0MDUxOCk6IEJyb2tlIENocm9taXVtIFdpbiBidWls ZCAoZ3lwIGZpbGUgbm90IHVwZGF0ZWQpIChSZXF1ZXN0ZWQgYnkgYXJ2IG9uICN3ZWJraXQpLgpk aWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vU2Nyb2xsVmlldy5jcHAgYi9Tb3Vy Y2UvV2ViQ29yZS9wbGF0Zm9ybS9TY3JvbGxWaWV3LmNwcAppbmRleCAyMTFjOWE3MTQ3NjlhZThh YmY2ZjI5NWIzMzc1YjgwODAzNGU2Y2I4Li43NGQ0ODBmZjE4NmU1YzhkYTBhZmMyZmNkM2ZhZDI2 MzExYTEyODllIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9TY3JvbGxWaWV3 LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9TY3JvbGxWaWV3LmNwcApAQCAtNjAy LDcgKzYwMiw3IEBAIHZvaWQgU2Nyb2xsVmlldzo6dXBkYXRlU2Nyb2xsYmFycyhjb25zdCBJbnRT aXplJiBkZXNpcmVkT2Zmc2V0KQogICAgICAgICAgICAgbV92ZXJ0aWNhbFNjcm9sbGJhci0+c2V0 U3VwcHJlc3NJbnZhbGlkYXRpb24oZmFsc2UpOwogICAgIH0KIAotICAgIGlmIChoYXNIb3Jpem9u dGFsU2Nyb2xsYmFyICE9IChtX2hvcml6b250YWxTY3JvbGxiYXIgIT0gMCkgfHwgaGFzVmVydGlj YWxTY3JvbGxiYXIgIT0gKG1fdmVydGljYWxTY3JvbGxiYXIgIT0gMCkpIHsKKyAgICBpZiAoaGFz SG9yaXpvbnRhbFNjcm9sbGJhciAhPSBuZXdIYXNIb3Jpem9udGFsU2Nyb2xsYmFyIHx8IGhhc1Zl cnRpY2FsU2Nyb2xsYmFyICE9IG5ld0hhc1ZlcnRpY2FsU2Nyb2xsYmFyKSB7CiAgICAgICAgIC8v IEZJWE1FOiBJcyBmcmFtZVJlY3RzQ2hhbmdlZCByZWFsbHkgbmVjZXNzYXJ5IGhlcmU/IEhhdmUg YW55IGZyYW1lIHJlY3RzIGNoYW5nZWQ/CiAgICAgICAgIGZyYW1lUmVjdHNDaGFuZ2VkKCk7CiAg ICAgICAgIHBvc2l0aW9uU2Nyb2xsYmFyTGF5ZXJzKCk7Cg==