Summary: | Division by zero in CSSGradientValue::addStops() | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Carlos Garcia Campos <cgarcia> | ||||||
Component: | CSS | Assignee: | Nobody <webkit-unassigned> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | anilsson, benjamin, commit-queue, dino, esprehn+autocc, macpherson, menard, mlattanzio, rwlbuis, tabatkins | ||||||
Priority: | P2 | ||||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Carlos Garcia Campos
2013-04-18 04:16:55 PDT
Created attachment 198718 [details]
Patch
Looks good to me! 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 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. (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. 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. Created attachment 198841 [details]
Updated patch
It includes a ref test now and the behaviour matches what other browsers do.
(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. (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 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 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 on attachment 198841 [details] Updated patch Clearing flags on attachment: 198841 Committed r148859: <http://trac.webkit.org/changeset/148859> All reviewed patches have been landed. Closing bug. |