Bug 114807 - Division by zero in CSSGradientValue::addStops()
Summary: Division by zero in CSSGradientValue::addStops()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-04-18 04:16 PDT by Carlos Garcia Campos
Modified: 2013-04-21 23:53 PDT (History)
10 users (show)

See Also:


Attachments
Patch (1.69 KB, patch)
2013-04-18 04:21 PDT, Carlos Garcia Campos
benjamin: review-
Details | Formatted Diff | Diff
Updated patch (5.05 KB, patch)
2013-04-19 04:17 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2013-04-18 04:16:55 PDT
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.
Comment 1 Carlos Garcia Campos 2013-04-18 04:21:56 PDT
Created attachment 198718 [details]
Patch
Comment 2 Tab Atkins 2013-04-18 10:53:42 PDT
Looks good to me!
Comment 3 Benjamin Poulain 2013-04-18 14:07:43 PDT
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.
Comment 4 Dean Jackson 2013-04-18 22:06:43 PDT
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.
Comment 5 Carlos Garcia Campos 2013-04-18 23:45:33 PDT
(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.
Comment 6 Carlos Garcia Campos 2013-04-19 02:15:51 PDT
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.
Comment 7 Carlos Garcia Campos 2013-04-19 04:17:45 PDT
Created attachment 198841 [details]
Updated patch

It includes a ref test now and the behaviour matches what other browsers do.
Comment 8 Tab Atkins 2013-04-19 13:03:18 PDT
(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.
Comment 9 Carlos Garcia Campos 2013-04-20 00:32:36 PDT
(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.
Comment 10 Dean Jackson 2013-04-20 14:07:31 PDT
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.
Comment 11 Carlos Garcia Campos 2013-04-21 23:24:43 PDT
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.
Comment 12 WebKit Commit Bot 2013-04-21 23:53:21 PDT
Comment on attachment 198841 [details]
Updated patch

Clearing flags on attachment: 198841

Committed r148859: <http://trac.webkit.org/changeset/148859>
Comment 13 WebKit Commit Bot 2013-04-21 23:53:24 PDT
All reviewed patches have been landed.  Closing bug.