1480832 - Define nsRegion::GetBounds() overflow behavior

Status: RESOLVED FIXED

(Core :: Graphics, enhancement, P2)

Description

[Miko Mynttinen]

nsRegion can store areas that are larger than what can be presented by nsRect returned by nsRegion::GetBounds().

For example:
nsIntRegion r;
r.OrWith(nsIntRect(std::numeric_limits<int>::max() - 1, 0, 1, 1));
r.OrWith(nsIntRect(std::numeric_limits<int>::min() + 1, 0, 1, 1));

Now r.GetBounds() returns overflowed nsIntRect [-2147483647, 0, -2, 1], which is empty.

To solve this, we either have to define the maximum bounds that are returned when region bounds would overflow, or alternatively, check for overflowed bounds at all the call sites of nsRegion::GetBounds().

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Bug 1480832: Convert nsRegion's internal bounds representation to nsRectAbsolute, and define overflow behaviors.

Review commit: https://reviewboard.mozilla.org/r/261576/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/261576/

Comment 2

[Bas Schouten (:bas.schouten)]

Comment on attachment 8997908 [details]
Bug 1480832: Convert nsRegion's internal bounds representation to nsRectAbsolute, and define overflow behaviors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261576/diff/1-2/

Comment 3

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8997908 [details]
Bug 1480832: Convert nsRegion's internal bounds representation to nsRectAbsolute, and define overflow behaviors.

https://reviewboard.mozilla.org/r/261576/#review268656

::: gfx/2d/RectAbsolute.h:67
(Diff revision 2)
> +  void Inflate(T aDx, T aDy)
> +  {
> +    left -= aDx;
> +    top -= aDy;
> +    right += 2 * aDx;
> +    bottom += 2 * aDy;

Do you want to deal with negative values of aDx/aDy that cause us to get an empty/ill-defined rectangle?

::: gfx/2d/RectAbsolute.h:243
(Diff revision 2)
> +  // the largest integer-coordinate rectangle contained by the unrounded result.
> +  void ScaleInverseRoundIn(double aScale) { ScaleInverseRoundIn(aScale, aScale); }
> +  // Scale 'this' by 1/aXScale and 1/aYScale, converting coordinates to integers so
> +  // that the result is the largest integer-coordinate rectangle contained by the
> +  // unrounded result.
> +  void ScaleInverseRoundIn(double aXScale, double aYScale)

Most of the changes here look to be copying things across from the BaseRect version.

It would be nice if we could share all the code for these 'logical' operations between the two rect implementation.

I think we could do something like:

template <typename T>
class RectOperations {
  void ScaleInverseRoundIn(...) {
    T::SetRightEdge(static_cast<T>(floor(double(T::XMost()) / aXScale));
     ....
  }
};

class BaseRectAbsolute : RectOperations<BaseRectAbsolute> {
  // define XMost and SetRightEdge as appropriate for the left/top/right/bottom format.
};

That way Rect/RectAbsolute would only have the minimal set of setters/getters needed to abstract over the different storage format, and all the logic operations for operating on rectangles would be in one place. Should still get good inlining and codegen I'd hope.

Might need a separate bug for that, but I think it'd be nice to have.

::: gfx/src/nsRectAbsolute.h:26
(Diff revision 2)
> +
> +  MOZ_ALWAYS_INLINE nscoord SafeWidth() const
> +  {
> +    int64_t width = right;
> +    width -= left;
> +    return nscoord(std::min<int64_t>(std::numeric_limits<nscoord>::max(), width));

We have an existing in-tree definition for nscoord_MAX and it's not the same value as what numeric_limits would give us I think (it's 1<<30 - 1).

Comment 4

[Bas Schouten (:bas.schouten)]

Comment on attachment 8997908 [details]
Bug 1480832: Convert nsRegion's internal bounds representation to nsRectAbsolute, and define overflow behaviors.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/261576/diff/2-3/

Comment 5

[Bas Schouten (:bas.schouten)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #3)
> Comment on attachment 8997908 [details]
> Bug 1480832: Convert nsRegion's internal bounds representation to
> nsRectAbsolute, and define overflow behaviors.
> 
> https://reviewboard.mozilla.org/r/261576/#review268656
> 
> ::: gfx/2d/RectAbsolute.h:67
> (Diff revision 2)
> > +  void Inflate(T aDx, T aDy)
> > +  {
> > +    left -= aDx;
> > +    top -= aDy;
> > +    right += 2 * aDx;
> > +    bottom += 2 * aDy;
> 
> Do you want to deal with negative values of aDx/aDy that cause us to get an
> empty/ill-defined rectangle?

We don't seem to do that for BaseRect..

> ::: gfx/src/nsRectAbsolute.h:26
> (Diff revision 2)
> > +
> > +  MOZ_ALWAYS_INLINE nscoord SafeWidth() const
> > +  {
> > +    int64_t width = right;
> > +    width -= left;
> > +    return nscoord(std::min<int64_t>(std::numeric_limits<nscoord>::max(), width));
> 
> We have an existing in-tree definition for nscoord_MAX and it's not the same
> value as what numeric_limits would give us I think (it's 1<<30 - 1).

Agreed, but we never limit nsRect to them in practice, so it would cause a significant behavioral change..

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ef8c61ebeda
Convert nsRegion's internal bounds representation to nsRectAbsolute, and define overflow behaviors. r=mattwoodrow

Comment 7

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/8ef8c61ebeda