Bug 93903 – Crash in RenderTableCell::borderTop() due to custom scrollbars after r124168

• Component: Tables
• Status: RESOLVED FIXED

[Julien Chaffraix]

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).

[Julien Chaffraix]

Created attachment 158114 [details]
Reproduction - beware it will crash your browser

[Julien Chaffraix]

Created attachment 158124 [details]
Proposed fix: Added parent NULL-checks to detect detach tree conditions.

[Tony Chang]

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?

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Tony Chang]

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.

[Julien Chaffraix]

(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.

[Julien Chaffraix]

Created attachment 158584 [details]
Proposed fix 2. Not super happy about the fix but it's the safest change.

[Julien Chaffraix]

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

[Ojan Vafai]

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?

[Julien Chaffraix]

(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)

[Tony Chang]

(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).

[Ojan Vafai]

(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.

[Julien Chaffraix]

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.

[Julien Chaffraix]

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

[Julien Chaffraix]

Created attachment 160189 [details]
Better work-around: use style information.

[Tony Chang]

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.

[Julien Chaffraix]

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.

[Tony Chang]

Comment on attachment 160189 [details]
Better work-around: use style information.

Ah, you're right, BorderData does the check.

[Tony Chang]

Oh, what about hidden borders? It looks like that will return 0 but take up space.

[Julien Chaffraix]

(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.

[Tony Chang]

(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.

[Julien Chaffraix]

Comment on attachment 160189 [details]
Better work-around: use style information.

Thanks Tony!

[WebKit Review Bot]

Comment on attachment 160189 [details]
Better work-around: use style information.

Clearing flags on attachment: 160189

Committed r126591: <http://trac.webkit.org/changeset/126591>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.