Closed Bug 943448 Opened 11 years ago Closed 11 years ago

Remove useless warnings about nscoord overflow in clamping functions

Categories

(Core :: Layout, defect, P4)

defect

Tracking

()

RESOLVED FIXED
mozilla28

People

(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)

Details

(Whiteboard: [qa-])

Attachments

(2 files)

I don't think these warnings are helpful at all, in fact they create
so much spam that it's easy to miss other assertions that are important
and thus they are actually harmful and we should remove them.

Also, they create HUGE logs for crashtest runs, in a sample OSX log
I found that they accounted for 37% of the total (uncompressed) size.
Also, every assertion has an associated CPU cost when symbolizing
the backtrace (which on Linux is quite costly in my experience).

Also, it's tedious to update the assertion counts in crash/reftest
manifests when we do code changes that affects them.
Attached patch fixSplinter Review
Attachment #8338608 - Flags: review?(roc)
FWIW, I'd prefer that we convert the assertions to NS_WARN_IF_FALSE. That'd address the assertion-count issue and the crashtest-log-size issue (since IIRC we don't bother with backtraces for warnings, so we'll only print 1 line instead of a full backtrace). 

It's possible that some of these warnings are not useful and should be dropped, but I'm not sure that's the case for all of them.

Also, note that there's already a bug filed (by Jesse I think) on setting a flag when we notice that a document contains huge specified sizes, and using that flag to suppress these sorts of assertions (since the nscoord_MAX values involved are then known to come from bogusly huge content, as opposed to e.g. someone incorrectly adding margins to NS_INTRINSICSIZE.)  I don't know the bug number offhand (maybe Jesse does?), but I think that would reduce the false positive rate for these warnings. Assuming we actually plan on fixing that other bug, I suspect it's premature to remove all of these warnings.
Priority: -- → P4
Version: unspecified → Trunk
(In reply to Daniel Holbert [:dholbert] from comment #3)
> FWIW, I'd prefer that we convert the assertions to NS_WARN_IF_FALSE. That'd
> address the assertion-count issue and the crashtest-log-size issue (since
> IIRC we don't bother with backtraces for warnings, so we'll only print 1
> line instead of a full backtrace). 

It doesn't help with the crashtest-log-size issue, because that's caused
by the amount of warnings; the assertion stacks isn't a size problem.
And it's not just a tbpl problem, but mainly when doing local testing,
analyzing crash / security problems etc.  It's very easy to miss some
vital piece of information in the console spam.

> It's possible that some of these warnings are not useful

They are not only not useful, they are actively harmful by making it harder
to find real issues.

> Also, note that there's already a bug filed (by Jesse I think) on setting a
> flag when we notice that a document contains huge specified sizes, and using
> that flag to suppress these sorts of assertions

That's good, but I don't think it should block this bug.  We can easily revert
this change when we have something like that implemented.
Comment on attachment 8338608 [details] [diff] [review]
fix

Review of attachment 8338608 [details] [diff] [review]:
-----------------------------------------------------------------

OK
Attachment #8338608 - Flags: review?(roc) → review+
> > Also, note that there's already [bug 765861] on setting a
> > flag when we notice that a document contains huge specified sizes, and using
> > that flag to suppress these sorts of assertions
> 
> That's good, but I don't think it should block this bug.  We can easily revert
> this change when we have something like that implemented.

Sounds reasonable.
https://hg.mozilla.org/mozilla-central/rev/0b5a8fd704ad
https://hg.mozilla.org/mozilla-central/rev/0e4c943ef4a6
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: