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)
Core
Layout
Tracking
()
RESOLVED
FIXED
mozilla28
People
(Reporter: MatsPalmgren_bugz, Assigned: MatsPalmgren_bugz)
Details
(Whiteboard: [qa-])
Attachments
(2 files)
6.27 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
5.93 KB,
patch
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
Attachment #8338608 -
Flags: review?(roc)
Assignee | ||
Comment 2•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=e4efa86b95d5
Comment 3•11 years ago
|
||
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.
Updated•11 years ago
|
Priority: -- → P4
Version: unspecified → Trunk
Assignee | ||
Comment 4•11 years ago
|
||
(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+
Comment 6•11 years ago
|
||
> > 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.
Assignee | ||
Comment 7•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0b5a8fd704ad https://hg.mozilla.org/integration/mozilla-inbound/rev/0e4c943ef4a6
Flags: in-testsuite-
Comment 8•11 years ago
|
||
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
Updated•10 years ago
|
Whiteboard: [qa-]
You need to log in
before you can comment on or make changes to this bug.
Description
•