Bug 266855 – Make FloatQuad::boundingBox manage large finite values and infinite ones

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

[Ahmad Saleem]

Hi Team,

While going through Blink's commit, I came across potential assertion fix but also might lead to bit more stability. I just wanted to raise so can get input, if we can merge this:

Blink Commit - https://chromium.googlesource.com/chromium/src.git/+/6f25d3a7fca5d1a30eb5f05c4057ee33aa7eb702

WebKit Source - https://searchfox.org/wubkat/rev/623da6e67d8956d951c7cdbbd53328ae0b7d2f72/Source/WebCore/platform/graphics/FloatQuad.cpp#84

___

this compiles:

static inline float saturateInf(float value)
{
    if (UNLIKELY(std::isinf(value)))
        return std::signbit(value) ? std::numeric_limits<int>::min() : std::numeric_limits<int>::min();
    
    return value;
}

FloatRect FloatQuad::boundingBox() const
{
    float left   = saturateInf(min4(m_p1.x(), m_p2.x(), m_p3.x(), m_p4.x()));
    float top    = saturateInf(min4(m_p1.y(), m_p2.y(), m_p3.y(), m_p4.y()));

    float right  = saturateInf(max4(m_p1.x(), m_p2.x(), m_p3.x(), m_p4.x()));
    float bottom = saturateInf(max4(m_p1.y(), m_p2.y(), m_p3.y(), m_p4.y()));
    
    return FloatRect(left, top, right - left, bottom - top);
}

___

Thanks!

[Ahmad Saleem]

This was further modified to add this as well:

https://chromium.googlesource.com/chromium/src/+/fdda021cbb442c16633595fa580614b6039132e3

[Ahmad Saleem]

Also just to be mindful - it will lead to this static analysis warning as well: https://chromium.googlesource.com/chromium/src/+/76905b16d9f5ba9533a7e8d41724fcd383fa4d37

[Ahmad Saleem]

static inline float clampToIntRange(float value)
{
    if (UNLIKELY(std::isinf(value) || std::abs(value) > std::numeric_limits<int>::max()))
        return std::signbit(value) ? std::numeric_limits<int>::min() : std::numeric_limits<int>::max();
    
    return value;
}

FloatRect FloatQuad::boundingBox() const
{
    float left   = clampToIntRange(min4(m_p1.x(), m_p2.x(), m_p3.x(), m_p4.x()));
    float top    = clampToIntRange(min4(m_p1.y(), m_p2.y(), m_p3.y(), m_p4.y()));

    float right  = clampToIntRange(max4(m_p1.x(), m_p2.x(), m_p3.x(), m_p4.x()));
    float bottom = clampToIntRange(max4(m_p1.y(), m_p2.y(), m_p3.y(), m_p4.y()));
    
    return FloatRect(left, top, right - left, bottom - top);
}

__

Including all three commits.

[Ahmad Saleem]

TEST(FloatQuad, BoundingBox)
{
    FloatQuad quad(FloatPoint(2, 3), FloatPoint(5, 7), FloatPoint(11, 13), FloatPoint(17, 19));
    FloatRect rect = quad.boundingBox();
    ASSERT_EQ(rect.x(), 2);
    ASSERT_EQ(rect.y(), 3);
    ASSERT_EQ(rect.width(), 17 - 2);
    ASSERT_EQ(rect.height(), 19 - 3);
}

TEST(FloatQuad, BoundingBoxSaturateInf)
{
    constexpr double inf = std::numeric_limits<double>::infinity();
    FloatQuad quad(FloatPoint(-inf, 3), FloatPoint(5, inf), FloatPoint(11, 13), FloatPoint(17, 19));
    FloatRect rect = quad.boundingBox();
    ASSERT_EQ(rect.x(), std::numeric_limits<int>::min());
    ASSERT_EQ(rect.y(), 3.0f);
    ASSERT_EQ(rect.width(), 17.0f - std::numeric_limits<int>::min());
    ASSERT_EQ(rect.height(), static_cast<float>(std::numeric_limits<int>::max()) - 3.0f);
}

TEST(FloatQuad, BoundingBoxSaturateLarge)
{
    constexpr double large = std::numeric_limits<float>::max() * 4;
    FloatQuad quad(FloatPoint(-large, 3), FloatPoint(5, large), FloatPoint(11, 13), FloatPoint(17, 19));
    FloatRect rect = quad.boundingBox();
    ASSERT_EQ(rect.x(), std::numeric_limits<int>::min());
    ASSERT_EQ(rect.y(), 3.0f);
    ASSERT_EQ(rect.width(), 17.0f - std::numeric_limits<int>::min());
    ASSERT_EQ(rect.height(), static_cast<float>(std::numeric_limits<int>::max()) - 3.0f);
}

[Ahmad Saleem]

(In reply to Ahmad Saleem from comment #4)
> TEST(FloatQuad, BoundingBox)
> {
>     FloatQuad quad(FloatPoint(2, 3), FloatPoint(5, 7), FloatPoint(11, 13),
> FloatPoint(17, 19));
>     FloatRect rect = quad.boundingBox();
>     ASSERT_EQ(rect.x(), 2);
>     ASSERT_EQ(rect.y(), 3);
>     ASSERT_EQ(rect.width(), 17 - 2);
>     ASSERT_EQ(rect.height(), 19 - 3);
> }
> 
> TEST(FloatQuad, BoundingBoxSaturateInf)
> {
>     constexpr double inf = std::numeric_limits<double>::infinity();
>     FloatQuad quad(FloatPoint(-inf, 3), FloatPoint(5, inf), FloatPoint(11,
> 13), FloatPoint(17, 19));
>     FloatRect rect = quad.boundingBox();
>     ASSERT_EQ(rect.x(), std::numeric_limits<int>::min());
>     ASSERT_EQ(rect.y(), 3.0f);
>     ASSERT_EQ(rect.width(), 17.0f - std::numeric_limits<int>::min());
>     ASSERT_EQ(rect.height(),
> static_cast<float>(std::numeric_limits<int>::max()) - 3.0f);
> }
> 
> TEST(FloatQuad, BoundingBoxSaturateLarge)
> {
>     constexpr double large = std::numeric_limits<float>::max() * 4;
>     FloatQuad quad(FloatPoint(-large, 3), FloatPoint(5, large),
> FloatPoint(11, 13), FloatPoint(17, 19));
>     FloatRect rect = quad.boundingBox();
>     ASSERT_EQ(rect.x(), std::numeric_limits<int>::min());
>     ASSERT_EQ(rect.y(), 3.0f);
>     ASSERT_EQ(rect.width(), 17.0f - std::numeric_limits<int>::min());
>     ASSERT_EQ(rect.height(),
> static_cast<float>(std::numeric_limits<int>::max()) - 3.0f);
> }

With change - 

Ran 3 tests of 3 with 3 successful
------------------------------
All tests successfully passed!

[Ahmad Saleem]

Without Change:

Ran 3 tests of 3 with 1 successful
------------------------------
Test suite failed

Failed

    TestWebKitAPI.FloatQuad.BoundingBoxSaturateInf
        /Users/ahmadsaleem/Documents/GitHub-Webkit-Ahmad-Fork/Untitled/Tools/TestWebKitAPI/Tests/WebCore/FloatQuadTests.cpp:184Expected equality of these values:  rect.x()    Which is: -inf  std::numeric_limits<int>::min()    Which is: -2147483648

    TestWebKitAPI.FloatQuad.BoundingBoxSaturateLarge
        /Users/ahmadsaleem/Documents/GitHub-Webkit-Ahmad-Fork/Untitled/Tools/TestWebKitAPI/Tests/WebCore/FloatQuadTests.cpp:195Expected equality of these values:  rect.x()    Which is: -inf  std::numeric_limits<int>::min()    Which is: -2147483648

[Ahmad Saleem]

PR - https://github.com/WebKit/WebKit/pull/22244

[EWS]

Committed 272533@main (57f898204047): <https://commits.webkit.org/272533@main>

Reviewed commits have been landed. Closing PR #22244 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/120285493>