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)
Core
Layout
Tracking
()
NEW
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.
Reporter | ||
Comment 2•12 years ago
|
||
Should the parameter be debug-only? If so, how would that work?
Reporter | ||
Comment 3•11 years ago
|
||
When fixing this, we might want to also re-instate the warnings and assertions removed in bug 943448.
Comment 4•11 years ago
|
||
Also add the one mentioned in http://hg.mozilla.org/integration/mozilla-inbound/rev/77dc30ebae1e
Comment 5•11 years ago
|
||
(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.)
Comment 6•11 years ago
|
||
(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 #/
Comment 7•10 years ago
|
||
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.
Comment 8•10 years ago
|
||
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)
Reporter | ||
Comment 9•10 years ago
|
||
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)
Comment 10•10 years ago
|
||
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.
Reporter | ||
Updated•9 years ago
|
Flags: needinfo?(bzbarsky)
Updated•9 years ago
|
Flags: needinfo?(bzbarsky)
Updated•2 years ago
|
Severity: normal → S3
You need to log in
before you can comment on or make changes to this bug.
Description
•