Bug 114807 – Division by zero in CSSGradientValue::addStops()

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Carlos Garcia Campos]

This causes an assertion when running fast/forms/type-after-focus-rule-shrink-width.html in BlackBerry port. The test contains the following:

input { width: 200px; background: -webkit-linear-gradient(left, white 110px, green 100px); }
input:focus { width: 100px; }

when the input is focused, the first offset of the gradient is outside the input. These offsest are normalized as 1.078431 and 0.980392 in CSSGradientValue::addStops(). But then there's a loop that checks if there are color stop with a position that is less than others specified in the list and setting them to the largest one. In our case we end up with two color stops at the same position 1.078431. At the end of the method, positions are normalized to the 0..1 scale in case of being out of the scale. Since both color stops are at the same position, its current scale is 0, so every offset is computed as a value divided by zero.

So platform gradients are filled with two color stops at 'nan' position. The BlackBerry port requires that the first color stop is at position 0 and the last one at position 1, there's code to ensure that, but it checks whether offset[0] > 0 which is the case, and in platform code there's an ASSERT that checks offset[0] == 0 and it crashes.

[Carlos Garcia Campos]

Created attachment 198718 [details]
Patch

[Tab Atkins]

Looks good to me!

[Benjamin Poulain]

Comment on attachment 198718 [details]
Patch

This is common code, not blackberry code.
How can this be tested? A division by zero should be reproducible on any platform.

[Dean Jackson]

Comment on attachment 198718 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        Fixes a crash in BlackBerry port running
> +        fast/forms/type-after-focus-rule-shrink-width.html.

As Ben said, this isn't Blackberry code even if that's where it was crashing. The test you mention does have a linear gradient, but you should make one that is specific to this change.

[Carlos Garcia Campos]

(In reply to comment #3)
> (From update of attachment 198718 [details])
> This is common code, not blackberry code.
> How can this be tested? A division by zero should be reproducible on any platform.

The problem is that for some reason the division by zero doesn't fail, but the offset is set to a 'nan' value. I don't know why, maybe it depends on the compiler. The thing is that platform gradients receive color stops at invalid positions. The fact that this crashes in the BlackBerry port is just coincidence, because it requires that gradients have a color stop at positions 0 and 1 and the platform code has an assert to check it. The code that ensures gradients are correctly passed to the platform doesn't deal correctly with those 'nan' values. I've checked that the same happens in cairo backend, but those values are passed directly to cairo that internally handles them.

In any case, I'll see how we can test it.

[Carlos Garcia Campos]

hmm, I've been checking what other browsers do, and maybe we should match what others do, see

http://plexode.com/eval3/#s=aekVQXANJVQMbAwEBHXgfPU+alQEPQ1BZAVycnZV5GwFKT02rRg5DTVBETBylpoOpEhERUVm2T6aVgqkUvL7AwgGGur2/t52Eur4BhwGxQrTJwosOU0ZRRkJVqU9Q3uDiVduVXpq3D61P4lMSo9CV3UpOQkhGVIOLBGIZMJRVBzyegOJBTIRFJRGJ5VBJ/BoBLBJgxGAITXa+i0MI0RjjHLIKdidfT8f0AJxQLcIVxCKcLhsPiMTisXjJVjcdZ0gb8jCclk74AMqfr/AIOJxUmLzmc1h0QiUUi0YjUcjxZoMiYNEZ1GYKddybsiVeMyKYTe9olIOfdKgECgkGqEKhlUnFXndaClcr0jY1ilD5uMrpdNl94qV6m9WnVZnoBwFAAMhwdFw1IxFzplOxs0x9VnNYnkby0fzFCYOEX1jU1mUwdCCYaDQDpyAJ3Dw9AcJmb2UBZ34f2pyTNv3W833Atdt4nG5HKAYNAZQJJLSQDCIDLwAA

In this example, both ff and ie10 render the second box with no background, so they ignore color stops outside the scale. The result in webkit depends on every port, since we are passing nan values to the platform it depends on how every platform deals with those values. With cairo backend I get a solid #80c080 background while with BlackBerry I get a solid green background.

[Carlos Garcia Campos]

Created attachment 198841 [details]
Updated patch

It includes a ref test now and the behaviour matches what other browsers do.

[Tab Atkins]

(In reply to comment #6)
> hmm, I've been checking what other browsers do, and maybe we should match what others do, see
> 
> http://plexode.com/eval3/#s=aekVQXANJVQMbAwEBHXgfPU+alQEPQ1BZAVycnZV5GwFKT02rRg5DTVBETBylpoOpEhERUVm2T6aVgqkUvL7AwgGGur2/t52Eur4BhwGxQrTJwosOU0ZRRkJVqU9Q3uDiVduVXpq3D61P4lMSo9CV3UpOQkhGVIOLBGIZMJRVBzyegOJBTIRFJRGJ5VBJ/BoBLBJgxGAITXa+i0MI0RjjHLIKdidfT8f0AJxQLcIVxCKcLhsPiMTisXjJVjcdZ0gb8jCclk74AMqfr/AIOJxUmLzmc1h0QiUUi0YjUcjxZoMiYNEZ1GYKddybsiVeMyKYTe9olIOfdKgECgkGqEKhlUnFXndaClcr0jY1ilD5uMrpdNl94qV6m9WnVZnoBwFAAMhwdFw1IxFzplOxs0x9VnNYnkby0fzFCYOEX1jU1mUwdCCYaDQDpyAJ3Dw9AcJmb2UBZ34f2pyTNv3W833Atdt4nG5HKAYNAZQJJLSQDCIDLwAA
> 
> In this example, both ff and ie10 render the second box with no background, so they ignore color stops outside the scale. The result in webkit depends on every port, since we are passing nan values to the platform it depends on how every platform deals with those values. With cairo backend I get a solid #80c080 background while with BlackBerry I get a solid green background.

FF and IE aren't rendering with no background, they're correctly handling the color-stops (correcting the second one's position to 310px), and then rendering the resulting gradient.  It just so happens that, with the specific gradient you've constructed, the visible area is completely white.

If you changed it to use a non-white color as the first color-stop, you'd see that they definitely do render things correctly.

[Carlos Garcia Campos]

(In reply to comment #8)
> (In reply to comment #6)
> > hmm, I've been checking what other browsers do, and maybe we should match what others do, see
> > 
> > http://plexode.com/eval3/#s=aekVQXANJVQMbAwEBHXgfPU+alQEPQ1BZAVycnZV5GwFKT02rRg5DTVBETBylpoOpEhERUVm2T6aVgqkUvL7AwgGGur2/t52Eur4BhwGxQrTJwosOU0ZRRkJVqU9Q3uDiVduVXpq3D61P4lMSo9CV3UpOQkhGVIOLBGIZMJRVBzyegOJBTIRFJRGJ5VBJ/BoBLBJgxGAITXa+i0MI0RjjHLIKdidfT8f0AJxQLcIVxCKcLhsPiMTisXjJVjcdZ0gb8jCclk74AMqfr/AIOJxUmLzmc1h0QiUUi0YjUcjxZoMiYNEZ1GYKddybsiVeMyKYTe9olIOfdKgECgkGqEKhlUnFXndaClcr0jY1ilD5uMrpdNl94qV6m9WnVZnoBwFAAMhwdFw1IxFzplOxs0x9VnNYnkby0fzFCYOEX1jU1mUwdCCYaDQDpyAJ3Dw9AcJmb2UBZ34f2pyTNv3W833Atdt4nG5HKAYNAZQJJLSQDCIDLwAA
> > 
> > In this example, both ff and ie10 render the second box with no background, so they ignore color stops outside the scale. The result in webkit depends on every port, since we are passing nan values to the platform it depends on how every platform deals with those values. With cairo backend I get a solid #80c080 background while with BlackBerry I get a solid green background.
> 
> FF and IE aren't rendering with no background, they're correctly handling the color-stops (correcting the second one's position to 310px), and then rendering the resulting gradient.  It just so happens that, with the specific gradient you've constructed, the visible area is completely white.
> 
> If you changed it to use a non-white color as the first color-stop, you'd see that they definitely do render things correctly.

Yes, this is exactly with my patch does, with the patch applied, changing the color to red instead of white makes the second gradient to be rendered solid red, at least with cairo backend.

[Dean Jackson]

Comment on attachment 198841 [details]
Updated patch

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

> Source/WebCore/css/CSSGradientValue.cpp:347
> -            for (size_t i = 0; i < numStops; ++i)
> -                stops[i].offset = (stops[i].offset - firstOffset) / scale;
> +                for (size_t i = 0; i < numStops; ++i)
> +                    stops[i].offset = (stops[i].offset - firstOffset) / scale;

I'm seeing darker green in the diff here. Are they tab characters? Maybe it's just highlighting the extra indentation.

[Carlos Garcia Campos]

Comment on attachment 198841 [details]
Updated patch

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

Thanks for the review.

>> Source/WebCore/css/CSSGradientValue.cpp:347
>> +                    stops[i].offset = (stops[i].offset - firstOffset) / scale;
> 
> I'm seeing darker green in the diff here. Are they tab characters? Maybe it's just highlighting the extra indentation.

I don't know what it means, but there are not tabs there, I've just checked it.

[WebKit Commit Bot]

Comment on attachment 198841 [details]
Updated patch

Clearing flags on attachment: 198841

Committed r148859: <http://trac.webkit.org/changeset/148859>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.