Prevent ScrollView::updateScrollbars() from creating scrollbars with negative sizes

• Component: WebCore Misc.
• Status: ASSIGNED ---

[Steve Block]

Creating scrollbars with negative sizes causes problems in GraphicsLayerChromium::setSize(). See http://code.google.com/p/chromium/issues/detail?id=113534 and http://trac.webkit.org/changeset/108007

[Steve Block]

Created attachment 181001 [details]
Patch

[Alexey Proskuryakov]

Why can the requested size be negative? 

In ChangeLog, you say that there is no change in behavior, but you change WebCore behavior, and update Chromium WebKit accordingly. What about other WebKits? Even if there is no change in observable behavior, there may be equivalent checks in setSize() that now become orphaned.

> See http://code.google.com/p/chromium/issues/detail?id=113534 and http://trac.webkit.org/changeset/108007

Please post reasonably complete rationale in bugs. It is disrespectful of others' time to make everyone looking at a patch guess the rationale from heaps of information in other web pages.

External links are useful when someone wants to do in depth research, not as the only source of information.

[James Robinson]

Comment on attachment 181001 [details]
Patch

If I remember correctly, at least one way to get these come out of negative width/heights for RenderBlocks in overconstrained layout situations.  I think we should try to track down and fix those rather than simply clamping in ScrollView

[Steve Block]

A more complete description of the problem ...

GraphicsLayerChromium requires that the width and height of the layer are non-negative.

http://crbug.com/113534 reported LayoutTest crashes due to WebCore setting negative sizes for GraphicsLayerChromium. As a temporary fix, code was added to GraphicsLayerChromium to clamp the size to zero. As noted in the above bug, the correct fix is to avoid generating negative sizes in WebCore, and instead have GraphicsLayerChromium assert that the size is non-negative.

I've looked into this and one source of negative layer sizes is ScrollView::updateScrollbars(). If the size of the view is zero, but scrollbars are present, the existing logic attempts to make the size of the scrollbars negative. This bug is concerned with fixing this.

[Steve Block]

> In ChangeLog, you say that there is no change in behavior, but you change
> WebCore behavior, and update Chromium WebKit accordingly. What about other
> WebKits?
I think that the fix to ScrollView::updateScrollbars() is correct irrespective of whether other ports require layer sizes to be non-negative, as I don't think it makes sense for the scrollbars to have negative sizes. I was hoping to use the EWS bots to double-check for changes in behavior, but I now learn that only the Chromium and Mac bots run LayoutTests. Is there a way to test all ports?

> Even if there is no change in observable behavior, there may be equivalent
> checks in setSize() that now become orphaned.
I've looked into this and there don't seem to be any such checks.

> If I remember correctly, at least one way to get these come out of negative
> width/heights for RenderBlocks in overconstrained layout situations.
That may be true, but isn't that a separate problem? I'm happy to look into it: do you have a repro case?

[Dana Jansens]

(In reply to comment #5)
> > If I remember correctly, at least one way to get these come out of negative
> > width/heights for RenderBlocks in overconstrained layout situations.
> That may be true, but isn't that a separate problem? I'm happy to look into it: do you have a repro case?

I recall that when we clamped incorrectly (set the position to 0 as well as the size), the http://acko.net website broke. So I guess there are some negative width things generated in that website.

[Alexey Proskuryakov]

> I think that the fix to ScrollView::updateScrollbars() is correct irrespective of whether other ports require layer sizes to be non-negative

While this is technically accurate, it would be pretty difficult to remove such orphaned checks in WebKits later. So fixing a WebCore bug without removing obsolete checks would create a minor, but long-term code maintenance burden.

> I've looked into this and there don't seem to be any such checks.

OK.

> I now learn that only the Chromium and Mac bots run LayoutTests. Is there a way to test all ports?

I don't think that there is.

[Steve Block]

> I recall that when we clamped incorrectly (set the position to 0 as well as the
> size), the http://acko.net website broke. So I guess there are some negative
> width things generated in that website.

Reading http://code.google.com/p/chromium/issues/detail?id=113534, it looks like the initial version of the clamping, which broke acko.net, clamped if either the width or height is zero. The problem was that acko.net generates layers which have zero for one dimension, but non-zero for the other, and these should not be clamped. http://trac.webkit.org/changeset/108007 fixed things to only clamp if the width or height is negative.

So acko.net doesn't produce negative sizes, and testing this again now confirms this.

[Steve Block]

Comment on attachment 181001 [details]
Patch

Setting r? again, as the issue raised by jamesr seems to be independent of this fix.

[James Robinson]

Comment on attachment 181001 [details]
Patch

I think you misunderstood the feedback.  The right thing to do here is to figure out how we're entering ScrollView::updateScrollbars() in a situation where these values are negative, figure out why those situations are arising, fix the root causes and add tests.

I also highly highly doubt that ScrollView::updateScrollbars is the only source of negative sizes in GraphicsLayer, so removing the check there will just reintroduce the crash you're claiming to fix.

[Steve Block]

> The right thing to do here is to figure out how we're entering
> ScrollView::updateScrollbars() in a situation where these values are negative,
> figure out why those situations are arising, fix the root causes and add tests.

For the particular case I'm looking at, there aren't any negative sizes when we enter ScrollView::updateScrollbars(). But if the size of the view is zero (or small), but scrollbars are present, the existing logic attempts to make the size of the scrollbars negative. This seems like a bug. Or are you saying that we should we never get into the situation where the view size is this small?

I agree that there may also be cases when we enter ScrollView::updateScrollbars() with negative sizes. However, this seems to be a separate issue, and I don't have a repro case.

> I also highly highly doubt that ScrollView::updateScrollbars is the only source
> of negative sizes in GraphicsLayer, so removing the check there will just
> reintroduce the crash you're claiming to fix.

OK, though this case isn't hit by any LayoutTests. I'll update the patch to reinstate the check, but perhaps we should also add an ASSERT to help track down the bug?

[Steve Block]

Created attachment 181794 [details]
Patch

[James Robinson]

Comment on attachment 181794 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=181794&action=review

> Source/WebCore/ChangeLog:8
> +        No change in behaviour, covered by existing tests.

are you saying this test case:

<!DOCTYPE html>
<div style="overflow:scroll; width:6px; height:30px">x

and this one:

<!DOCTYPE html>
<body dir="rtl">
<div style="overflow:scroll; width:6px; height:30px">x


render the same with and without this patch?  I really doubt that's the case and the behavior (even if it is unchanged) needs to be covered by layout tests.

[Steve Block]

Created attachment 186512 [details]
Patch

[Build Bot]

Comment on attachment 186512 [details]
Patch

Attachment 186512 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16369500

New failing tests:
scrollbars/clamp-to-zero-size.html
scrollbars/clamp-to-zero-size-rtl.html

[WebKit Review Bot]

Comment on attachment 186512 [details]
Patch

Attachment 186512 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/16374487

New failing tests:
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/clamp-to-zero-size.html
platform/chromium/virtual/gpu/compositedscrolling/scrollbars/clamp-to-zero-size-rtl.html

[Build Bot]

Comment on attachment 186512 [details]
Patch

Attachment 186512 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16371539

New failing tests:
scrollbars/clamp-to-zero-size.html
scrollbars/clamp-to-zero-size-rtl.html

[James Robinson]

Comment on attachment 186512 [details]
Patch

Can you add tests for horizontal scrollbars too, since you're also changing their behavior?

It's not immediately obvious what the expected output on these layout tests are.  Could these be reference tests?  If not, could you add an HTML comment to the tests describing what the pixel output should be and add testRunner.dumpAsText(true) in a script block inside the tests.

[Build Bot]

Comment on attachment 186512 [details]
Patch

Attachment 186512 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/16375525

New failing tests:
scrollbars/clamp-to-zero-size.html
scrollbars/clamp-to-zero-size-rtl.html

[Build Bot]

Comment on attachment 186512 [details]
Patch

Attachment 186512 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/16368554