73348 2011-11-29 12:34:07 -0800 REGRESSION: Assertion in RenderBox::mapAbsoluteToLocalPoint when loading a page with a scrollable RenderLayer 2011-12-08 11:45:06 -0800 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) Mac OS X 10.7 RESOLVED FIXED InRadar, Regression P2 Normal --- 1 bdakin bdakin aroben bdakin jchaffraix mitz sam simon.fraser thorton tony webkit-bug-importer xji oldest_to_newest 511004 0 bdakin 2011-11-29 12:34:07 -0800 This assertion started filing after http://trac.webkit.org/changeset/100819 which was for https://bugs.webkit.org/show_bug.cgi?id=70395 To reproduce: Using a Debug build, load a web page with a scrollable RenderLayer such as the attached test case. As far as I can tell, there is not a problem in release builds, and for some reason, this assertion does not fire when running the layout tests (though it does fire with many layout tests when they are loaded manually in the browser). ASSERTION FAILED: !view() || !view()->layoutStateEnabled() /Volumes/Big/Source/Labyrinth/OpenSource/Source/WebCore/rendering/RenderBox.cpp(1386) : virtual void WebCore::RenderBox::mapAbsoluteToLocalPoint(bool, bool, WebCore::TransformState &) const 1 0x10933cfc4 WebCore::RenderBox::mapAbsoluteToLocalPoint(bool, bool, WebCore::TransformState&) const 2 0x10940d005 WebCore::RenderObject::absoluteToLocal(WebCore::FloatPoint const&, bool, bool) const 3 0x10890e4b3 WebCore::FrameView::convertToRenderer(WebCore::RenderObject const*, WebCore::IntPoint const&) const 4 0x1093b887e WebCore::RenderLayer::convertFromContainingViewToScrollbar(WebCore::Scrollbar const*, WebCore::IntPoint const&) const 5 0x10958e548 WebCore::Scrollbar::convertFromContainingView(WebCore::IntPoint const&) const 6 0x109585118 -[WebScrollbarPainterControllerDelegate scrollerImpPair:convertContentPoint:toScrollerImp:] 7 0x7fff8e053b78 -[NSScrollerImpPair _updateOverlayScrollersStateWithReason:forceAtLeastKnobsVisible:] 8 0x109588fa1 WebCore::ScrollAnimatorMac::notifyPositionChanged() 9 0x109588a8a WebCore::ScrollAnimatorMac::immediateScrollToPoint(WebCore::FloatPoint const&) 10 0x109588a23 WebCore::ScrollAnimatorMac::scrollToOffsetWithoutAnimation(WebCore::FloatPoint const&) 11 0x10958280c WebCore::ScrollableArea::scrollToOffsetWithoutAnimation(WebCore::FloatPoint const&) 12 0x1093b52c3 WebCore::RenderLayer::scrollToOffset(int, int, WebCore::RenderLayer::ScrollOffsetClamping) 13 0x1093ba639 WebCore::RenderLayer::updateScrollInfoAfterLayout() 14 0x1092d4321 WebCore::RenderBlock::updateScrollInfoAfterLayout() 15 0x1092d50d0 WebCore::RenderBlock::layoutBlock(bool, int, WebCore::RenderBlock::BlockLayoutPass) 16 0x1092d4377 WebCore::RenderBlock::layout() 17 0x1092dd4ff WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, int&, int&) 18 0x1092d72d3 WebCore::RenderBlock::layoutBlockChildren(bool, int&) 19 0x1092d4cb7 WebCore::RenderBlock::layoutBlock(bool, int, WebCore::RenderBlock::BlockLayoutPass) 20 0x1092d4377 WebCore::RenderBlock::layout() 21 0x1092dd4ff WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, int&, int&) 22 0x1092d72d3 WebCore::RenderBlock::layoutBlockChildren(bool, int&) 23 0x1092d4cb7 WebCore::RenderBlock::layoutBlock(bool, int, WebCore::RenderBlock::BlockLayoutPass) 24 0x1092d4377 WebCore::RenderBlock::layout() 25 0x1092dd4ff WebCore::RenderBlock::layoutBlockChild(WebCore::RenderBox*, WebCore::RenderBlock::MarginInfo&, int&, int&) 26 0x1092d72d3 WebCore::RenderBlock::layoutBlockChildren(bool, int&) 27 0x1092d4cb7 WebCore::RenderBlock::layoutBlock(bool, int, WebCore::RenderBlock::BlockLayoutPass) 28 0x1092d4377 WebCore::RenderBlock::layout() 29 0x1094f2715 WebCore::RenderView::layout() 30 0x108906095 WebCore::FrameView::layout(bool) 31 0x10865a27b WebCore::Document::implicitClose() 511005 1 117023 bdakin 2011-11-29 12:34:41 -0800 Created attachment 117023 Test that asserts 511013 2 bdakin 2011-11-29 12:39:36 -0800 I looked at this briefly in the debugger, and immediateScrollToPoint() is called with the point (0,0). It doesn't make sense to call notifyPositionChanged() in that case because, since the page it just loading and laying out for the first time, the position has not changed. 511054 3 xji 2011-11-29 13:23:51 -0800 The reason to trigger notifyChange is explained at: https://bugs.webkit.org/show_bug.cgi?id=70395#c2 We probably need a better way to distinguish the situations. Sam is probably the person knowing the code the best. 513014 4 xji 2011-12-01 12:55:52 -0800 Is the assertion triggered by just loading the attached test case in a debug build? I tried it, but it does not tigger assertion, neither when I scroll/resize the page. 513090 5 bdakin 2011-12-01 14:11:23 -0800 The assertion will only happen on Mac OS X Lion with overlay scrollbars since it involves the new overlay scrollbar code. Are you running Lion and using overlay scrollbars? In that configuration, for me, the assertion fires on load. 513137 6 xji 2011-12-01 14:42:52 -0800 No wonder it does not crash for me in Mac and Chromium. I am running Snowleopard. 513870 7 aroben 2011-12-02 09:57:12 -0800 Is this a dupe of bug 69187? I'm seeing this all the time. 513919 8 bdakin 2011-12-02 10:47:13 -0800 (In reply to comment #7) > Is this a dupe of bug 69187? > > I'm seeing this all the time. It certainly seems so! I guess this was a crash we always had, but http://trac.webkit.org/changeset/100819 definitely seems to make it happen much more frequently. 513941 9 simon.fraser 2011-12-02 11:08:09 -0800 <rdar://problem/10518918> 513946 10 simon.fraser 2011-12-02 11:09:04 -0800 *** Bug 69187 has been marked as a duplicate of this bug. *** 513947 11 simon.fraser 2011-12-02 11:10:28 -0800 It's wrong to be calling Scrollbar::convertFromContainingView() in the middle of layout, since coordinates are in flux. Maybe we should postpone calls to RenderBlock::updateScrollInfoAfterLayout() until after layout is complete. 514298 12 bdakin 2011-12-02 17:35:02 -0800 *** Bug 73723 has been marked as a duplicate of this bug. *** 515392 13 bdakin 2011-12-05 14:19:59 -0800 (In reply to comment #11) > It's wrong to be calling Scrollbar::convertFromContainingView() in the middle of layout, since coordinates are in flux. Maybe we should postpone calls to RenderBlock::updateScrollInfoAfterLayout() until after layout is complete. If you look at that stack trace, you can see that this work is happening as a result of code in updateScrollInfoAfterLayout(). the layout state doesn't seem to be popped until after that. 515395 14 bdakin 2011-12-05 14:23:07 -0800 (In reply to comment #13) > (In reply to comment #11) > > It's wrong to be calling Scrollbar::convertFromContainingView() in the middle of layout, since coordinates are in flux. Maybe we should postpone calls to RenderBlock::updateScrollInfoAfterLayout() until after layout is complete. > > If you look at that stack trace, you can see that this work is happening as a result of code in updateScrollInfoAfterLayout(). the layout state doesn't seem to be popped until after that. Oh, I just misunderstood you. I see you are saying that updateScrollInfoAfterLayout() itself should maybe be postponed. 517046 15 118296 bdakin 2011-12-07 18:00:54 -0800 Created attachment 118296 Patch 517621 16 bdakin 2011-12-08 11:45:06 -0800 Fixed with http://trac.webkit.org/changeset/102355 Filed https://bugs.webkit.org/show_bug.cgi?id=74111 to cover the not-regularly-reproducible occurrence of this assertion. 117023 2011-11-29 12:34:41 -0800 2011-11-29 12:34:41 -0800 Test that asserts scrolly.html text/html 1185 bdakin PGhlYWQ+CjxzY3JpcHQ+CgpmdW5jdGlvbiBtZXRyaWNzKCkKewogICAgdmFyIG91dGVyID0gZG9j dW1lbnQuZ2V0RWxlbWVudEJ5SWQoIm91dGVyIik7CiAgICB2YXIgaW5uZXIgPSBkb2N1bWVudC5n ZXRFbGVtZW50QnlJZCgiaW5uZXIiKTsKICAgIC8vYWxlcnQoIk91dGVyIHNjcm9sbCB3aWR0aDog IiArIG91dGVyLnNjcm9sbFdpZHRoKTsKICAgIC8vYWxlcnQoIklubmVyIHNjcm9sbCB3aWR0aDog IiArIGlubmVyLnNjcm9sbFdpZHRoKTsKICAgIC8vYWxlcnQoIk91dGVyIGNsaWVudCB3aWR0aDog IiArIG91dGVyLmNsaWVudFdpZHRoKTsKICAgIC8vYWxlcnQoIklubmVyIGNsaWVudCB3aWR0aDog IiArIGlubmVyLmNsaWVudFdpZHRoKTsKICAgIC8vYWxlcnQoIk91dGVyIG9mZnNldCB3aWR0aDog IiArIG91dGVyLm9mZnNldFdpZHRoKTsKICAgIC8vYWxlcnQoIklubmVyIG9mZnNldCB3aWR0aDog IiArIGlubmVyLm9mZnNldFdpZHRoKTsKfQoKPC9zY3JpcHQ+CjwvaGVhZD4KCjxib2R5IG9ubG9h ZD0ibWV0cmljcygpOyI+Cgo8ZGl2IGlkPSJvdXRlciIgc3R5bGU9IndpZHRoOjEwMHB4OyBoZWln aHQ6MTAwcHg7IG92ZXJmbG93Om92ZXJsYXk7IGJvcmRlcjoycHggc29saWQgcHVycGxlOyByZXNp emU6Ym90aDsgLXdlYmtpdC10cmFuc2Zvcm06dHJhbnNsYXRlWCg1MHB4KTsiPgo8ZGl2IGlkPSJp bm5lciIgc3R5bGU9ICJ3aWR0aDoyMDBweCI+CllvdSBzYXkgaXQncyB5b3VyIEJpcnRoZGF5LiBJ dCdzIG15IEJpcnRoZGF5IHRvbyEgWW91IHNheSBpdCdzIHlvdXIgQmlydGhkYXkuIEl0J3MgbXkg QmlydGhkYXkgdG9vISBZb3Ugc2F5IGl0J3MgeW91ciBCaXJ0aGRheS4gSXQncyBteSBCaXJ0aGRh eSB0b28hIFlvdSBzYXkgaXQncyB5b3VyIEJpcnRoZGF5LiBJdCdzIG15IEJpcnRoZGF5IHRvbyEg WW91IHNheSBpdCdzIHlvdXIgQmlydGhkYXkuIEl0J3MgbXkgQmlydGhkYXkgdG9vISBZb3Ugc2F5 IGl0J3MgeW91ciBCaXJ0aGRheS4gSXQncyBteSBCaXJ0aGRheSB0b28hIFlvdSBzYXkgaXQncyB5 b3VyIEJpcnRoZGF5LiBJdCdzIG15IEJpcnRoZGF5IHRvbyEgWW91IHNheSBpdCdzIHlvdXIgQmly dGhkYXkuIEl0J3MgbXkgQmlydGhkYXkgdG9vISBZb3Ugc2F5IGl0J3MgeW91ciBCaXJ0aGRheS4g SXQncyBteSBCaXJ0aGRheSB0b28hCjwvZGl2Pgo8L2Rpdj4KCjwvYm9keT4K 118296 2011-12-07 18:00:54 -0800 2011-12-07 19:05:42 -0800 Patch for-review.txt text/plain 3562 bdakin SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEwMjI5MCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDM2IEBACisyMDExLTEyLTA3ICBCZXRoIERh a2luICA8YmRha2luQGFwcGxlLmNvbT4KKworICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9NzMzNDgKKyAgICAgICAgUkVHUkVTU0lPTjogQXNzZXJ0aW9uIHdo ZW4gbG9hZGluZyBhIHBhZ2Ugd2l0aCBhIHNjcm9sbGFibGUgUmVuZGVyTGF5ZXIgCisgICAgICAg IC1hbmQgY29ycmVzcG9uZGluZy0KKyAgICAgICAgPHJkYXI6Ly9wcm9ibGVtLzEwNTE4OTE4Pgor CisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRoZSBtYWlu IHByb2JsZW0gaGVyZSBpcyB0aGF0IGNlcnRhaW4gZGVsZWdhdGUgY2FsbHMgaW50byBBcHBLaXQg Zm9yIG92ZXJsYXkgCisgICAgICAgIHNjcm9sbGJhcnMgY2FuIGNhdXNlIEFwcEtpdCB0byBjYWxs IGJhY2sgaW50byBXZWJLaXQgbG9va2luZyBmb3IgbW9yZSBpbmZvcm1hdGlvbi4gCisgICAgICAg IFRoZSBhc3NlcnRpb24gaGFwcGVucyB3aGVuIFdlYktpdCB0ZWxscyBBcHBLaXQgdGhhdCB0aGUg c2Nyb2xsIHBvc2l0aW9uIGhhcyAKKyAgICAgICAgY2hhbmdlZCBkdXJpbmcgYSBsYXlvdXQsIGFu ZCBBcHBLaXQgaW1tZWRpYXRlbHkgYXNrcyBXZWJLaXQgdG8gY29udmVydCBzb21lIAorICAgICAg ICBjb29yZGluYXRlcywgYW5kIFdlYktpdCBhc3NlcnRzIHRoYXQgeW91IHNob3VsZG4ndCBkbyB0 aGF0IHdoaWxlIGEgbGF5b3V0IGlzIHN0aWxsIAorICAgICAgICBoYXBwZW5pbmcuIEl0J3Mgc3Rp bGwgcG9zc2libGUgZm9yIEFwcEtpdCB0byBjYWxsIHRoaXMgZGVsZWdhdGUgbWV0aG9kIHdoaWxl IGEgCisgICAgICAgIGxheW91dCBpcyBoYXBwZW5pbmcsIGFuZCB3ZSBzaG91bGQgZ3VhcmQgYWdh aW5zdCB0aGF0LiBUaGlzIHBhdGNoLCBob3dldmVyLCBkb2VzIAorICAgICAgICBub3QgZG8gdGhh dC4KKworICAgICAgICBUaGlzIGNoYW5nZSBpbnN0ZWFkIGFkZHJlc3NlcyB0aGUgcmVhc29uIHRo aXMgYXNzZXJ0aW9uIHN0YXJ0ZWQgaGFwcGVuaW5nIG11Y2ggCisgICAgICAgIG1vcmUgZnJlcXVl bnRseSByZWNlbnRseSwgd2hpY2ggaXMgdGhhdCBpdCByZWNlbnRseSBiZWNhbWUgdHJ1ZSB0aGF0 IAorICAgICAgICBub3RpZnlQb3NpdGlvbkNoYW5nZWQoKSBjYW4gYmUgY2FsbGVkIHdoZW4gdGhl IHBvc2l0aW9uIGhhcyBub3QgY2hhbmdlZC4gVG8gZml4IAorICAgICAgICB0aGUgYXNzZXJ0aW9u IEFORCB0aGUgYnVnIHRoYXQgdGhhdCBjaGFuZ2Ugd2FzIGludGVuZGVkIHRvIGZpeCwgd2UgY2Fu IGp1c3QgbWFrZSAKKyAgICAgICAgc3VyZSB0aGF0IGVpdGhlciB0aGUgcG9zaXRpb24gT1IgdGhl IHNjcm9sbCBvcmlnaW4gaGFzIGNoYW5nZWQgYmVmb3JlIGNhbGxpbmcgCisgICAgICAgIG5vdGlm eVBvc2l0aW9uQ2hhbmdlZCgpLiAKKworICAgICAgICAqIHBsYXRmb3JtL21hYy9TY3JvbGxBbmlt YXRvck1hYy5tbToKKyAgICAgICAgKFdlYkNvcmU6OlNjcm9sbEFuaW1hdG9yTWFjOjppbW1lZGlh dGVTY3JvbGxUb1BvaW50KToKKworICAgICAgICBDYWxsIHJlc2V0U2Nyb2xsT3JpZ2luQ2hhbmdl ZCgpIGFmdGVyIHRoZSBzY3JvbGwgaW5zdGVhZCBvZiBiZWZvcmUgc28gdGhhdCB3ZSBrbm93IAor ICAgICAgICB3aGV0aGVyIG9yIG5vdCB0byBjYWxsIG5vdGlmeVBvc2l0aW9uQ2hhbmdlZCgpLgor ICAgICAgICAqIHBsYXRmb3JtL1Njcm9sbFZpZXcuY3BwOgorICAgICAgICAoV2ViQ29yZTo6U2Ny b2xsVmlldzo6dXBkYXRlU2Nyb2xsYmFycyk6CisKIDIwMTEtMTItMDcgIEZsb3JpbiBNYWxpdGEg IDxmbWFsaXRhQGdvb2dsZS5jb20+CiAKICAgICAgICAgPGxpIHZhbHVlPSIwIj4gYmVoYXZlcyBs aWtlIDxsaT4gKHRoZSBzYW1lIGZvciBuZWdhdGl2ZSBudW1iZXJzKQpJbmRleDogU291cmNlL1dl YkNvcmUvcGxhdGZvcm0vU2Nyb2xsVmlldy5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNv cmUvcGxhdGZvcm0vU2Nyb2xsVmlldy5jcHAJKHJldmlzaW9uIDEwMjAyNykKKysrIFNvdXJjZS9X ZWJDb3JlL3BsYXRmb3JtL1Njcm9sbFZpZXcuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC01ODYsOCAr NTg2LDggQEAgdm9pZCBTY3JvbGxWaWV3Ojp1cGRhdGVTY3JvbGxiYXJzKGNvbnN0IAogCiAgICAg SW50UG9pbnQgYWRqdXN0ZWRTY3JvbGxQb3NpdGlvbiA9IGFkanVzdFNjcm9sbFBvc2l0aW9uV2l0 aGluUmFuZ2UoSW50UG9pbnQoZGVzaXJlZE9mZnNldCkpOwogICAgIGlmIChhZGp1c3RlZFNjcm9s bFBvc2l0aW9uICE9IHNjcm9sbFBvc2l0aW9uKCkgfHwgc2Nyb2xsT3JpZ2luQ2hhbmdlZCgpKSB7 Ci0gICAgICAgIHJlc2V0U2Nyb2xsT3JpZ2luQ2hhbmdlZCgpOwogICAgICAgICBTY3JvbGxhYmxl QXJlYTo6c2Nyb2xsVG9PZmZzZXRXaXRob3V0QW5pbWF0aW9uKGFkanVzdGVkU2Nyb2xsUG9zaXRp b24gKyBJbnRTaXplKHNjcm9sbE9yaWdpbigpLngoKSwgc2Nyb2xsT3JpZ2luKCkueSgpKSk7Cisg ICAgICAgIHJlc2V0U2Nyb2xsT3JpZ2luQ2hhbmdlZCgpOwogICAgIH0KIAogICAgIC8vIE1ha2Ug c3VyZSB0aGUgc2Nyb2xsYmFyIG9mZnNldHMgYXJlIHVwIHRvIGRhdGUuCkluZGV4OiBTb3VyY2Uv V2ViQ29yZS9wbGF0Zm9ybS9tYWMvU2Nyb2xsQW5pbWF0b3JNYWMubW0KPT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0g U291cmNlL1dlYkNvcmUvcGxhdGZvcm0vbWFjL1Njcm9sbEFuaW1hdG9yTWFjLm1tCShyZXZpc2lv biAxMDIwMjcpCisrKyBTb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9tYWMvU2Nyb2xsQW5pbWF0b3JN YWMubW0JKHdvcmtpbmcgY29weSkKQEAgLTY2OCw2ICs2NjgsMTAgQEAgdm9pZCBTY3JvbGxBbmlt YXRvck1hYzo6aW1tZWRpYXRlU2Nyb2xsVAogewogICAgIEZsb2F0UG9pbnQgYWRqdXN0ZWRQb3Np dGlvbiA9IGFkanVzdFNjcm9sbFBvc2l0aW9uSWZOZWNlc3NhcnkobmV3UG9zaXRpb24pOwogIAor ICAgIGJvb2wgcG9zaXRpb25DaGFuZ2VkID0gYWRqdXN0ZWRQb3NpdGlvbi54KCkgIT0gbV9jdXJy ZW50UG9zWCB8fCBhZGp1c3RlZFBvc2l0aW9uLnkoKSAhPSBtX2N1cnJlbnRQb3NZOworICAgIGlm ICghcG9zaXRpb25DaGFuZ2VkICYmICFzY3JvbGxhYmxlQXJlYSgpLT5zY3JvbGxPcmlnaW5DaGFu Z2VkKCkpCisgICAgICAgIHJldHVybjsKKwogICAgIG1fY3VycmVudFBvc1ggPSBhZGp1c3RlZFBv c2l0aW9uLngoKTsKICAgICBtX2N1cnJlbnRQb3NZID0gYWRqdXN0ZWRQb3NpdGlvbi55KCk7CiAg ICAgbm90aWZ5UG9zaXRpb25DaGFuZ2VkKCk7Cg==