Open Bug 765861 Opened 12 years ago Updated 2 years ago

Maintain a flag for whether a document's sizes are sane

Categories

(Core :: Layout, enhancement)

enhancement

Tracking

()

People

(Reporter: jruderman, Unassigned)

Details

Idea:

1. Add a debug-only "everClampedSizes" flag to each document.
2. Make NSCoordSaturatingAdd set everClampedSizes to true when clamping.
3. Change relevant assertions to (everClampedSizes || (...)).

Relevant assertions are ones that have to do with sizes being correct, and are not relied upon for safety.  Many of these assertions mention NS_UNCONSTRAINEDSIZE, nscoord_MAX, or nscoord_MIN.

We'd be able to un-ignore many assertions:

* About 60 that appear in the fuzzer ignore list.
* A bunch that were downgraded to warnings, which makes the fuzzer (and regression tests) ignore them.
That sounds like a pretty good idea. The flag should live on the prescontext I guess. NSCoordSaturatingAdd should take a bool* as a parameter.
Should the parameter be debug-only?  If so, how would that work?
When fixing this, we might want to also re-instate the warnings and assertions removed in bug 943448.
(In reply to :Ms2ger from comment #4)
> Also add the one mentioned in
> http://hg.mozilla.org/integration/mozilla-inbound/rev/77dc30ebae1e

(When we fix this bug, we can search for mentions of this bug number in the codebase and upgrade/enable assertions as-appropriate. No need to specifically call out specifical places here - we can find them with an MXR search when the time comes.)
(In reply to Daniel Holbert [:dholbert] (mostly away until Jan 2) from comment #5)
> No need to specifically call out specifical places here - we can find them with an MXR search

er, s/specifical places/specific places that already include a comment mentioning this bug #/
An alternative would be to set the flag while resolving lengths in the
style system.  I once made a patch to detect "huge" values and it was
a reasonably small patch (4-5 places IIRC).  I think that would mute
most of these assertions.
If the only purpose of this flag is to make fuzzer happy, why not just add another special power function which allows fuzzer set it itself?

Alternately, we can have a special type of assertion which declares that it could be violated if a document contains unreasonable huge length, so that fuzzer can simply ignore those assertion when it does generate huge lengthes in the document.

The current situation, in my opinion, is very unhelpful. Fuzzer continuously reports assertions simply because of those kind of number which is usually safe.

I think these proposals could avoid adding too much complexity to our codebase for a single purpose, which also means they could be easier to implement. Jesse, what do you think about the proposals?
Flags: needinfo?(jruderman)
Re #8, I'd prefer for Gecko to detect the situation. There are many ways for the fuzzer to introduce large sizes into a document.

Re #7, if the detection is done in the style system, we'll have to pick a conservative cutoff for "huge" values, so we don't hit assertions by having several nearby elements just under the limit.

What's the problem with the original plan? Would putting the detection in NSCoordSaturatingAdd miss things? Is it too hard to get a PresContext pointer there? (That could be solved by making the bool a global, or by printing "Ignore some assertions from this point forward" on stderr.)
Flags: needinfo?(jruderman)
It's not just the fuzzer, but the more general rule that we shouldn't assert when our implementation is behaving as expected.

I think the original plan seems fine.
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bzbarsky)
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.