Tests overflow LayoutUnits in rendering code

• Component: Layout and Rendering
• Status: NEW ---

[Levi Weintraub]

A number of tests overflow LayoutUnits (currently integers) in rendering code. When we switch to FractionalLayoutUnits, we get asserts on debug builds when performing operations that overflow LayoutUnits. They do not crash in release builds. Here's a list of tests that overflow integers:

fast/canvas/canvas-toDataURL-crash.html
fast/spatial-navigation/snav-unit-overflow-and-scroll-in-direction.html
fast/overflow/overflow-height-float-not-removed-crash3.html
fast/selectors/querySelector-in-range-crash.html
fast/block/float/no-overhanging-float-crash.html

[Eric Seidel (no email)]

I would say that we should just skip these tests unless their rendering errors are easy to fix.

It seems like we do want to test our overflow behavior however.  We'll probably need to replace these ASSERTs with some other behavior, no?  What do we do on pages like these in the wild?

[Levi Weintraub]

(In reply to comment #1)
> I would say that we should just skip these tests unless their rendering errors are easy to fix.
> 
> It seems like we do want to test our overflow behavior however.  We'll probably need to replace these ASSERTs with some other behavior, no?  What do we do on pages like these in the wild?

I agree on debug builds that it makes sense to skip (I filed this bug to place in test_expectations.txt). On release, it seems worthy to continue checking for crashes.

It seems handy to make developers aware of overflows (this is how we discovered the bad behavior in bug 68744), but perhaps asserting is too strong a signal?

[Eric Seidel (no email)]

Of which developers do you speak?  WebKit devs (in which case, ASSERT is great).  Or Web Devs? (in which case a console message would be much more appropriate.)

[Levi Weintraub]

(In reply to comment #3)
> Of which developers do you speak?  WebKit devs (in which case, ASSERT is great).  Or Web Devs? (in which case a console message would be much more appropriate.)

You're right, I should have been more clear. I mean for WebKit Devs. Debug binaries can be pretty iffy for Web Devs.

A console message wouldn't be out of the question, you're right, but for performance reasons I don't think we should. In Debug builds, we check the conditions of the assert for all sorts of arithmetic operations and assignments. I don't think we'd want to ship that!

[Eric Seidel (no email)]

So should callers of these functions be checking for overflow then? before hitting these asserts?

[Levi Weintraub]

(In reply to comment #5)
> So should callers of these functions be checking for overflow then? before hitting these asserts?

That's the question. There's a common pattern present in rendering code now whereby two positive numbers are added together, and the result is maxed with zero to account for overflow. This sends a strong message to the Web Dev by often collapsing the size of an element that's grown too large, which is good, but most of the tests triggering these assertions are the result of former crashes when we didn't do this right.

It seems like it may be better to sanitize inputs earlier so we don't rely on error-prone values, but given how values are multiplied and added together throughout the rendering code, this isn't always easy or obvious.

[Eric Seidel (no email)]

ASSERTS are used for programming errors.  So if its a programming error to overflow these, then we should leave the asserts in.  If not, then we should find some other way to handle this. :)

It sounds like you're saying its a programming error to not check for overflow, if so, it should be simple to fix these tests, no?

[Levi Weintraub]

(In reply to comment #7)
> ASSERTS are used for programming errors.  So if its a programming error to overflow these, then we should leave the asserts in.  If not, then we should find some other way to handle this. :)
> 
> It sounds like you're saying its a programming error to not check for overflow, if so, it should be simple to fix these tests, no?

Well they're not all programming errors, so I guess asserting is incorrect behavior. How can we avoid dropping this information on the floor for WK Devs? My first reaction is a log message to STDERR (on debug builds only, of course), but right now WebKit is graciously quiet there, and I wouldn't really want to start us down the slippery slope of noise. Suggestions?