https://trac.webkit.org/changeset/124168 changed the code scrollbar creation code to run out of styleDidChange instead of layout. This means that we can now be potentially detached (the first time styleDidChange is called) when we create our scrollbars, which the old code didn't expect. This is causing crashes in some renderer assuming that they are inserted in the tree when we request layout information (like RenderTableCell::borderLeft).
Created attachment 158114 [details] Reproduction - beware it will crash your browser
Created attachment 158124 [details] Proposed fix: Added parent NULL-checks to detect detach tree conditions.
Comment on attachment 158124 [details] Proposed fix: Added parent NULL-checks to detect detach tree conditions. View in context: https://bugs.webkit.org/attachment.cgi?id=158124&action=review > Source/WebCore/ChangeLog:9 > + r124168 moved scrollbar creation from layout to styleDidChange. Because the first styleDidChange > + is called before we insert the new renderer into the tree, the code has to be made aware of detached Do we get a second styleDidChange after the new renderer is attached?
Comment on attachment 158124 [details] Proposed fix: Added parent NULL-checks to detect detach tree conditions. Attachment 158124 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13491393 New failing tests: scrollbars/scrollbar-orientation.html scrollbars/scrollbar-buttons.html compositing/overflow/clip-content-under-overflow-controls.html scrollbars/overflow-scrollbar-combinations.html fast/events/touch/gesture/touch-gesture-scroll-sideways.html
Created attachment 158193 [details] Archive of layout-test-results from gce-cr-linux-04 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-04 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid
Comment on attachment 158124 [details] Proposed fix: Added parent NULL-checks to detect detach tree conditions. Hmm, the cq failures seem to suggest that we don't get a second styleDidChange in all cases.
(In reply to comment #6) > (From update of attachment 158124 [details]) > Hmm, the cq failures seem to suggest that we don't get a second styleDidChange in all cases. On some of the tests, there is a style recalc when closing the document which is done after dumping the pixels (pretty weird). As you have pointed out, the change is wrong as it assumes that a second styleDidChange will occur. Let me dig deeper on that.
Created attachment 158584 [details] Proposed fix 2. Not super happy about the fix but it's the safest change.
*** Bug 94126 has been marked as a duplicate of this bug. ***
Comment on attachment 158584 [details] Proposed fix 2. Not super happy about the fix but it's the safest change. Couldn't the same bug happen in a table cell that is in a detached table row?
(In reply to comment #10) > (From update of attachment 158584 [details]) > Couldn't the same bug happen in a table cell that is in a detached table row? That cannot happen as we ignore overflow: scroll / auto on table row and table section (see StyleResolver::adjustRenderStyle)
(In reply to comment #11) > (In reply to comment #10) > > (From update of attachment 158584 [details] [details]) > > Couldn't the same bug happen in a table cell that is in a detached table row? > > That cannot happen as we ignore overflow: scroll / auto on table row and table section (see StyleResolver::adjustRenderStyle) I think Ojan means we have a detached row + cell (parent() is not null, but parent()->parent() is null).
(In reply to comment #12) > I think Ojan means we have a detached row + cell (parent() is not null, but parent()->parent() is null). Yes, that's what I meant.
Comment on attachment 158584 [details] Proposed fix 2. Not super happy about the fix but it's the safest change. I will need to investigate and test the row case, clearing the review flag in the meantime.
*** Bug 94403 has been marked as a duplicate of this bug. ***
Created attachment 160189 [details] Better work-around: use style information.
Comment on attachment 160189 [details] Better work-around: use style information. View in context: https://bugs.webkit.org/attachment.cgi?id=160189&action=review > Source/WebCore/rendering/RenderScrollbarPart.cpp:96 > + int visibleSize = m_scrollbar->owningRenderer()->width() - m_scrollbar->owningRenderer()->style()->borderLeftWidth() - m_scrollbar->owningRenderer()->style()->borderRightWidth(); I think you also need to check m_scrollbar->owningRenderer()->style()->border{Left,Right}().nonZero() since the width can be non-zero, but if the style is none, it'll resolve as no border. It should be easy to make a test case with this.
Comment on attachment 160189 [details] Better work-around: use style information. View in context: https://bugs.webkit.org/attachment.cgi?id=160189&action=review >> Source/WebCore/rendering/RenderScrollbarPart.cpp:96 >> + int visibleSize = m_scrollbar->owningRenderer()->width() - m_scrollbar->owningRenderer()->style()->borderLeftWidth() - m_scrollbar->owningRenderer()->style()->borderRightWidth(); > > I think you also need to check m_scrollbar->owningRenderer()->style()->border{Left,Right}().nonZero() since the width can be non-zero, but if the style is none, it'll resolve as no border. It should be easy to make a test case with this. I don't think so. First that's what RenderBoxModelObject::border* functions return. But also the code explicitly checks 'border-style' before returning the value: http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/style/BorderData.h#L63 Let me know if you want me to add a test covering that case.
Comment on attachment 160189 [details] Better work-around: use style information. Ah, you're right, BorderData does the check.
Oh, what about hidden borders? It looks like that will return 0 but take up space.
(In reply to comment #20) > Oh, what about hidden borders? It looks like that will return 0 but take up space. I am assuming you are talking about 'border-style: hidden' borders here. Per CSS 2.1, those borders should be treated as 'none', except for table cell with collapsing borders (http://www.w3.org/TR/CSS2/box.html#border-style-properties). The new code will work properly for the first case and only one side of the second (the one having the 'hidden' border). The second case is explicitly mentioned as a case where the new code won't work properly but I am pretty sure that the old code wasn't working totally fine either due to reading layout information that are potentially dirty or zeroed during layout. See bug 94054 for example.
(In reply to comment #21) > (In reply to comment #20) > > Oh, what about hidden borders? It looks like that will return 0 but take up space. > > I am assuming you are talking about 'border-style: hidden' borders here. Per CSS 2.1, those borders should be treated as 'none', except for table cell with collapsing borders (http://www.w3.org/TR/CSS2/box.html#border-style-properties). The new code will work properly for the first case and only one side of the second (the one having the 'hidden' border). The second case is explicitly mentioned as a case where the new code won't work properly but I am pretty sure that the old code wasn't working totally fine either due to reading layout information that are potentially dirty or zeroed during layout. See bug 94054 for example. Thanks, sounds like this will work like before. Works for me.
Comment on attachment 160189 [details] Better work-around: use style information. Thanks Tony!
Comment on attachment 160189 [details] Better work-around: use style information. Clearing flags on attachment: 160189 Committed r126591: <http://trac.webkit.org/changeset/126591>
All reviewed patches have been landed. Closing bug.