David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla) Reporter
	Description • 14 years ago

I think the way we've introduced NSCoordSaturating* operations into the tree is unmaintainable, and we should figure out a different solution to the problem they're solving.

The basic problem is that these operations are currently used on only a small percentage of the places that do nscoord arithmetic.  However, they assert that their inputs are within nscoord_MAX range, which is essentially an assertion that the inputs have been produced only using these operations.  These assertions are a substantial portion of the assertions in reftests.

One possibility is that we use a new C++ type that always does saturating operations in the places where we need these saturating operations.  It's probably best to use such a type in as few places as possible (where it's needed to maintain invariants), and just allow wraparound in other places (and clamp the result to invariants, such as being nonnegative, as necessary).

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 1 • 14 years ago

(In reply to comment #0)
> The basic problem is that these operations are currently used on only a small
> percentage of the places that do nscoord arithmetic.  However, they assert that
> their inputs are within nscoord_MAX range, which is essentially an assertion
> that the inputs have been produced only using these operations.  These
> assertions are a substantial portion of the assertions in reftests.
> 
> One possibility is that we use a new C++ type that always does saturating
> operations in the places where we need these saturating operations.  It's
> probably best to use such a type in as few places as possible (where it's
> needed to maintain invariants),

It's possible that "as few places as possible" is "almost everywhere". I'm not sure, but I'd really like to know. If it is, then making nscoord a float starts to look compelling.

	Jesse Ruderman
	Comment 2 • 14 years ago

Hearing "floats" and "maintain invariants" in the same paragraph scares me.  Should it?

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 3 • 14 years ago

I'm not sure what you're scared of.

If we use floats, we'll take steps to ensure that an nscoord is still always an integer number of appunits. That should avoid rounding issues, at least until you're dealing with extremely large documents.

	Jesse Ruderman
	Comment 4 • 14 years ago

I was scared that float imprecision could lead to memory safety bugs.  dbaron explained to me that safety depends only on invariants that shouldn't be affected by loss of precision (a>=0 and a>=b, but never a>b).  So I'm less scared now.

	Jesse Ruderman
	Comment 5 • 12 years ago

See also bug 765861, "Maintain a flag for whether a document's sizes are sane".

	Karl Tomlinson (:karlt)
	Comment 6 • 11 years ago

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #0)
> One possibility is that we use a new C++ type that always does saturating
> operations in the places where we need these saturating operations.

One advantage would be that it makes it clear which variables have been saturated, and those that haven't could be saturated on conversion.

	Karl Tomlinson (:karlt)
	Comment 7 • 11 years ago

The current defence against nscoord overflow seems to resemble a minefield.
There are mines scattered about the place to catch overflow, but there are
plenty of gaps where overflows sneak in.  When one of these causes a problem,
another mine is added, but I don't understand the strategy (if there is any)
in the placement of the mines.

The current behaviour of feeding unsaturated nscoords into NSCoordSaturating*
and then asserting is obviously wrong.  It would be good to pick an approach
and aim towards it.  Possible solutions to this problem are:

1) Don't assert in NSCoordSaturating*.

2) Have conversion functions to saturate/sanitize nscoords if using
NSCoordSaturating* on unsanitized input.

With either of approaches 1 and 2, these NSCoordSaturating functions can still
produce output within [nscoord_MIN, nscoord_MAX] but it may not necessarily be
the closest approximation to the original input.

3) Check all arithmetic for overflow, through extensive use of NSCoordSaturating*

Having different types is in category 2.
Having a single type would be category 3.

	Christian Holler (:decoder)
	Comment 8 • 11 years ago

We were discussing in bug 920471 and bug 919486 that the nscoord arithmetics we're doing are not only guarded against overflows (which would be ok if we don't care for the result), but also undefined behavior because the type is signed.

If making the type float isn't a good choice, then we should maybe try to wrap it with a class that does well-defined overflowing or doesn't allow overflow at all, depending on what's better.

I was trying to implement something like that, but I got stuck deep in some ipdl code where nscoord is serialized/deserialized it seems. If someone is willing to help with that, we could get it at least compiling and measure the performance impact such a wrapper would have.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 9 • 11 years ago

(In reply to Christian Holler (:decoder) from comment #8)
> I was trying to implement something like that, but I got stuck deep in some
> ipdl code where nscoord is serialized/deserialized it seems. If someone is
> willing to help with that,

Sure!

	Mats Palmgren (inactive)
	Comment 10 • 11 years ago

I think we should simply remove these NS_WARNINGs/NS_ASSERTIONs because
they are just spam that adds no value.  As I said in bug 920471, I'm not
against adding a wrapper per se, but I don't think adding a run-time
cost to make all nscoord operations saturating in release builds is
justified.