REGRESSION (r226981): ASSERTION FAILED: startY >= 0 && endY <= height && startY ...
authorsimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 23 Jan 2018 01:34:33 +0000 (01:34 +0000)
committersimon.fraser@apple.com <simon.fraser@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 23 Jan 2018 01:34:33 +0000 (01:34 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181836

Reviewed by Tim Horton.
Source/WebCore:

All the filters that use ParallelJobs<> has the same type of bug where very wide but not tall
filter regions could result in computing an optimalThreadNumber that was greater than the
number of rows to process, which resulted in jobs with zero rows to process.

Since we split the work by rows, cap the maximum number of threads to height/8 so that each job
has at least 8 rows of pixels to process. Add some assertions to detect jobs with zero rows.

FEMorphology was also using implicit float -> int conversion to detect integer overflow of radius,
so change that to use explicit clamping.

Tests: svg/filters/feLighting-parallel-jobs.svg
       svg/filters/feTurbulence-parallel-jobs-wide.svg

* platform/graphics/filters/FELighting.cpp:
(WebCore::FELighting::platformApplyGenericPaint):
(WebCore::FELighting::platformApplyGeneric):
* platform/graphics/filters/FEMorphology.cpp:
(WebCore::FEMorphology::platformApplyGeneric):
(WebCore::FEMorphology::platformApply):
(WebCore::FEMorphology::platformApplyDegenerate):
(WebCore::FEMorphology::platformApplySoftware):
* platform/graphics/filters/FETurbulence.cpp:
(WebCore::FETurbulence::fillRegion const):
(WebCore::FETurbulence::platformApplySoftware):

LayoutTests:

* svg/filters/feLighting-parallel-jobs.svg: Added.
* svg/filters/feMorphology-invalid-radius.svg: Change the test to detect the bug on non-Retina too.
* svg/filters/feTurbulence-parallel-jobs-wide.svg: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@227374 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/svg/filters/feLighting-parallel-jobs-expected.txt [new file with mode: 0644]
LayoutTests/svg/filters/feLighting-parallel-jobs.svg [new file with mode: 0644]
LayoutTests/svg/filters/feMorphology-invalid-radius.svg
LayoutTests/svg/filters/feTurbulence-parallel-jobs-wide-expected.txt [new file with mode: 0644]
LayoutTests/svg/filters/feTurbulence-parallel-jobs-wide.svg [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/platform/graphics/filters/FELighting.cpp
Source/WebCore/platform/graphics/filters/FEMorphology.cpp
Source/WebCore/platform/graphics/filters/FETurbulence.cpp

index 34ffa57..0e4a91e 100644 (file)
@@ -1,3 +1,14 @@
+2018-01-22  Simon Fraser  <simon.fraser@apple.com>
+
+        REGRESSION (r226981): ASSERTION FAILED: startY >= 0 && endY <= height && startY < endY in WebCore::FEMorphology::platformApplyGeneric
+        https://bugs.webkit.org/show_bug.cgi?id=181836
+
+        Reviewed by Tim Horton.
+
+        * svg/filters/feLighting-parallel-jobs.svg: Added.
+        * svg/filters/feMorphology-invalid-radius.svg: Change the test to detect the bug on non-Retina too.
+        * svg/filters/feTurbulence-parallel-jobs-wide.svg: Added.
+
 2018-01-22  Nikita Vasilyev  <nvasilyev@apple.com>
 
         Web Inspector: Styles Redesign: data corruption when updating values quickly
diff --git a/LayoutTests/svg/filters/feLighting-parallel-jobs-expected.txt b/LayoutTests/svg/filters/feLighting-parallel-jobs-expected.txt
new file mode 100644 (file)
index 0000000..e907538
--- /dev/null
@@ -0,0 +1,2 @@
+Test passes if it does not assert in debug builds.
+
diff --git a/LayoutTests/svg/filters/feLighting-parallel-jobs.svg b/LayoutTests/svg/filters/feLighting-parallel-jobs.svg
new file mode 100644 (file)
index 0000000..158fa07
--- /dev/null
@@ -0,0 +1,21 @@
+<svg xmlns="http://www.w3.org/2000/svg">
+
+<defs>
+<filter id="light" primitiveUnits="userSpaceOnUse">
+<feSpecularLighting lighting-color="blue" surfaceScale="5" specularConstant="10" specularExponent="6">
+    <feDistantLight azimuth="0" elevation="30"/>
+</feSpecularLighting>
+</filter>
+</defs>
+
+<text y="20">Test passes if it does not assert in debug builds.</text>
+<rect x="0" y="30"  height="10"  width="10000" filter="url(#light)" fill="black" />
+
+<script>
+  <![CDATA[
+    if (window.testRunner)
+        testRunner.dumpAsText();
+]]>
+</script>
+
+</svg>
index 6c1101d..d44d957 100644 (file)
 
       <g id="morphology">
              <rect x="0" y="0" height="252" width="256" fill="green" />
-             <rect x="0" y="0" height="28" width="256" fill="red" filter="url(#f1)" />
-             <rect x="0" y="10" height="28" width="256" fill="red" filter="url(#f2)" />
-             <rect x="0" y="20" height="28" width="256" fill="red" filter="url(#f3)" />
-             <rect x="0" y="30" height="28" width="256" fill="red" filter="url(#f4)" />
-             <rect x="0" y="40" height="28" width="256" fill="red" filter="url(#f5)" />
-             <rect x="0" y="50" height="28" width="256" fill="red" filter="url(#f6)" />
-             <rect x="0" y="60" height="28" width="256" fill="red" filter="url(#f7)" />
-             <rect x="0" y="70" height="28" width="256" fill="red" filter="url(#f8)" />
-             <rect x="0" y="80" height="28" width="256" fill="red" filter="url(#f9)" />
+             <rect x="0" y="0"  height="4"  width="512" fill="red" filter="url(#f1)" />
+             <rect x="0" y="10" height="46" width="512" fill="red" filter="url(#f2)" />
+             <rect x="0" y="20" height="46" width="512" fill="red" filter="url(#f3)" />
+             <rect x="0" y="30" height="46" width="512" fill="red" filter="url(#f4)" />
+             <rect x="0" y="40" height="46" width="512" fill="red" filter="url(#f5)" />
+             <rect x="0" y="50" height="46" width="512" fill="red" filter="url(#f6)" />
+             <rect x="0" y="60" height="46" width="512" fill="red" filter="url(#f7)" />
+             <rect x="0" y="70" height="46" width="512" fill="red" filter="url(#f8)" />
+             <rect x="0" y="80" height="46" width="512" fill="red" filter="url(#f9)" />
+             <rect x="0" y="90" height="46" width="512" fill="red" filter="url(#f4)" />
       </g>
     </svg>
     <script>
diff --git a/LayoutTests/svg/filters/feTurbulence-parallel-jobs-wide-expected.txt b/LayoutTests/svg/filters/feTurbulence-parallel-jobs-wide-expected.txt
new file mode 100644 (file)
index 0000000..e907538
--- /dev/null
@@ -0,0 +1,2 @@
+Test passes if it does not assert in debug builds.
+
diff --git a/LayoutTests/svg/filters/feTurbulence-parallel-jobs-wide.svg b/LayoutTests/svg/filters/feTurbulence-parallel-jobs-wide.svg
new file mode 100644 (file)
index 0000000..f070e1a
--- /dev/null
@@ -0,0 +1,17 @@
+<svg xmlns="http://www.w3.org/2000/svg">
+    <defs>
+        <filter id="light">
+            <feTurbulence seed="10" type="turbulence" baseFrequency="0.01" numOctaves="1" />
+        </filter>
+    </defs>
+
+    <text y="20">Test passes if it does not assert in debug builds.</text>
+    <rect x="0" y="30" width="10000" height="10" filter="url(#light)"/>
+
+    <script>
+      <![CDATA[
+        if (window.testRunner)
+            testRunner.dumpAsText();
+    ]]>
+    </script>
+</svg>
index 9658850..b59bca4 100644 (file)
@@ -1,3 +1,35 @@
+2018-01-22  Simon Fraser  <simon.fraser@apple.com>
+
+        REGRESSION (r226981): ASSERTION FAILED: startY >= 0 && endY <= height && startY < endY in WebCore::FEMorphology::platformApplyGeneric
+        https://bugs.webkit.org/show_bug.cgi?id=181836
+
+        Reviewed by Tim Horton.
+        
+        All the filters that use ParallelJobs<> has the same type of bug where very wide but not tall
+        filter regions could result in computing an optimalThreadNumber that was greater than the
+        number of rows to process, which resulted in jobs with zero rows to process.
+
+        Since we split the work by rows, cap the maximum number of threads to height/8 so that each job
+        has at least 8 rows of pixels to process. Add some assertions to detect jobs with zero rows.
+
+        FEMorphology was also using implicit float -> int conversion to detect integer overflow of radius,
+        so change that to use explicit clamping.
+        
+        Tests: svg/filters/feLighting-parallel-jobs.svg
+               svg/filters/feTurbulence-parallel-jobs-wide.svg
+
+        * platform/graphics/filters/FELighting.cpp:
+        (WebCore::FELighting::platformApplyGenericPaint):
+        (WebCore::FELighting::platformApplyGeneric):
+        * platform/graphics/filters/FEMorphology.cpp:
+        (WebCore::FEMorphology::platformApplyGeneric):
+        (WebCore::FEMorphology::platformApply):
+        (WebCore::FEMorphology::platformApplyDegenerate):
+        (WebCore::FEMorphology::platformApplySoftware):
+        * platform/graphics/filters/FETurbulence.cpp:
+        (WebCore::FETurbulence::fillRegion const):
+        (WebCore::FETurbulence::platformApplySoftware):
+
 2018-01-22  Eric Carlson  <eric.carlson@apple.com>
 
         Resign NowPlaying status when no media element is eligible
index 00d53f5..63bc826 100644 (file)
@@ -1,6 +1,7 @@
 /*
  * Copyright (C) 2010 University of Szeged
  * Copyright (C) 2010 Zoltan Herczeg
+ * Copyright (C) 2018 Apple Inc.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -307,6 +308,7 @@ void FELighting::platformApplyGenericPaint(const LightingData& data, const Light
 {
     // Make sure startY is > 0 since we read from the previous row in the loop.
     ASSERT(startY);
+    ASSERT(endY > startY);
 
     for (int y = startY; y < endY; ++y) {
         int rowStartOffset = y * data.widthMultipliedByPixelSize;
@@ -341,7 +343,9 @@ void FELighting::platformApplyGenericWorker(PlatformApplyGenericParameters* para
 
 void FELighting::platformApplyGeneric(const LightingData& data, const LightSource::PaintingData& paintingData)
 {
-    int optimalThreadNumber = ((data.widthDecreasedByOne - 1) * (data.heightDecreasedByOne - 1)) / s_minimalRectDimension;
+    unsigned rowsToProcess = data.heightDecreasedByOne - 1;
+    unsigned maxNumThreads = rowsToProcess / 8;
+    unsigned optimalThreadNumber = std::min<unsigned>(((data.widthDecreasedByOne - 1) * rowsToProcess) / s_minimalRectDimension, maxNumThreads);
     if (optimalThreadNumber > 1) {
         // Initialize parallel jobs
         WTF::ParallelJobs<PlatformApplyGenericParameters> parallelJobs(&platformApplyGenericWorker, optimalThreadNumber);
@@ -351,8 +355,8 @@ void FELighting::platformApplyGeneric(const LightingData& data, const LightSourc
         if (job > 1) {
             // Split the job into "yStep"-sized jobs but there a few jobs that need to be slightly larger since
             // yStep * jobs < total size. These extras are handled by the remainder "jobsWithExtra".
-            const int yStep = (data.heightDecreasedByOne - 1) / job;
-            const int jobsWithExtra = (data.heightDecreasedByOne - 1) % job;
+            const int yStep = rowsToProcess / job;
+            const int jobsWithExtra = rowsToProcess % job;
 
             int yStart = 1;
             for (--job; job >= 0; --job) {
index 938a786..283ac93 100644 (file)
@@ -4,7 +4,7 @@
  * Copyright (C) 2005 Eric Seidel <eric@webkit.org>
  * Copyright (C) 2009 Dirk Schulze <krit@webkit.org>
  * Copyright (C) Research In Motion Limited 2010. All rights reserved.
- * Copyright (C) Apple Inc. 2017. All rights reserved.
+ * Copyright (C) Apple Inc. 2017-2018 All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ -139,6 +139,8 @@ ALWAYS_INLINE ColorComponents kernelExtremum(const ColumnExtrema& kernel, Morpho
 
 void FEMorphology::platformApplyGeneric(const PaintingData& paintingData, int startY, int endY)
 {
+    ASSERT(endY > startY);
+
     const auto& srcPixelArray = *paintingData.srcPixelArray;
     auto& dstPixelArray = *paintingData.dstPixelArray;
 
@@ -188,16 +190,17 @@ void FEMorphology::platformApply(const PaintingData& paintingData)
     float kernelFactor = sqrt(paintingData.radiusX * paintingData.radiusY) * 0.65;
 
     static const int minimalArea = (160 * 160); // Empirical data limit for parallel jobs
-    int optimalThreadNumber = (paintingData.width * paintingData.height * kernelFactor) / minimalArea;
-
+    
+    unsigned maxNumThreads = paintingData.height / 8;
+    unsigned optimalThreadNumber = std::min<unsigned>((paintingData.width * paintingData.height * kernelFactor) / minimalArea, maxNumThreads);
     if (optimalThreadNumber > 1) {
-        ParallelJobs<PlatformApplyParameters> parallelJobs(&WebCore::FEMorphology::platformApplyWorker, optimalThreadNumber);
-        int numOfThreads = parallelJobs.numberOfJobs();
+        WTF::ParallelJobs<PlatformApplyParameters> parallelJobs(&WebCore::FEMorphology::platformApplyWorker, optimalThreadNumber);
+        auto numOfThreads = parallelJobs.numberOfJobs();
         if (numOfThreads > 1) {
             // Split the job into "jobSize"-sized jobs but there a few jobs that need to be slightly larger since
             // jobSize * jobs < total size. These extras are handled by the remainder "jobsWithExtra".
-            const int jobSize = paintingData.height / numOfThreads;
-            const int jobsWithExtra = paintingData.height % numOfThreads;
+            int jobSize = paintingData.height / numOfThreads;
+            int jobsWithExtra = paintingData.height % numOfThreads;
             int currentY = 0;
             for (int job = numOfThreads - 1; job >= 0; --job) {
                 PlatformApplyParameters& param = parallelJobs.parameter(job);
@@ -224,8 +227,9 @@ bool FEMorphology::platformApplyDegenerate(Uint8ClampedArray& dstPixelArray, con
         return true;
     }
 
-    // Input radius is equal to zero or the scaled radius is less than one.
-    if (!m_radiusX || !m_radiusY) {
+    // FIXME: this should allow erode/dilate on one axis. webkit.org/b/181903.
+    // Also if both x radiusX and radiusY are zero, the result should be transparent black.
+    if (!radiusX || !radiusY) {
         FilterEffect* in = inputEffect(0);
         in->copyPremultipliedResult(dstPixelArray, imageRect);
         return true;
@@ -245,7 +249,9 @@ void FEMorphology::platformApplySoftware()
     setIsAlphaImage(in->isAlphaImage());
 
     IntRect effectDrawingRect = requestedRegionOfInputImageData(in->absolutePaintRect());
-    if (platformApplyDegenerate(*dstPixelArray, effectDrawingRect, m_radiusX, m_radiusY))
+
+    IntSize radius = flooredIntSize(FloatSize(m_radiusX, m_radiusY));
+    if (platformApplyDegenerate(*dstPixelArray, effectDrawingRect, radius.width(), radius.height()))
         return;
 
     Filter& filter = this->filter();
@@ -253,7 +259,7 @@ void FEMorphology::platformApplySoftware()
     if (!srcPixelArray)
         return;
 
-    IntSize radius = flooredIntSize(filter.scaledByFilterResolution({ m_radiusX, m_radiusY }));
+    radius = flooredIntSize(filter.scaledByFilterResolution({ m_radiusX, m_radiusY }));
     int radiusX = std::min(effectDrawingRect.width() - 1, radius.width());
     int radiusY = std::min(effectDrawingRect.height() - 1, radius.height());
 
index 73a8eb7..d369632 100644 (file)
@@ -5,7 +5,7 @@
  * Copyright (C) 2009 Dirk Schulze <krit@webkit.org>
  * Copyright (C) 2010 Renata Hodovan <reni@inf.u-szeged.hu>
  * Copyright (C) 2011 Gabor Loki <loki@webkit.org>
- * Copyright (C) 2017 Apple Inc.
+ * Copyright (C) 2017-2018 Apple Inc.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
 #include "FETurbulence.h"
 
 #include "Filter.h"
-#include <wtf/text/TextStream.h>
-
 #include <runtime/Uint8ClampedArray.h>
 #include <wtf/MathExtras.h>
 #include <wtf/ParallelJobs.h>
+#include <wtf/text/TextStream.h>
 
 namespace WebCore {
 
@@ -368,6 +367,8 @@ ColorComponents FETurbulence::calculateTurbulenceValueForPoint(const PaintingDat
 
 void FETurbulence::fillRegion(Uint8ClampedArray& pixelArray, const PaintingData& paintingData, StitchData stitchData, int startY, int endY) const
 {
+    ASSERT(endY > startY);
+
     IntRect filterRegion = absolutePaintRect();
     FloatPoint point(0, filterRegion.y() + startY);
     int indexOfPixelChannel = startY * (filterRegion.width() << 2);
@@ -411,18 +412,19 @@ void FETurbulence::platformApplySoftware()
     PaintingData paintingData(m_seed, tileSize, baseFrequencyX, baseFrequencyY);
     initPaint(paintingData);
 
-    int height = absolutePaintRect().height();
-
     auto area = absolutePaintRect().area();
     if (area.hasOverflowed())
         return;
 
-    unsigned optimalThreadNumber = area.unsafeGet() / s_minimalRectDimension;
+    int height = absolutePaintRect().height();
+
+    unsigned maxNumThreads = height / 8;
+    unsigned optimalThreadNumber = std::min<unsigned>(area.unsafeGet() / s_minimalRectDimension, maxNumThreads);
     if (optimalThreadNumber > 1) {
         WTF::ParallelJobs<FillRegionParameters> parallelJobs(&WebCore::FETurbulence::fillRegionWorker, optimalThreadNumber);
 
         // Fill the parameter array
-        unsigned numJobs = parallelJobs.numberOfJobs();
+        auto numJobs = parallelJobs.numberOfJobs();
         if (numJobs > 1) {
             // Split the job into "stepY"-sized jobs, distributing the extra rows into the first "jobsWithExtra" jobs.
             unsigned stepY = height / numJobs;