1171528 - "WARNING: Overflowed nscoord_MAX in conversion to nscoord width" logged over 65,000 times in linux debug test logs

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Eric Rahm [:erahm]]

A NS_WARNING in |SaturatingUnionEdges| is hit over 65,000 times in linux debug test logs generating the messsage:

> [NNNNN] WARNING: Overflowed nscoord_MAX in conversion to nscoord width: file ../../dist/include/nsRect.h, line 83

Either we're doing something horribly wrong (I somewhat doubt that due to the frequency of this message) and it needs to be fixed or we should consider removing the warning.

Comment 1

[Lee Salzman [:lsalzman]]

(In reply to Eric Rahm [:erahm] from comment #0)
> A NS_WARNING in |SaturatingUnionEdges| is hit over 65,000 times in linux
> debug test logs generating the messsage:
> 
> > [NNNNN] WARNING: Overflowed nscoord_MAX in conversion to nscoord width: file ../../dist/include/nsRect.h, line 83
> 
> Either we're doing something horribly wrong (I somewhat doubt that due to
> the frequency of this message) and it needs to be fixed or we should
> consider removing the warning.

Any more information you could give as to some specific tests this is occurring on and/or a link to some set of logs where this can be found in volumes?

Comment 2

[Eric Rahm [:erahm]]

(In reply to Lee Salzman [:eihrul] from comment #1)
> Any more information you could give as to some specific tests this is
> occurring on and/or a link to some set of logs where this can be found in
> volumes?

It seems to all be from crashtest. An example log: http://ftp.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-debug/1433523286/mozilla-central_ubuntu64_vm-debug_test-crashtest-bm122-tests1-linux64-build19.txt.gz

Comment 3

[Eric Rahm [:erahm]]

Attachment: mozilla-central_ubuntu64_vm-debug_test-crashtest-bm122-tests1-linux64-build19.txt.bz2

Crashtest log w/ pids and timestamps normalized out.

The command I used to accumulate all the instances:

> cat mozilla-central_ubuntu64_vm-debug_test-crashtest-bm122-tests1-linux64-build19.txt | grep WARNING | sort | uniq -c | sort -bnr | head -n 40

Comment 4

[Eric Rahm [:erahm]]

And instances per test:

49354 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/478185-1.html
11601 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/421671.html
2250 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/812893.html
1463 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/767765.html
188 - file:///builds/slave/test/build/tests/reftest/tests/layout/tables/crashtests/420654-1.xhtml
164 - file:///builds/slave/test/build/tests/reftest/tests/layout/base/crashtests/453894-1.xhtml
111 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/765621.html
85 - file:///builds/slave/test/build/tests/reftest/tests/layout/base/crashtests/576649-1.html
66 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/first-letter-638937-2.html
55 - file:///builds/slave/test/build/tests/reftest/tests/widget/cocoa/crashtests/449111-1.html
55 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/323386-1.html
54 - file:///builds/slave/test/build/tests/reftest/tests/layout/base/crashtests/439343.html
32 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/807565-1.html
32 - file:///builds/slave/test/build/tests/reftest/tests/gfx/tests/crashtests/839745-1.html
25 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/673770.html
24 - file:///builds/slave/test/build/tests/reftest/tests/layout/tables/crashtests/410426-1.html
22 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/479938-1.html
20 - file:///builds/slave/test/build/tests/reftest/tests/widget/cocoa/crashtests/435223-1.html
20 - file:///builds/slave/test/build/tests/reftest/tests/layout/base/crashtests/266360-1.html
19 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/545571-1.html
18 - file:///builds/slave/test/build/tests/reftest/tests/layout/base/crashtests/265986-1.html
16 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/459968.html
16 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/398181-2.html
16 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/398181-1.html
16 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/1169420-2.html
16 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/1169420-1.html
10 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/499885-1.xhtml
10 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/451315-1.html
10 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/444861-1.html
8 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/text-overflow-bug670564.xhtml
8 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/807565-2.html
8 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/798020-1.html
8 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/412201-1.xhtml
7 - file:///builds/slave/test/build/tests/reftest/tests/layout/tables/crashtests/450311-1.html
6 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/462968.xhtml
6 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/448903-1.html
6 - file:///builds/slave/test/build/tests/reftest/tests/layout/base/crashtests/541869-2.html
6 - file:///builds/slave/test/build/tests/reftest/tests/layout/base/crashtests/444967-1.html
5 - file:///builds/slave/test/build/tests/reftest/tests/layout/base/crashtests/763223-1.html
4 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/812879.html
4 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/553504-1.xhtml
4 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/508168-1.html
4 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/493118-1.html
4 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/411851-1.html
4 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/1146114.html
4 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/1146107.html
4 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/1015563-2.html
4 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/1015563-1.html
4 - file:///builds/slave/test/build/tests/reftest/tests/layout/base/crashtests/265973-1.html
3 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/541714-1.html
2 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/570289-1.html
2 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/489480-1.xhtml
2 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/265867-1.html
2 - file:///builds/slave/test/build/tests/reftest/tests/layout/generic/crashtests/1146103.html
2 - file:///builds/slave/test/build/tests/reftest/tests/layout/base/crashtests/266445-2.html
2 - file:///builds/slave/test/build/tests/reftest/tests/layout/base/crashtests/266445-1.html
2 - file:///builds/slave/test/build/tests/reftest/tests/layout/base/crashtests/265999-1.html
2 - file:///builds/slave/test/build/tests/reftest/tests/layout/base/crashtests/265899-1.html

Comment 5

[Lee Salzman [:lsalzman]]

(In reply to Eric Rahm [:erahm] from comment #0)
> A NS_WARNING in |SaturatingUnionEdges| is hit over 65,000 times in linux
> debug test logs generating the messsage:
> 
> > [NNNNN] WARNING: Overflowed nscoord_MAX in conversion to nscoord width: file ../../dist/include/nsRect.h, line 83
> 
> Either we're doing something horribly wrong (I somewhat doubt that due to
> the frequency of this message) and it needs to be fixed or we should
> consider removing the warning.

It would appear this occurs on all platforms, not just Linux, and is a result of the crashtests using fairly large values on purpose to try and trigger various bugs. So, in this case, we're doing something horribly wrong, but it is on purpose... So we're down to either living with the warning or just removing it as options.

Comment 6

[Eric Rahm [:erahm]]

Attachment: Silence overflowed nscoord_MAX warning

Disable the extremely verbose warning for overflowed width in nsRect. It can
be reenabled at build time by adding the |--with-debug-label=gfx| flag.

Comment 7

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2667422bc9c8

Comment 8

[Daniel Holbert [:dholbert]]

See also https://groups.google.com/d/msg/mozilla.dev.tech.layout/YXauN50HDhI/ybqAuH-ZtaUJ -- can we use MOZ_LOG here instead, so that we can configure this at runtime (or browser startup time) instead of at compile time?

Comment 9

[Eric Rahm [:erahm]]

(In reply to Daniel Holbert [:dholbert] from comment #8)
> See also
> https://groups.google.com/d/msg/mozilla.dev.tech.layout/YXauN50HDhI/ybqAuH-
> ZtaUJ -- can we use MOZ_LOG here instead, so that we can configure this at
> runtime (or browser startup time) instead of at compile time?

Sure, I'll need to add a logger though so it will be slightly more complicated.

Comment 10

[Eric Rahm [:erahm]]

Attachment: Silence overflowed nscoord_MAX warning by default

Adds a gfx specific log module and uses that to warn about overflowed
nscoord_MAX.

Comment 11

[Daniel Holbert [:dholbert]]

Minor concern: we already have gfx/2d/Logging.h, and I slightly worry that "gfxLog.h" might be confusing alongside that.

BUT, after doing a bit more research, I think we probably should just drop this warning.

Some history:
 - This warning was added by mats in bug 665597 (and has since been moved around a bit)
 - He likely added it for consistency with our NSCoordSaturatingAdd() etc. functions, defined in nscoord.h.  (Those functions all warned about overflow, at that point in time.)
 - Later on, he removed the nscoord.h versions of these warnings, in bug 943448.
 - SO, I think these nsRect.h versions are kind of orphaned, and don't really have much reason to exist given that we're not warning about the same thing for nscoord overflow in general.

(Sorry for prompting you to do the extra work of spinning up a MOZ_LOG solution! I'd forgotten that we'd dropped the nscoord.h versions in bug 943448, until now.)

Comment 12

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdbfb8c14f6f

Comment 13

[Daniel Holbert [:dholbert]]

(on the other hand: maybe we should bring back the warnings that were removed in bug 943448, as opt-in warnings statements, now using something like GFX_LOG() from this patch. But given that we haven't missed these overflow warnings since bug 943448, I tend to think we're OK with them gone.)

Comment 14

[Eric Rahm [:erahm]]

Attachment: Remove overflowed nscoord_MAX warnings from nsRect

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8620447 [details] [diff] [review]
Remove overflowed nscoord_MAX warnings from nsRect

Please also remove this line:
> 13 #include "nsDebug.h"                    // for NS_WARNING
...from the top of this file. (since you're removing the last NS_WARNING usages)

Thanks! r=me

Comment 16

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7ffed43fd4fd

Comment 17

[Eric Rahm [:erahm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b3f90ab360af

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b3f90ab360af