Bug 95883 – Prevent overflows in FractionalLayoutUnit

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Emil A Eklund]

snapSizeToPixel overflows if the size and/or location is close to MAX_INT/60.

[Emil A Eklund]

Created attachment 162314 [details]
Patch

[Levi Weintraub]

Comment on attachment 162314 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162314&action=review

Seems like another step in the right direction, but we need to catch all the cases where we do this.

> Source/WebCore/ChangeLog:10
> +        No new tests, covered by existing tests if SATURATED_LAYOUT_ARITHMETIC is enabled.

Can we not write a test case otherwise? Should it be gated by the same flag?

> Source/WebCore/ChangeLog:14
> +        (WebCore::snapSizeToPixel): Change to use long longs and roundRawValue.

I'm a little concerned about performance here given we perform this operation a lot.

> Source/WebCore/platform/FractionalLayoutUnit.h:798
> +inline long long roundRawValue(long long value)
> +{
> +    if (value > 0)
> +        return (value + (kFixedPointDenominator / 2)) / kFixedPointDenominator;
> +    return (value - (kFixedPointDenominator / 2)) / kFixedPointDenominator;
> +}

We need to change pixelSnappedIntRect (and anywhere else we may be implicitly doing this) to use this logic to be consistent.

[Emil A Eklund]

Created attachment 162587 [details]
Patch

[Levi Weintraub]

Comment on attachment 162587 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162587&action=review

> Source/WebCore/platform/FractionalLayoutUnit.h:238
> -        m.m_value = std::numeric_limits<int>::max();
> +        m.m_value = std::numeric_limits<int>::max() - kFixedPointDenominator / 2;

This is confusing without a comment or named constant.

> Source/WebCore/platform/FractionalLayoutUnit.h:244
> -        m.m_value = std::numeric_limits<int>::min();
> +        m.m_value = std::numeric_limits<int>::min() + kFixedPointDenominator / 2;

Ditto.

[Emil A Eklund]

Created attachment 162591 [details]
Patch

[Levi Weintraub]

Comment on attachment 162591 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162591&action=review

> Source/WebCore/platform/FractionalLayoutUnit.h:239
> +        // Reduce max value by kFixedPointDenominator / 2 to prevent max().round() from overflowing.
> +        m.m_value = std::numeric_limits<int>::max() - kFixedPointDenominator / 2;

So there's some weird things to think about here... Now it's possible that FractionalLayoutUnit::max() + (1.0f / 60) > FractionalLayoutUnit::max()! Also, without actually double-checking the math, might it also be possible that max().round() > max()?

[Emil A Eklund]

(In reply to comment #6)
> (From update of attachment 162591 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162591&action=review
> 
> > Source/WebCore/platform/FractionalLayoutUnit.h:239
> > +        // Reduce max value by kFixedPointDenominator / 2 to prevent max().round() from overflowing.
> > +        m.m_value = std::numeric_limits<int>::max() - kFixedPointDenominator / 2;
> 
> So there's some weird things to think about here... Now it's possible that FractionalLayoutUnit::max() + (1.0f / 60) > FractionalLayoutUnit::max()! Also, without actually double-checking the math, might it also be possible that max().round() > max()?

max() and max().round() have different types so that shouldn't matter.

[Levi Weintraub]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 162591 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=162591&action=review
> > 
> > > Source/WebCore/platform/FractionalLayoutUnit.h:239
> > > +        // Reduce max value by kFixedPointDenominator / 2 to prevent max().round() from overflowing.
> > > +        m.m_value = std::numeric_limits<int>::max() - kFixedPointDenominator / 2;
> > 
> > So there's some weird things to think about here... Now it's possible that FractionalLayoutUnit::max() + (1.0f / 60) > FractionalLayoutUnit::max()! Also, without actually double-checking the math, might it also be possible that max().round() > max()?
> 
> max() and max().round() have different types so that shouldn't matter.

But FractionalLayoutUnit::max() + FractionalLayoutUnit::epsilon() do

[Emil A Eklund]

(In reply to comment #8)
> But FractionalLayoutUnit::max() + FractionalLayoutUnit::epsilon() do

Uhm, no. epsilon returns a float and max returns a FractionalLayoutUnit.

Either way I get what you are saying that max and min no longer represents the true maximum and minimum values but this seems like a better option than manually subtracting from the value when using them.

[WebKit Review Bot]

Comment on attachment 162591 [details]
Patch

Attachment 162591 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13776734

New failing tests:
media/video-zoom-controls.html

[Emil A Eklund]

Created attachment 162785 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 162785 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162785&action=review

> Source/WebCore/css/CSSPrimitiveValue.cpp:470
> +    return Length(static_cast<float>(value > intMaxForLayoutUnit || value < intMinForLayoutUnit ? 0.0 : value), Fixed);

This feels like a helper function. :)

> LayoutTests/platform/chromium/fast/css/large-number-round-trip-expected.txt:1
> +PASS: read 90010000px back as 0px, read again as 0px

What was our old behavior?

[Emil A Eklund]

(In reply to comment #12)
> (From update of attachment 162785 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=162785&action=review
> 
> > Source/WebCore/css/CSSPrimitiveValue.cpp:470
> > +    return Length(static_cast<float>(value > intMaxForLayoutUnit || value < intMinForLayoutUnit ? 0.0 : value), Fixed);
> 
> This feels like a helper function. :)

It does but it is only used in a single place right now.


> > LayoutTests/platform/chromium/fast/css/large-number-round-trip-expected.txt:1
> > +PASS: read 90010000px back as 0px, read again as 0px
> 
> What was our old behavior?

computed style would return 90010000px but offsetWidth would not.

[Emil A Eklund]

ping?

[Emil A Eklund]

Created attachment 162872 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 162872 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=162872&action=review

> Source/WebCore/platform/FractionalLayoutUnit.h:249
> +    static const FractionalLayoutUnit safeMax()

I think we could come up with a better name than "safeMax".  Maybe "nearlyMax" or "almostMax"?

> Source/WebCore/platform/graphics/FractionalLayoutRect.h:170
> +        return FractionalLayoutRect(FractionalLayoutUnit::safeMin() / 2, FractionalLayoutUnit::safeMin() / 2, FractionalLayoutUnit::safeMax(), FractionalLayoutUnit::safeMax());

we should document here why this uses "safeMax", something like:

infiniteRect is slightly smaller than INT_MAX, INT_MAX, to allow pixelSnapping to round up to the nearest value  w/o overflowing.

[Emil A Eklund]

Created attachment 162884 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 162884 [details]
Patch for landing

Clearing flags on attachment: 162884

Committed r127933: <http://trac.webkit.org/changeset/127933>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.