The 'calculateUnscaledKernelSize' method improperly treats signed and unsigned values as interchangeable. This creates problems when we have extremely large stdDevication values (such as in the test case "css3/filters/huge-blur-value.html"). When the standard deviation supplied to the routine overflows a int value, we pass In this pathological case, the Std Deviation used for the blur is the float 2.14748365E+9. When this is cast as an unsigned, then assigned to a signed integer, we end up with -257759744. The minimum of -257759744 and 500 turns out to be -257759744. IntSize kernelSize; // Limit the kernel size to 500. A bigger radius won't make a big difference for the result image but // inflates the absolute paint rect too much. This is compatible with Firefox' behavior. if (stdDeviation.x()) { int size = std::max<unsigned>(2, static_cast<unsigned>(floorf(stdDeviation.x() * gaussianKernelFactor() + 0.5f))); kernelSize.setWidth(std::min(size, gMaxKernelSize)); } Later, this kernelSize is cast back to an unsigned, bypassing this ceiling, and subjecting other code to values far in excess of 500.
<rdar://problem/19837103>
Created attachment 246582 [details] Patch
Comment on attachment 246582 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246582&action=review > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:479 > + kernelSize.setWidth(static_cast<int>(clampedSize)); I'd rather use clampTo and not static casting.
Comment on attachment 246582 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246582&action=review >> Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:479 >> + kernelSize.setWidth(static_cast<int>(clampedSize)); > > I'd rather use clampTo and not static casting. OK!
Created attachment 246591 [details] Patch
Created attachment 246593 [details] Patch
Comment on attachment 246593 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246593&action=review > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:469 > +static int clampedKernelSize(float value) clampedToKernelSize() might be a better name. > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:475 > + unsigned clampedSize = std::min(size, static_cast<unsigned>(gMaxKernelSize)); > + return clampTo<int>(clampedSize); the clampedSize variable is a bit redundant, i'd just go with clampTo<int>(std::min(size, static_cast<unsigned>(gMaxKernelSize)));
Committed r180147: <http://trac.webkit.org/changeset/180147>
Comment on attachment 246593 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=246593&action=review > Source/WebCore/platform/graphics/filters/FEGaussianBlur.cpp:473 > + unsigned size = std::max<unsigned>(2, static_cast<unsigned>(floorf(value * gaussianKernelFactor() + 0.5f))); Why not just do this math as floats, then clamp to int directly without going via unsigned?