Bug 266939 – Prevent overflow in LayoutUnit operator++

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Ahmad Saleem]

Hi Team,

While looking into Blink's merge, I came across another potential merge:

Blink Commit: https://source.chromium.org/chromium/chromium/src/+/43a5cd79fe73e334515f0ef0a55dbf670f133923

WebKit Source: https://github.com/WebKit/WebKit/blob/b9d0bc2f5b61191b15b3d1e5a42616f10c16a27e/Source/WebCore/platform/LayoutUnit.h#L132

Just wanted to raise so we can fix it.

I am not sure of equivalent of `ClampAdd` in WebKit.

So I will not be able to do PR directly unless if someone can guide but might come back later.

Thanks!

[Ahmad Saleem]

Latest code in Chromium:

value_ = base::ClampAdd(value_, kFixedPointDenominator);

where `ClampAdd` is:

BASE_NUMERIC_ARITHMETIC_OPERATORS(Clamped, Clamp, Add, +, +=)

__

is it similar to `std::clamp`?

[Ahmad Saleem]

One which compiles (previous had some errors):

TEST(WebCoreLayoutUnit, FromFloatCeil)
{
    const float tolerance = 1.0f / kFixedPointDenominator;
    ASSERT_EQ(LayoutUnit(1.25f), LayoutUnit::fromFloatCeil(1.25f));
    ASSERT_EQ(LayoutUnit(1.25f + tolerance), LayoutUnit::fromFloatCeil(1.25f + tolerance / 2));
    ASSERT_EQ(LayoutUnit(), LayoutUnit::fromFloatCeil(-tolerance / 2));

    using Limits = std::numeric_limits<float>;
    // Larger than max()
    ASSERT_EQ(LayoutUnit::max(), LayoutUnit::fromFloatCeil(Limits::max()));
    ASSERT_EQ(LayoutUnit::max(), LayoutUnit::fromFloatCeil(Limits::infinity()));
    // Smaller than Min()
    ASSERT_EQ(LayoutUnit::min(), LayoutUnit::fromFloatCeil(Limits::lowest()));
    ASSERT_EQ(LayoutUnit::min(), LayoutUnit::fromFloatCeil(-Limits::infinity()));

    ASSERT_EQ(LayoutUnit(), LayoutUnit::fromFloatCeil(Limits::quiet_NaN()));
}

TEST(WebCoreLayoutUnit, FromFloatFloor)
{
    const float tolerance = 1.0f / kFixedPointDenominator;
    ASSERT_EQ(LayoutUnit(1.25f), LayoutUnit::fromFloatFloor(1.25f));
    ASSERT_EQ(LayoutUnit(1.25f), LayoutUnit::fromFloatFloor(1.25f + tolerance / 2));
    ASSERT_EQ(LayoutUnit(-tolerance), LayoutUnit::fromFloatFloor(-tolerance / 2));

    using Limits = std::numeric_limits<float>;
    // Larger than max()
    ASSERT_EQ(LayoutUnit::max(), LayoutUnit::fromFloatFloor(Limits::max()));
    ASSERT_EQ(LayoutUnit::max(), LayoutUnit::fromFloatFloor(Limits::infinity()));
    // Smaller than min()
    ASSERT_EQ(LayoutUnit::min(), LayoutUnit::fromFloatFloor(Limits::lowest()));
    ASSERT_EQ(LayoutUnit::min(), LayoutUnit::fromFloatFloor(-Limits::infinity()));

    ASSERT_EQ(LayoutUnit(), LayoutUnit::fromFloatFloor(Limits::quiet_NaN()));
}

TEST(WebCoreLayoutUnit, FromFloatRound)
{
    const float tolerance = 1.0f / kFixedPointDenominator;
    ASSERT_EQ(LayoutUnit(1.25f), LayoutUnit::fromFloatRound(1.25f));
    ASSERT_EQ(LayoutUnit(1.25f), LayoutUnit::fromFloatRound(1.25f + tolerance / 4));
    ASSERT_EQ(LayoutUnit(1.25f + tolerance), LayoutUnit::fromFloatRound(1.25f + tolerance * 3 / 4));
    ASSERT_EQ(LayoutUnit(-tolerance), LayoutUnit::fromFloatRound(-tolerance * 3 / 4));

    using Limits = std::numeric_limits<float>;
    // Larger than max()
    ASSERT_EQ(LayoutUnit::max(), LayoutUnit::fromFloatRound(Limits::max()));
    ASSERT_EQ(LayoutUnit::max(), LayoutUnit::fromFloatRound(Limits::infinity()));
    // Smaller than Min()
    ASSERT_EQ(LayoutUnit::min(), LayoutUnit::fromFloatRound(Limits::lowest()));
    ASSERT_EQ(LayoutUnit::min(), LayoutUnit::fromFloatRound(-Limits::infinity()));

    ASSERT_EQ(LayoutUnit(), LayoutUnit::fromFloatRound(Limits::quiet_NaN()));
}


___

i think I would remove 'quiet_Nan()' and land this to just have good coverage.

[Radar WebKit Bug Importer]

<rdar://problem/120572251>

[zak ridouh]

Pull request: https://github.com/WebKit/WebKit/pull/53550

[EWS]

Committed 302719@main (4d7b119b4c65): <https://commits.webkit.org/302719@main>

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