The War on Warnings

[Eric Rahm]

We emit a *lot* of runtime warnings when running debug tests. I inadvertently triggered a max log size failure during a landing this week which encouraged me to take a look at what all is being logged, and what I found was a ton of warnings (sometimes accompanied by stack traces). Most of these should probably be removed (of course if they're real issues they should be fixed, but judging by the frequency most are probably non-issues).

I'm currently cleaning up some of these, but if you happen to see something in the following list and are feeling proactive I would appreciate the help. There's even a meta bug for tracking these: https://bugzilla.mozilla.org/show_bug.cgi?id=765224

I generated this list by grabbing the logs for a recent m-c linux64 debug run, normalizing out PIDs and timestamps and then doing some sort/uniq-fu to get counts of unique lines.

This is roughly the top 40 offenders:

65959 [NNNNN] WARNING: Overflowed nscoord_MAX in conversion to nscoord width: file ../../dist/include/nsRect.h, line 83
63460 [NNNNN] WARNING: NS_ENSURE_TRUE(piTarget) failed: file gdom/events/EventDispatcher.cpp, line 469
20039 [NNNNN] WARNING: 'NS_FAILED(rv)', file gdom/workers/ServiceWorkerManager.cpp, line 2529
20039 [NNNNN] WARNING: '!BasePrincipal::IsCodebasePrincipal(aPrincipal)', file gdom/workers/ServiceWorkerManager.cpp, line 2591
17784 [NNNNN] WARNING: Subdocument container has no frame: file glayout/base/nsDocumentViewer.cpp, line 2506
16322 JavaScript warning: file:///builds/slave/test/build/tests/jsreftest/tests/js1_8/extensions/regress-476427.js, line 1: JavaScript 1.6's for-each-in loops are deprecated; consider using ES6 for-of instead
14159 [NNNNN] WARNING: NS_ENSURE_TRUE(mMutable) failed: file gnetwerk/base/nsSimpleURI.cpp, line 264
14087 [NNNNN] WARNING: NS_ENSURE_SUCCESS(EnsureScriptEnvironment(), nullptr) failed with result 0x80040111: file gdocshell/base/nsDocShell.cpp, line 4592
11315 [NNNNN] WARNING: '!mMainThread', file gxpcom/threads/nsThreadManager.cpp, line 299
10574 [NNNNN] WARNING: No docshells for remote frames!: file gdom/base/nsFrameLoader.cpp, line 491
9201 [NNNNN] WARNING: have unconstrained width; this should only result from very large sizes, not attempts at intrinsic width calculation: 'psd->mIEnd != NS_UNCONSTRAINEDSIZE', file glayout/generic/nsLineLayout.cpp, line 884
9155 [NNNNN] WARNING: have unconstrained width; this should only result from very large sizes, not attempts at intrinsic width calculation: 'psd->mIEnd != NS_UNCONSTRAINEDSIZE', file glayout/generic/nsLineLayout.cpp, line 3058
9130 [NNNNN] WARNING: have unconstrained width; this should only result from very large sizes, not attempts at intrinsic width calculation: 'aISize != NS_UNCONSTRAINEDSIZE', file glayout/generic/nsLineLayout.cpp, line 160
8844 [NNNNN] WARNING: Someone passed native anonymous content directly into frame construction. Stop doing that!: file glayout/base/nsCSSFrameConstructor.cpp, line 6559
7599 [NNNNN] WARNING: NS_ENSURE_TRUE(mDocShell) failed: file gembedding/browser/nsWebBrowser.cpp, line 363
7454 [NNNNN] WARNING: anonymous nodes should not be in child lists (bug 439258): file glayout/base/RestyleManager.cpp, line 1440
6544 [NNNNN] WARNING: Graph thread slowdown?: 'std::abs(framePosition - CurrentDriver()->StateComputedTime()) < MillisecondsToMediaTime(5)', file gdom/media/MediaStreamGraph.cpp, line 1195
6126 [NNNNN] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file gnetwerk/base/nsFileStreams.cpp, line 492
6126 [NNNNN] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file gnetwerk/base/nsFileStreams.cpp, line 205
5637 [NNNNN] WARNING: No outer window available!: file gdom/base/nsGlobalWindow.cpp, line 3915
5109 [NNNNN] WARNING: NS_ENSURE_TRUE(domWindow) failed: file gembedding/browser/nsDocShellTreeOwner.cpp, line 83
5085 [NNNNN] WARNING: NS_ENSURE_TRUE(aInBrowser) failed: file gembedding/browser/nsDocShellTreeOwner.cpp, line 79
4856 [NNNNN] WARNING: zero axis length: file gdom/svg/nsSVGLength2.cpp, line 124
4708 [NNNNN] WARNING: Shouldn't call SchedulePaint in a detached pres context: file glayout/generic/nsFrame.cpp, line 5181
4051 [NNNNN] WARNING: have unconstrained inline-size; this should only result from very large sizes, not attempts at intrinsic inline-size calculation: '(mFrameType == NS_CSS_FRAME_TYPE_INLINE && !frame->IsFrameOfType(nsIFrame::eReplaced)) || type == nsGkAtoms::textFrame || ComputedISize() != NS_UNCONSTRAINEDSIZE', file glayout/generic/nsHTMLReflowState.cpp, line 448
4050 [NNNNN] WARNING: have unconstrained inline-size; this should only result from very large sizes, not attempts at intrinsic inline-size calculation: 'AvailableISize() != NS_UNCONSTRAINEDSIZE', file glayout/generic/nsHTMLReflowState.cpp, line 360
3897 [NNNNN] WARNING: have unconstrained width; this should only result from very large sizes, not attempts at intrinsic width calculation: 'NS_UNCONSTRAINEDSIZE != aReflowState.ComputedISize()', file glayout/generic/nsBlockReflowState.cpp, line 118
3892 [NNNNN] WARNING: have unconstrained inline-size; this should only result from very large sizes, not attempts at intrinsic inline-size calculation: 'NS_UNCONSTRAINEDSIZE != computedISizeCBWM && NS_UNCONSTRAINEDSIZE != availISizeCBWM', file glayout/generic/nsHTMLReflowState.cpp, line 2398
3581 [NNNNN] WARNING: Please do not use mouseenter/leave events in chrome. They are slower than mouseover/out!: '!nsContentUtils::IsChromeDoc(d)', file gdom/events/EventListenerManager.cpp, line 367
3452 [NNNNN] WARNING: NS_ENSURE_TRUE(textComposition) failed: file gwidget/PuppetWidget.cpp, line 729

And the overall log sizes (~218M total):

31M mozilla-central_ubuntu64_vm-debug_test-crashtest-bm122-tests1-linux64-build17.txt
20M mozilla-central_ubuntu64_vm-debug_test-jsreftest-bm113-tests1-linux64-build5.txt
13M mozilla-central_ubuntu64_vm-debug_test-reftest-e10s-1-bm51-tests1-linux64-build1.txt
12M mozilla-central_ubuntu64_vm-debug_test-reftest-e10s-2-bm114-tests1-linux64-build2.txt
12M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-2-bm118-tests1-linux64-build42.txt
11M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-1-bm116-tests1-linux64-build10.txt
10M mozilla-central_ubuntu64_vm-debug_test-mochitest-3-bm52-tests1-linux64-build21.txt
8.7M mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-1-bm67-tests1-linux64-build0.txt
8.0M mozilla-central_ubuntu64_vm-debug_test-mochitest-other-bm113-tests1-linux64-build3.txt
7.5M mozilla-central_ubuntu64_vm-debug_test-reftest-1-bm117-tests1-linux64-build2.txt
7.2M mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-2-bm122-tests1-linux64-build18.txt
5.9M mozilla-central_ubuntu64_vm-debug_test-reftest-3-bm121-tests1-linux64-build0.txt
5.4M mozilla-central_ubuntu64_vm-debug_test-reftest-4-bm120-tests1-linux64-build5.txt
5.2M mozilla-central_ubuntu64_vm-debug_test-reftest-2-bm53-tests1-linux64-build17.txt
5.1M mozilla-central_ubuntu64_vm-debug_test-mochitest-2-bm121-tests1-linux64-build18.txt
5.0M mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-3-bm114-tests1-linux64-build0.txt
5.0M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-3-bm67-tests1-linux64-build15.txt
4.8M mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-3-bm51-tests1-linux64-build9.txt
4.8M mozilla-central_ubuntu64_vm-debug_test-mochitest-1-bm52-tests1-linux64-build0.txt
4.6M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-3-bm51-tests1-linux64-build22.txt
4.2M mozilla-central_ubuntu64_vm-debug_test-cppunit-bm113-tests1-linux64-build9.txt
3.5M mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-1-bm120-tests1-linux64-build4.txt
3.1M mozilla-central_ubuntu64_vm-debug_test-mochitest-4-bm114-tests1-linux64-build5.txt
2.9M mozilla-central_ubuntu64_vm-debug_test-mochitest-5-bm122-tests1-linux64-build0.txt
2.8M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-1-bm51-tests1-linux64-build52.txt
2.6M mozilla-central_ubuntu64_vm-debug_test-mochitest-jetpack-bm118-tests1-linux64-build37.txt
2.4M mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-4-bm118-tests1-linux64-build6.txt
2.4M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-5-bm118-tests1-linux64-build21.txt
2.0M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-4-bm117-tests1-linux64-build5.txt
2.0M mozilla-central_ubuntu64_vm-debug_test-jittest-1-bm123-tests1-linux64-build2.txt
2.0M mozilla-central_ubuntu64_vm-debug_test-jittest-2-bm116-tests1-linux64-build4.txt
1.8M mozilla-central_ubuntu64_vm-debug_test-mochitest-gl-bm118-tests1-linux64-build29.txt
1.7M mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-2-bm51-tests1-linux64-build7.txt
1.4M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-2-bm117-tests1-linux64-build5.txt
552K mozilla-central_ubuntu64_vm-debug_test-xpcshell-bm67-tests1-linux64-build1.txt
288K mozilla-central_ubuntu64_vm-debug_test-marionette-bm118-tests1-linux64-build8.txt
188K mozilla-central_ubuntu64_vm-debug_test-mochitest-push-bm53-tests1-linux64-build13.txt

[Martin Thomson]

On Wed, Jun 3, 2015 at 6:14 PM, Eric Rahm <er...@mozilla.com> wrote:
> I generated this list by grabbing the logs for a recent m-c linux64 debug run, normalizing out PIDs and timestamps and then doing some sort/uniq-fu to get counts of unique lines.

If your tool could be turned into a web page that runs against the
occasional build, we could use this on a more permanent basis.

BTW, I appreciate the effort. I guess that most of these are as a
result of actual problems, even if they are minor. And making tests
faster, and output easier to find is a real win.

[Karl Tomlinson]

Martin Thomson writes:

> I guess that most of these are as a result of actual problems,
> even if they are minor.

The ones that are actual problems would be the ones that are
harder to resolve.

In my experience, however, when I've seen many of one kind of
warning, investigation has revealed that the problem is that a
warning is presented when there is nothing wrong. Often this is
because someone has carelessly used NS_ENSURE_something.

Perhaps that is because the macros names don't say what they do.

[Jonas Sicking]

FWIW, I suspect it'll be hard to put a dent in the number of warnings
that we emit unless we either change all instances of
NS_ENSURE_SUCCESS(rv, rv) to use some other macro which doesn't warn,
or unless we change NS_ENSURE_SUCCESS(rv, rv) to not warn.

It feels like right now we have three incompatible desires:

* Test lots of failure cases.
* Make errors warn in debug builds on all/most frames as the failure
is propagated up the callstack.
* Don't warn a lot when testing debug builds.

It seems like we can only pick two out of these three.

/ Jonas

On Wed, Jun 3, 2015 at 6:14 PM, Eric Rahm <er...@mozilla.com> wrote:

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

[Nicholas Nethercote]

On Thu, Jun 4, 2015 at 2:49 AM, Jonas Sicking <jo...@sicking.cc> wrote:
>
> It feels like right now we have three incompatible desires:
>
> * Test lots of failure cases.
> * Make errors warn in debug builds on all/most frames as the failure
> is propagated up the callstack.
> * Don't warn a lot when testing debug builds.
>
> It seems like we can only pick two out of these three.

Do warnings (as opposed to NS_ASSERTION) do anything in tests? I don't
think they do. If that's right, a warning is only useful if a human
looks at it and acts on it, and that's clearly not happening for a lot
of these.

Nick

[David Rajchenbach-Teller]

Part of my world domination plans are to turn warnings into something
that causes test to actually fail (see bug 1080457 & co). Would you like
to join forces?

Cheers,
David

On 04/06/15 03:14, Eric Rahm wrote:
> We emit a *lot* of runtime warnings when running debug tests. I inadvertently triggered a max log size failure during a landing this week which encouraged me to take a look at what all is being logged, and what I found was a ton of warnings (sometimes accompanied by stack traces). Most of these should probably be removed (of course if they're real issues they should be fixed, but judging by the frequency most are probably non-issues).
>
> I'm currently cleaning up some of these, but if you happen to see something in the following list and are feeling proactive I would appreciate the help. There's even a meta bug for tracking these: https://bugzilla.mozilla.org/show_bug.cgi?id=765224
>
> I generated this list by grabbing the logs for a recent m-c linux64 debug run, normalizing out PIDs and timestamps and then doing some sort/uniq-fu to get counts of unique lines.
>
> This is roughly the top 40 offenders:

--
David Rajchenbach-Teller, PhD
Performance Team, Mozilla

[Karl Tomlinson]

Nicholas Nethercote writes:

> Do warnings (as opposed to NS_ASSERTION) do anything in tests? I don't
> think they do. If that's right, a warning is only useful if a human
> looks at it and acts on it, and that's clearly not happening for a lot
> of these.

Warnings in tests don't do anything but log that a problem has
occurred.

But, when the warnings are not noisy, this is often useful for
tracking the cause of intermittent failures.

Similarly, warnings about real problems are helpful when running
debug builds, but we are being spammed with so many warnings that
it is hard to imagine they can all be real problems. If they are,
then we are doing a poor job of keeping up with them.

[Ehsan Akhgari]

On 2015-06-04 6:07 AM, David Rajchenbach-Teller wrote:
> Part of my world domination plans are to turn warnings into something
> that causes test to actually fail (see bug 1080457 & co). Would you like
> to join forces?

There are very good reasons for warnings to not cause tests to fail. We
have a lot of tests that are testing failure conditions that are
expected to warn, because they are failure conditions.

In my experience, in some cases where we have a particularly spammy
warning, the underlying reason is that there is an error condition that
we know how to handle properly, but we still warn about it. Or
alternatively, there are warnings which indicate that the author of the
code did not think at the time that can happen, but a lot of them can be
ignored.

There are also warnings that were actually meant to be assertions, and
we should try to convert them to assertions, and that way they will fail
tests. That being said, assertion report stack traces in debug builds,
which is excruciatingly slow, so we have had to switch them to be
warnings in at least some cases (see bug 756045 for example.)

[Ehsan Akhgari]

On 2015-06-04 5:49 AM, Jonas Sicking wrote:
> FWIW, I suspect it'll be hard to put a dent in the number of warnings
> that we emit unless we either change all instances of
> NS_ENSURE_SUCCESS(rv, rv) to use some other macro which doesn't warn,
> or unless we change NS_ENSURE_SUCCESS(rv, rv) to not warn.

Do we really have to do that? It seems like all we need to do is to
investigate the highly occurring warnings and silence them somehow.
NS_ENSURE_SUCCESS warning is super useful when debugging things such as
intermittent test failures, or big complicated web pages that are
triggering an issue somehow -- they can often give you a starting point
on where you need to look.

> It feels like right now we have three incompatible desires:
>
> * Test lots of failure cases.
> * Make errors warn in debug builds on all/most frames as the failure
> is propagated up the callstack.
> * Don't warn a lot when testing debug builds.
>
> It seems like we can only pick two out of these three.

It seems to me that if we change #3 to mean fixing highly frequently
occurring warnings, we can have all three of the above together.

[Robert O'Callahan]

Usually I use NS_WARNING to mean "something weird and unexpected is
happening, e.g. a bug in Web page code, but not necessarily a browser bug".
Sometimes I get useful hints from NS_WARNING spew leading up to a serious
failure, but for those usages could probably be switched to PR_LOG without
losing much.

Rob
--
oIo otoeololo oyooouo otohoaoto oaonoyooonoeo owohooo oioso oaonogoroyo
owoiotoho oao oboroootohoeoro oooro osoiosotoeoro owoiololo oboeo
osouobojoeocoto otooo ojouodogomoeonoto.o oAogoaoiono,o oaonoyooonoeo
owohooo
osoaoyoso otooo oao oboroootohoeoro oooro osoiosotoeoro,o o‘oRoaocoao,o’o
oioso
oaonosowoeoroaoboloeo otooo otohoeo ocooouoroto.o oAonodo oaonoyooonoeo
owohooo
osoaoyoso,o o‘oYooouo ofooooolo!o’o owoiololo oboeo oiono odoaonogoeoro
ooofo
otohoeo ofoioroeo ooofo ohoeololo.

[David Rajchenbach-Teller]

On 04/06/15 14:30, Ehsan Akhgari wrote:
> There are very good reasons for warnings to not cause tests to fail. We
> have a lot of tests that are testing failure conditions that are
> expected to warn, because they are failure conditions.

Well, that's why the patch linked above offers a whitelist API, so that
tests can individually decide which families of warnings are either
expected, or ok-for-the-time-being.

[...]

> There are also warnings that were actually meant to be assertions, and
> we should try to convert them to assertions, and that way they will fail
> tests. That being said, assertion report stack traces in debug builds,
> which is excruciatingly slow, so we have had to switch them to be
> warnings in at least some cases (see bug 756045 for example.)

Are you talking about our weird non-fatal assertions or actual fatal
assertions?

[Andrew McCreight]

On Thu, Jun 4, 2015 at 2:49 AM, Jonas Sicking <jo...@sicking.cc> wrote:

> FWIW, I suspect it'll be hard to put a dent in the number of warnings
> that we emit unless we either change all instances of
> NS_ENSURE_SUCCESS(rv, rv) to use some other macro which doesn't warn,
> or unless we change NS_ENSURE_SUCCESS(rv, rv) to not warn.
>

As you can see the from the list, the distribution of these warnings is
very uneven. It is not necessary to solve the entire warning issue to make
huge reductions in the total volume of warnings we see. For instance, in
bug 1170642 I noticed that there were about 16MB of warnings in e10s M2,
produced by a single line of code (which Eric fixed by removing the
warning).

Andrew

> It feels like right now we have three incompatible desires:
>
> * Test lots of failure cases.
> * Make errors warn in debug builds on all/most frames as the failure
> is propagated up the callstack.
> * Don't warn a lot when testing debug builds.
>
> It seems like we can only pick two out of these three.
>

> / Jonas
>
>
>
> On Wed, Jun 3, 2015 at 6:14 PM, Eric Rahm <er...@mozilla.com> wrote:

[Daniel Holbert]

On 06/04/2015 05:30 AM, Ehsan Akhgari wrote:
> There are very good reasons for warnings to not cause tests to fail. We
> have a lot of tests that are testing failure conditions that are
> expected to warn, because they are failure conditions.

Also: in layout, there are various warnings related to coordinate
wraparound/overflow, where we're basically throwing up our hands and
warning that broken layout is likely to occur because the page is
millions of pixels tall. These can be useful hints, when debugging page
brokenness.

We have a lot of tests that exercise how we behave around this
coordinate-overflow threshold [mostly that we don't crash], and it's
expected that such tests would trigger these warnings.

One thing that might help for noise from these: Jesse filed a bug[1] a
while back on adding a flag to detect huge sizes & suppress some
assertions when we do; we could conceivably use this flag to suppress
warnings [perhaps just displaying a single "detected huge sizes"
warning], as well.

~Daniel

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=765861

[Martin Thomson]

On Jun 4, 2015 10:27 AM, "Daniel Holbert" <dhol...@mozilla.com> wrote:
> Also: in layout, there are various warnings related to coordinate
> wraparound/overflow, where we're basically throwing up our hands and
> warning that broken layout is likely to occur because the page is
> millions of pixels tall. These can be useful hints, when debugging page
> brokenness.

I know that it's more work, but isn't that what the console is best suited
for? We have some spammy warnings there too, of course (try looking for rc4
warnings if you have gmail open).

[Jonas Sicking]

On Thu, Jun 4, 2015 at 5:38 AM, Robert O'Callahan <rob...@ocallahan.org> wrote:
> Usually I use NS_WARNING to mean "something weird and unexpected is
> happening, e.g. a bug in Web page code, but not necessarily a browser bug".
> Sometimes I get useful hints from NS_WARNING spew leading up to a serious

> failure.

Yup. I think this is a quite common, and quite useful, usage of
NS_WARNING. But testing runs a lot of "weird and unexpected" things.
That's a good thing because those tend to be things that often
regress. But it also leads to a lot of warnings.

> but for those usages could probably be switched to PR_LOG without
> losing much.

I think this would mean changing most NS_ENSURE_SUCCESS(rv, rv), and
likely many other NS_ENSURE_* to macros that call PR_LOG instead.

So like I said, we'll have to either change a huge number of
NS_ENSURE_* macros to use something else, or change what NS_ENSURE_*
does.

/ Jonas

>
> Rob
> --
> oIo otoeololo oyooouo otohoaoto oaonoyooonoeo owohooo oioso oaonogoroyo
> owoiotoho oao oboroootohoeoro oooro osoiosotoeoro owoiololo oboeo
> osouobojoeocoto otooo ojouodogomoeonoto.o oAogoaoiono,o oaonoyooonoeo
> owohooo
> osoaoyoso otooo oao oboroootohoeoro oooro osoiosotoeoro,o o'oRoaocoao,o'o
> oioso
> oaonosowoeoroaoboloeo otooo otohoeo ocooouoroto.o oAonodo oaonoyooonoeo
> owohooo
> osoaoyoso,o o'oYooouo ofooooolo!o'o owoiololo oboeo oiono odoaonogoeoro
> ooofo
> otohoeo ofoioroeo ooofo ohoeololo.

[smaug]

On 06/04/2015 01:07 PM, David Rajchenbach-Teller wrote:
> Part of my world domination plans are to turn warnings into something
> that causes test to actually fail (see bug 1080457 & co). Would you like
> to join forces?

Turning warnings into failures sounds odd (but bug 1080457 seems to be actually something different).
Warnings aren't anything which should cause tests to fail.
Warnings, especially those generated by NS_ENSURE_* macros are things which are super
useful for debugging - they often point rather exactly where to start looking for an issue.
And since there is NS_ENSURE_*, there isn't necessarily any problem or bug, just an unusual state, which
is then handled by the NS_ENSURE_* macro.


-Olli

[smaug]

On 06/04/2015 09:52 PM, Jonas Sicking wrote:
> On Thu, Jun 4, 2015 at 5:38 AM, Robert O'Callahan <rob...@ocallahan.org> wrote:
>> Usually I use NS_WARNING to mean "something weird and unexpected is
>> happening, e.g. a bug in Web page code, but not necessarily a browser bug".
>> Sometimes I get useful hints from NS_WARNING spew leading up to a serious
>> failure.
>
> Yup. I think this is a quite common, and quite useful, usage of
> NS_WARNING. But testing runs a lot of "weird and unexpected" things.
> That's a good thing because those tend to be things that often
> regress. But it also leads to a lot of warnings.
>
>> but for those usages could probably be switched to PR_LOG without
>> losing much.
>
> I think this would mean changing most NS_ENSURE_SUCCESS(rv, rv), and
> likely many other NS_ENSURE_* to macros that call PR_LOG instead.
>
> So like I said, we'll have to either change a huge number of
> NS_ENSURE_* macros to use something else, or change what NS_ENSURE_*
> does.
>

More likely we need to change a small number of noisy NS_ENSURE_* macro users to use something else,
and keep most of the NS_ENSURE_* usage as it is.


-Olli

[Bobby Holley]

On Thu, Jun 4, 2015 at 1:18 PM, smaug <sm...@welho.com> wrote:

> More likely we need to change a small number of noisy NS_ENSURE_* macro
> users to use something else,
> and keep most of the NS_ENSURE_* usage as it is.
>

+1.

[Luke Wagner]

In addition to judging noisiness by volume over a whole test run, can we
also include any warning that happens on normal browser startup, new tab,
and other vanilla browser operations? This has always annoyed me.

On Thu, Jun 4, 2015 at 3:33 PM, Bobby Holley <bobby...@gmail.com> wrote:

> On Thu, Jun 4, 2015 at 1:18 PM, smaug <sm...@welho.com> wrote:
>

> > More likely we need to change a small number of noisy NS_ENSURE_* macro
> > users to use something else,
> > and keep most of the NS_ENSURE_* usage as it is.
> >
>

> +1.

[Daniel Holbert]

On 06/04/2015 01:18 PM, smaug wrote:
> More likely we need to change a small number of noisy NS_ENSURE_* macro
> users to use something else,
> and keep most of the NS_ENSURE_* usage as it is.

I agree -- I posted about switching to something opt-in, like MOZ_LOG,
for some of the spammier layout NS_WARNINGS, too:

https://groups.google.com/forum/?fromgroups#!topic/mozilla.dev.tech.layout/YXauN50HDhI

~Daniel

[Eric Rahm]

On Thursday, June 4, 2015 at 1:48:30 PM UTC-7, Luke Wagner wrote:
> In addition to judging noisiness by volume over a whole test run, can we
> also include any warning that happens on normal browser startup, new tab,
> and other vanilla browser operations? This has always annoyed me.

Yes, this bothers me as well. Here's a rough look at a run I just did (this is simply opening the browser to a blank page and then closing it):

27 [NNNNN] WARNING: '!mMainThread', file /home/erahm/dev/mozilla-central/xpcom/threads/nsThreadManager.cpp, line 299
10 [NNNNN] WARNING: 'NS_FAILED(DebuggerOnGCRunnable::Enqueue(aRuntime, aDesc))', file /home/erahm/dev/mozilla-central/xpcom/base/CycleCollectedJSRuntime.cpp, line 743
5 [NNNNN] WARNING: XPCOM objects created/destroyed from static ctor/dtor: file /home/erahm/dev/mozilla-central/xpcom/base/nsTraceRefcnt.cpp, line 148
4 [NNNNN] WARNING: RemoveObserver() called for unregistered observer: file /home/erahm/dev/mozilla-central/hal/Hal.cpp, line 205
3 [NNNNN] WARNING: NS_ENSURE_TRUE(mDocShell) failed: file /home/erahm/dev/mozilla-central/embedding/browser/nsWebBrowser.cpp, line 363
3 [NNNNN] WARNING: NS_ENSURE_SUCCESS(EnsureScriptEnvironment(), nullptr) failed with result 0x80040111: file /home/erahm/dev/mozilla-central/docshell/base/nsDocShell.cpp, line 4592
2 [NNNNN] WARNING: Subdocument container has no frame: file /home/erahm/dev/mozilla-central/layout/base/nsDocumentViewer.cpp, line 2506
2 [NNNNN] WARNING: Re-registering a CID?: file /home/erahm/dev/mozilla-central/xpcom/components/nsComponentManager.cpp, line 551
2 [NNNNN] WARNING: 'NS_FAILED(rv)', file /home/erahm/dev/mozilla-central/dom/workers/ServiceWorkerManager.cpp, line 2529
2 [NNNNN] WARNING: NS_ENSURE_TRUE(domWindow) failed: file /home/erahm/dev/mozilla-central/embedding/browser/nsDocShellTreeOwner.cpp, line 83
2 [NNNNN] WARNING: NS_ENSURE_TRUE(aInBrowser) failed: file /home/erahm/dev/mozilla-central/embedding/browser/nsDocShellTreeOwner.cpp, line 79
2 [NNNNN] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80040111: file /home/erahm/dev/mozilla-central/dom/base/nsFrameLoader.cpp, line 267
2 [NNNNN] WARNING: Enabling vsync refresh driver: file /home/erahm/dev/mozilla-central/layout/base/nsRefreshDriver.cpp, line 859
2 [NNNNN] WARNING: '!BasePrincipal::IsCodebasePrincipal(aPrincipal)', file /home/erahm/dev/mozilla-central/dom/workers/ServiceWorkerManager.cpp, line 2591
1 [NNNNN] WARNING: nsWindow::GetNativeData not implemented for this type: file /home/erahm/dev/mozilla-central/widget/PuppetWidget.cpp, line 1162
1 [NNNNN] WARNING: NS_ENSURE_TRUE(domDoc && target) failed: file /home/erahm/dev/mozilla-central/dom/base/nsContentUtils.cpp, line 3636
1 [NNNNN] WARNING: NS_ENSURE_TRUE(currentInner) failed: file /home/erahm/dev/mozilla-central/dom/base/nsGlobalWindow.cpp, line 9004
1 [NNNNN] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /home/erahm/dev/mozilla-central/dom/base/nsContentUtils.cpp, line 3691
1 [NNNNN] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /home/erahm/dev/mozilla-central/toolkit/xre/nsXREDirProvider.cpp, line 1374
1 [NNNNN] WARNING: No docshells for remote frames!: file /home/erahm/dev/mozilla-central/dom/base/nsFrameLoader.cpp, line 491
1 [NNNNN] WARNING: Hardware Vsync support not yet implemented. Falling back to software timers: file /home/erahm/dev/mozilla-central/gfx/thebes/gfxPlatform.cpp, line 2410
1 [NNNNN] WARNING: Enabling vsync compositor: file /home/erahm/dev/mozilla-central/gfx/layers/ipc/CompositorParent.cpp, line 679
1 [NNNNN] WARNING: '!editorRectEvent.mSucceeded', file /home/erahm/dev/mozilla-central/widget/PuppetWidget.cpp, line 800
1 [NNNNN] WARNING: Could not get disk status from nsIDiskSpaceWatcher: file /home/erahm/dev/mozilla-central/uriloader/prefetch/nsOfflineCacheUpdateService.cpp, line 317
1 [NNNNN] WARNING: Could not get disk information from DiskSpaceWatcher: file /home/erahm/dev/mozilla-central/dom/storage/DOMStorageIPC.cpp, line 320
1 [NNNNN] WARNING: '!compMgr', file /home/erahm/dev/mozilla-central/xpcom/glue/nsComponentManagerUtils.cpp, line 63

There's bug 1171716 for the !mMainThread warning.

[smaug]

There is also DEBUG_foo
and then using it --with-debug-label=foo

There are couple of #ifdef DEBUG_smaug checks, but it isn't really
nice to pollute the code with developer specific ifdefs.
However for your case DEBUG_layout might work.


-Olli

[Eric Rahm]

This does seem more appropriate for warnings targeted at web developers. Most end users probably will not be using debug builds of Firefox (particularly considering we market a "dev edition", you know, for web devs).

[Ehsan Akhgari]

On 2015-06-04 9:40 AM, David Rajchenbach-Teller wrote:

> On 04/06/15 14:30, Ehsan Akhgari wrote:
>> There are very good reasons for warnings to not cause tests to fail. We
>> have a lot of tests that are testing failure conditions that are
>> expected to warn, because they are failure conditions.
>

> Well, that's why the patch linked above offers a whitelist API, so that
> tests can individually decide which families of warnings are either
> expected, or ok-for-the-time-being.

Families of warnings? I don't really understand what you mean. Also,
looking at the patch in the bug, your changes seem to affect Assert.jsm
stuff, which is not related to NS_WARNING.

>> There are also warnings that were actually meant to be assertions, and
>> we should try to convert them to assertions, and that way they will fail
>> tests. That being said, assertion report stack traces in debug builds,
>> which is excruciatingly slow, so we have had to switch them to be
>> warnings in at least some cases (see bug 756045 for example.)
>
> Are you talking about our weird non-fatal assertions or actual fatal
> assertions?

They are both equally slow, but fatal assertions happen only once, by
definition. ;-)

[Boris Zbarsky]

On 6/4/15 6:14 PM, Ehsan Akhgari wrote:
> They are both equally slow, but fatal assertions happen only once, by
> definition. ;-)

Except that for some of our tests we restart after a crash (e.g. for web
platform tests).

So in that test harness, fatal assertions that are hit are much slower
than non-fatal ones, fwiw.

-Boris

[Nicholas Nethercote]

On Thu, Jun 4, 2015 at 1:48 PM, Luke Wagner <lwa...@mozilla.com> wrote:
> In addition to judging noisiness by volume over a whole test run, can we
> also include any warning that happens on normal browser startup, new tab,
> and other vanilla browser operations? This has always annoyed me.

Indeed. Here's an attempt at establishing common ground that hopefully
almost everyone can agree with...

- There should be no warnings when starting and immediately closing
desktop Firefox on a new profile. Or maybe a very small number (e.g.
less than 5) on some configurations... certainly not dozens.

- There should be no warnings that trigger 1000s of times while
running standard test suites.

Because this is just silly:

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

Nick

[David Rajchenbach-Teller]

My patch introduces a new Warnings mechanism for JS. If this works
nicely, I'd like to extend this to C++.

Basically, whenever you encounter a situation that should never happen
in production but that is not quite fatal enough to warrant a crash, we
call (from the top of my head)

Warnings.warn("the module that triggered the warning",
`Hey, I still have ${n} splines to reticulate, I should have 0.`);

By default, this causes tests to fail. However, tests that are designed
to cause this kind of warning can whitelist the warning with

Assert.expect("the module that triggered the warning",
/Hey, I still have [0-9]* splines to reticulate, I should have 0./)
(note the regexp)

and tests that are known to cause this kind of warning can graylist it
until the warning has been fixed

Assert.FIXME("the module that triggered the warning"
/Hey, I still have [0-9]* splines to reticulate, I should have 0./)
(also a regexp)

This API covers all the needs I have encountered for warnings so far in
JS code. I don't think it's terribly different in C++ code.

Cheers,
David

On 05/06/15 00:14, Ehsan Akhgari wrote:
> Families of warnings? I don't really understand what you mean. Also,
> looking at the patch in the bug, your changes seem to affect Assert.jsm
> stuff, which is not related to NS_WARNING.

[ISHIKAWA,chiaki]

On 2015/06/04 21:38, Robert O'Callahan wrote:
> Usually I use NS_WARNING to mean "something weird and unexpected is
> happening, e.g. a bug in Web page code, but not necessarily a browser bug".
> Sometimes I get useful hints from NS_WARNING spew leading up to a serious

> failure, but for those usages could probably be switched to PR_LOG without
> losing much.
>
> Rob
>

After coping with voluminous messages in C-C TB |make mozmill| test
suite log [much smaller volume than full FF logs],
I think we should have NS_INFORMATION() macro that
prints out an information for someone who is developing code and
just wants to know how the code is doing.

So many "WARNING" lines such as the ones below can be turned into
INFORMATION messages and we can probably ignore these lines from NG/GO
decisions of tests.

Hmm. The particular log I picked from two months ago is not
quite representative. I specifically excluded NS_ENSURE_TRUE summary
from below, but for this particular log, the messages near the top of
list (decreasing frequency order) seem too serious to be INFORMATION kind.

> WARNING: Allow one letter C/c as language code.:
The above line below is from my own attempt to
make C-C TB accpet C.UTF-8 as locale: I think cygwin's default locale is
this. But mozilla code refuses to accept this claiming that the first
part of the locale needs to be either two or three letters.
And I used dreaded NS_WARNING() instead of something better: if
NS_INFORMATION() is there, I will use it.

--- begin quote ---
Second without NS_ENSURE macros:

Count/sum is modulo [without the first PID]

1576 [10262] WARNING: '!mMainThread', file
/REF-COMM-CENTRAL/comm-central/mozilla/xpcom/threads/nsThreadManager.cpp, line
395
868 [10262] WARNING: Failed to retarget HTML data delivery to the
parser thread.: file
/REF-COMM-CENTRAL/comm-central/mozilla/parser/html/nsHtml5StreamParser.cpp,
line 951
693 [10262] WARNING: Subdocument container has no frame: file
/REF-COMM-CENTRAL/comm-central/mozilla/layout/base/nsDocumentViewer.cpp,
line 2511
100 [21074] WARNING: We should have hit the document element...:
file /REF-COMM-CENTRAL/comm-central/mozilla/layout/xul/BoxObject.cpp,
line 183
58 [10262] WARNING: unable to post continuation event: file
/REF-COMM-CENTRAL/comm-central/mozilla/xpcom/io/nsStreamUtils.cpp, line 453
58 [10262] WARNING: An event was posted to a thread that will
never run it (rejected): file
/REF-COMM-CENTRAL/comm-central/mozilla/xpcom/threads/nsThread.cpp, line 505
45 [10553] WARNING: No inner window available!: file
/REF-COMM-CENTRAL/comm-central/mozilla/dom/base/nsGlobalWindow.cpp, line
9658
36 [10262] WARNING: Re-registering a CID?: file
/REF-COMM-CENTRAL/comm-central/mozilla/xpcom/components/nsComponentManager.cpp,
line 531
36 [10262] WARNING: Hardware Vsync support not yet implemented.

Falling back to software timers

36 [10262] WARNING: Failed to open external DTD: publicId
"-//W3C//DTD SVG 1.1//EN" systemId
"http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd" base
"file:///REF-COMM-CENTRAL/comm-central/mail/themes/linux/mail/tabs/selected-start.svg"
URL "resource://gre/res/dtd/svg11.dtd": file
/REF-COMM-CENTRAL/comm-central/mozilla/parser/htmlparser/nsExpatDriver.cpp,
line 703
36 [10262] WARNING: Failed to open external DTD: publicId
"-//W3C//DTD SVG 1.1//EN" systemId
"http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd" base
"file:///REF-COMM-CENTRAL/comm-central/mail/themes/linux/mail/tabs/selected-end.svg"
URL "resource://gre/res/dtd/svg11.dtd": file
/REF-COMM-CENTRAL/comm-central/mozilla/parser/htmlparser/nsExpatDriver.cpp,
line 703
36 [10262] WARNING: Allow one letter C/c as language code.: file
/REF-COMM-CENTRAL/comm-central/mozilla/intl/locale/unix/nsPosixLocale.cpp,
line 128
35 [10262] WARNING: unable to Flush() dirty datasource during
XPCOM shutdown: file
/REF-COMM-CENTRAL/comm-central/mozilla/rdf/base/nsRDFXMLDataSource.cpp,
line 754
35 WARNING: YOU ARE LEAKING THE WORLD (at least one JSRuntime and
everything alive inside it, that is) AT JS_ShutDown TIME. FIX THIS!
32 [21074] WARNING: Must complete empty transaction when
compositing!: file
/REF-COMM-CENTRAL/comm-central/mozilla/layout/base/nsPresShell.cpp, line
6257
30 [21074] WARNING: Someone passed native anonymous content

directly into frame construction. Stop doing that!: file

/REF-COMM-CENTRAL/comm-central/mozilla/layout/base/nsCSSFrameConstructor.cpp,
line 6550
28 [21074] WARNING: nsMsgProtocol::SetContentCharset() not
implemented: file
/REF-COMM-CENTRAL/comm-central/mailnews/base/util/nsMsgProtocol.cpp,
line 580
20 [21074] WARNING: anonymous nodes should not be in child lists
(bug 439258): file
/REF-COMM-CENTRAL/comm-central/mozilla/layout/base/RestyleManager.cpp,
line 1441
18 [8115] WARNING: There is no observer for "invalidformsubmit".
One should be implemented!: file
/REF-COMM-CENTRAL/comm-central/mozilla/dom/html/HTMLFormElement.cpp,
line 2005
17 [10262] WARNING: some msg dbs left open: '!m_dbCache.Length()',
file
/REF-COMM-CENTRAL/comm-central/mailnews/db/msgdb/src/nsMsgDatabase.cpp,
line 85
11 [10262] WARNING: Failed to open external DTD: publicId
"-//W3C//DTD SVG 1.1//EN" systemId
"http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd" base
"file:///REF-COMM-CENTRAL/comm-central/mail/themes/linux/mail/tabs/closeTab.svg"
URL "resource://gre/res/dtd/svg11.dtd": file
/REF-COMM-CENTRAL/comm-central/mozilla/parser/htmlparser/nsExpatDriver.cpp,
line 703
10 [24607] WARNING: XPCOM objects created/destroyed from static
ctor/dtor: file
/REF-COMM-CENTRAL/comm-central/mozilla/xpcom/base/nsTraceRefcnt.cpp,
line 132
6 [8115] WARNING: cannot SetMetaDataElement: 'NS_SUCCEEDED(rv)',
file /REF-COMM-CENTRAL/comm-central/mozilla/dom/html/nsHTMLDocument.cpp,
line 765

....

---- end quote ---

Anyway, I agree that we should simply try to attack the top entries
first to figure out whether they are indication of genuine issues/bugs, etc.

Eventually we could cut down the number to manageable size.
(At near the beginning of 2013, the list became only about a dozen items
long, but started to grow again. In a sense it reflects the desire of
code authors to catch strange conditions, but I agree we must keep on
looking at the logs and do something about the top offenders in the log
list.)

TIA

[Ryan VanderMeulen]

Uncaught Promise rejections were made fatal and for the most part go
ignored when they fail intermittently. I'm not a huge fan of adding more
ways for tests to fail if people aren't willing to actually do something
about it when they do.

-Ryan

[Eric Rahm]

On Friday, June 5, 2015 at 8:23:53 AM UTC-7, ISHIKAWA,chiaki wrote:
> After coping with voluminous messages in C-C TB |make mozmill| test
> suite log [much smaller volume than full FF logs],
> I think we should have NS_INFORMATION() macro that
> prints out an information for someone who is developing code and
> just wants to know how the code is doing.

I suppose we could add yet another macro, but really this sounds like a use case for:

MOZ_LOG(myModule, LogLevel::Info, "Dev message!");

which could then be enabled with something like:

|NSPR_MODULES="myModule:3" ./mach run|

It might be useful to eventually add a |MOZ_LOG_IF| to make things easier.

> 1576 [10262] WARNING: '!mMainThread', file
> /REF-COMM-CENTRAL/comm-central/mozilla/xpcom/threads/nsThreadManager.cpp, line

We're working on this in bug 1171716, it's related to cycle collection on shutdown.

> Anyway, I agree that we should simply try to attack the top entries
> first to figure out whether they are indication of genuine issues/bugs, etc.

Yes! Please file blocking bugs for the top c-c entries against bug 765224.

> Eventually we could cut down the number to manageable size.
> (At near the beginning of 2013, the list became only about a dozen items
> long, but started to grow again. In a sense it reflects the desire of
> code authors to catch strange conditions, but I agree we must keep on
> looking at the logs and do something about the top offenders in the log
> list.)

It might be nice to create a very simple start/stop test that just makes sure there are no warnings so that we can avoid regressing that in the future.

[David Rajchenbach-Teller]

The question is what is best: ignored warnings or ignored intermittent
oranges? I assume that the latter have the best chance of being
eventually fixed, but I'm not sure.

On 05/06/15 18:17, Ryan VanderMeulen wrote:
> Uncaught Promise rejections were made fatal and for the most part go
> ignored when they fail intermittently. I'm not a huge fan of adding more
> ways for tests to fail if people aren't willing to actually do something
> about it when they do.
>
> -Ryan

[ISHIKAWA,chiaki]

Hi,

On 2015/06/06 2:39, Eric Rahm wrote:
> On Friday, June 5, 2015 at 8:23:53 AM UTC-7, ISHIKAWA,chiaki wrote:
>> After coping with voluminous messages in C-C TB |make mozmill| test
>> suite log [much smaller volume than full FF logs],
>> I think we should have NS_INFORMATION() macro that
>> prints out an information for someone who is developing code and
>> just wants to know how the code is doing.
>
> I suppose we could add yet another macro, but really this sounds like a use case for:
>
> MOZ_LOG(myModule, LogLevel::Info, "Dev message!");
>
> which could then be enabled with something like:
>
> |NSPR_MODULES="myModule:3" ./mach run|
>

Thank you for the tips.

(I was referring to *NS*_INFORMATION() since
when I tried to use MOZ_ABORT() or something in some test codes,
I could not compile and link them and learned some MOZ_* macros and NS_*
macros don't mix very well during linking(?).)

Anyway, I am a little confused since MOZ_LOG() is very new.

What is the first argument to the following code?

|MOZ_LOG(myModule, LogLevel::Info, "Dev message!");|

Looking at how PR_LOG() was called with PRLogModuleInfo,

typedef struct PRLogModuleInfo {
const char *name;
PRLogModuleLevel level;
struct PRLogModuleInfo *next;
} PRLogModuleInfo;

Is |myModule| it declared as

PRLogModuleInfo myModule;

is passed to as the first argument of MOZ_LOG().?
But it sounds as if we should stay away from PRLog* data structure.

Or is it simply anything (and stringified internally) that
will be specified in environmental variable later?

> It might be useful to eventually add a |MOZ_LOG_IF| to make things easier.
>

Yes, for readability, we need some creative macros.

>> 1576 [10262] WARNING: '!mMainThread', file
>> /REF-COMM-CENTRAL/comm-central/mozilla/xpcom/threads/nsThreadManager.cpp, line
>
> We're working on this in bug 1171716, it's related to cycle collection on shutdown.
>
>> Anyway, I agree that we should simply try to attack the top entries
>> first to figure out whether they are indication of genuine issues/bugs, etc.
>
> Yes! Please file blocking bugs for the top c-c entries against bug 765224.

I will try to do so.

>
>> Eventually we could cut down the number to manageable size.
>> (At near the beginning of 2013, the list became only about a dozen items
>> long, but started to grow again. In a sense it reflects the desire of
>> code authors to catch strange conditions, but I agree we must keep on
>> looking at the logs and do something about the top offenders in the log
>> list.)
>
> It might be nice to create a very simple start/stop test that just makes sure there are no warnings so that we can avoid regressing that in the future.
>

Sounds a sensible idea.

TIA

[Ehsan Akhgari]

On 2015-06-05 6:08 AM, David Rajchenbach-Teller wrote:
> This API covers all the needs I have encountered for warnings so far in
> JS code. I don't think it's terribly different in C++ code.

For C++ non-fatal assertions, we already have a mechanism similar to
this (but it doesn't rely on the exact message printed in the warning,
of course.)

I prefer to not try to do what you are doing for C++ warnings, because
if there are ones that we should really not trigger in a test, those
should be promoted to be assertions, and then we would be able to rely
on the existing mechanisms to catch them during test runs.

Cheers,
Ehsan

[David Rajchenbach-Teller]

Last time I looked at whitelisting non-fatal assertions, there was only
the option to accept up-to a number of assertions. Have I missed
something? Because a blank check to sweep assertions under the carpet is
really an awful mechanism.

Now, I fully agree that warnings that are not whitelisted should most
likely cause real assertion failures.

Cheers,
David

[Boris Zbarsky]

On 6/8/15 4:28 PM, David Rajchenbach-Teller wrote:
> Last time I looked at whitelisting non-fatal assertions, there was only
> the option to accept up-to a number of assertions.

You can specify an exact number or a range.

But you're right that you can't specify the assertion text involved...

-Boris

[David Rajchenbach-Teller]

In my books, that's pretty footgunish.

On 09/06/15 04:04, Boris Zbarsky wrote:
> You can specify an exact number or a range.
>
> But you're right that you can't specify the assertion text involved...
>
> -Boris

> _______________________________________________
> dev-platform mailing list
> dev-pl...@lists.mozilla.org
> https://lists.mozilla.org/listinfo/dev-platform

[Robert O'Callahan]

Does anyone know of a case where we had a regression that traded one
assertion for another? I don't.

[Ryan VanderMeulen]

On 6/9/2015 8:33 AM, Robert O'Callahan wrote:
> Does anyone know of a case where we had a regression that traded one
> assertion for another? I don't.
>
> Rob
>

How would we have found out? :)

[Robert O'Callahan]

On Wed, Jun 10, 2015 at 12:34 AM, Ryan VanderMeulen <rya...@gmail.com>
wrote:

> On 6/9/2015 8:33 AM, Robert O'Callahan wrote:
>
>> Does anyone know of a case where we had a regression that traded one
>> assertion for another? I don't.
>
>

>> How would we have found out? :)

Detecting the regression some other way.

[Ehsan Akhgari]

On 2015-06-09 8:33 AM, Robert O'Callahan wrote:
> Does anyone know of a case where we had a regression that traded one
> assertion for another? I don't.

I don't either. Which is why I believe that David's point, while being
perfectly valid in theory, doesn't matter a lot in practice.

Also, you need to consider the fact that one assertion appearing in
place of another one in the same commits going into one build compared
to the previous not breaking *anything else* or have any observable
effect on all of our test suites is extremely unlikely.

[David Rajchenbach-Teller]

However, I do believe in the following scenario:
- oh, there is an assertion warning, it's not my fault, let's allow one
assertion;
- wait, there are a few, let's allow several;
- oh, they are intermittent, let's make it an interval;
- [at some point, some of the warnings are fixed, but nobody realized];
- [at some point, something causes other warnings to be triggered, but
nobody realized];
- we realize the regression years later.

I am almost sure that I have witnessed this kind of scenario while
working on Session Restore. It used to cause assertion warnings at some
place in the DOM, which were not documented in any bug that I could
find, and nobody cared.

Also, second issue of the current approach: I have seen very few (if
any) bugs yet that documents exactly the assertion warnings that they
trigger, explaining why they are normal and should be ignored. That's a
codetrap for whoever comes next. Requiring a test to actively opt-in for
whitelisting a specific assertion warning means that there is a good
place to r- the test until this is documented.

Frankly, I'm really sick of warnings, and anything we can do to make
them more actionable would be one step towards restoring sanity in our
tests and in some parts of our codebase.

Best regards,
David

[Ehsan Akhgari]

On 2015-06-10 3:38 AM, David Rajchenbach-Teller wrote:
> However, I do believe in the following scenario:
> - oh, there is an assertion warning, it's not my fault, let's allow one
> assertion;
> - wait, there are a few, let's allow several;
> - oh, they are intermittent, let's make it an interval;
> - [at some point, some of the warnings are fixed, but nobody realized];

Yes, assuming the number of assertions doesn't fall outside of the
range, of course.

> - [at some point, something causes other warnings to be triggered, but
> nobody realized];
> - we realize the regression years later.

Yes, like I said, I agree this problem exists _in theory_. I however
think that it's unlikely. (Note that we currently have only 82
SimpleTest.expectAssertions annotations in our tests, so we're already
talking about an edge case of something that is extremely uncommon
itself. :-)

> I am almost sure that I have witnessed this kind of scenario while
> working on Session Restore. It used to cause assertion warnings at some
> place in the DOM, which were not documented in any bug that I could
> find, and nobody cared.

Concrete examples would help. I can't find any test in
browser/components/sessionstore right now with an assertion annotation
so it's hard to say what the exact issue that you saw was, but I think
you are complaining about the lack of documentation and that nobody
cared. Neither of those issues will be solved with what you are suggesting.

> Also, second issue of the current approach: I have seen very few (if
> any) bugs yet that documents exactly the assertion warnings that they
> trigger, explaining why they are normal and should be ignored. That's a
> codetrap for whoever comes next. Requiring a test to actively opt-in for
> whitelisting a specific assertion warning means that there is a good
> place to r- the test until this is documented.

Not necessarily. Reviewers can choose to require exactly that
everywhere SimpleTest.expectAssertions is used (I for one try to do
that, but these are so rare that I can't remember the last time I
reviewed such a test very well...)

My point is, changing |SimpleTest.expectAssertions(2)| to
|SimpleTest.expectAssertions(["first assertion message", "second
assertion message"])| doesn't really give us much besides the ability to
pinpoint the expected assertions to the exact assertions that the test
triggers. And I'm not saying that is not valuable, it's just a solution
to an extremely rare problem.

Issues such as lack of documentation of why the test is triggering an
assertion are orthogonal to the syntax we use to declare the whitelist.

> Frankly, I'm really sick of warnings, and anything we can do to make
> them more actionable would be one step towards restoring sanity in our
> tests and in some parts of our codebase.

Warnings are different. There is basically a spectrum of things you can
do, from setting the MOZ_IGNORE_WARNINGS (and MOZ_QUIET) environment
variables if you're just annoyed with them and don't want to see them,
to filing bugs for the ones that happen way too often in tests (or
during common operations such as opening a tab, typing something in a
form, etc) and CCing the relevant people and asking whether the warning
can be silenced, etc. I very much welcome the latter and have tried to
act on such bugs where I can. Right now we have so many warnings that a
project to annotate every test with every single warning that it
generates is *really* impractical. And once we fix the problem of
having too many frequently occurring one, the value of such a project is
heavily diminished.

Cheers,
Ehsan

[Ryan VanderMeulen]

I'm 99% sure we've had mis-stars on intermittent assertion oranges where
the assertion changed and nobody bothered checking the logs to notice.
Various media tests are coming to mind for when that's happened.

[David Rajchenbach-Teller]

On 10/06/15 16:10, Ehsan Akhgari wrote:
> Concrete examples would help. I can't find any test in
> browser/components/sessionstore right now with an assertion annotation
> so it's hard to say what the exact issue that you saw was, but I think
> you are complaining about the lack of documentation and that nobody
> cared. Neither of those issues will be solved with what you are
> suggesting.

Well, that was at least 2 years ago.

[...]

Anyway, I'm just saying that if/when we redesign our warnings (and
non-fatal assertions are one category of warnings, even if they are not
formally called "warnings"), we need to make sure that warnings are as
actionable as possible. One way to do this is to progressively convert
each the warning into either a real assertion or into something that, by
design, needs to be whitelisted individually (and documented).

Best regards,
David

[Eric Rahm]

An update on progress. I've landed bugs which should clean up ~180,000 warnings. I have bugs pending for another ~26,000.

The latest top 40:

*Note: I improved my normalization a bit so it might look a bit different

18012 [NNNNN] WARNING: Subdocument container has no frame: file layout/base/nsDocumentViewer.cpp, line 2520
14816 [NNNNN] WARNING: NS_ENSURE_SUCCESS(EnsureScriptEnvironment(), nullptr) failed with result 0xNNNNNNNN: file docshell/base/nsDocShell.cpp, line 4605
9809 [NNNNN] WARNING: No docshells for remote frames!: file dom/base/nsFrameLoader.cpp, line 459
8929 [NNNNN] WARNING: Someone passed native anonymous content directly into frame construction. Stop doing that!: file layout/base/nsCSSFrameConstructor.cpp, line 6559
8062 [NNNNN] WARNING: Suboptimal indexes for the SQL statement 0xNNNNNNNN (http://mzl.la/1FuID0j).: file storage/mozStoragePrivateHelpers.cpp, line 109
7760 [NNNNN] WARNING: NS_ENSURE_TRUE(mDocShell) failed: file embedding/browser/nsWebBrowser.cpp, line 363
7454 [NNNNN] WARNING: anonymous nodes should not be in child lists (bug 439258): file layout/base/RestyleManager.cpp, line 1439
6294 [NNNNN] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0xNNNNNNNN: file netwerk/base/nsFileStreams.cpp, line 492
6294 [NNNNN] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0xNNNNNNNN: file netwerk/base/nsFileStreams.cpp, line 205
6131 [NNNNN] WARNING: No outer window available!: file dom/base/nsGlobalWindow.cpp, line 3920
5207 [NNNNN] WARNING: NS_ENSURE_TRUE(domWindow) failed: file embedding/browser/nsDocShellTreeOwner.cpp, line 83
5193 [NNNNN] WARNING: NS_ENSURE_TRUE(aInBrowser) failed: file embedding/browser/nsDocShellTreeOwner.cpp, line 79
5073 [NNNNN] WARNING: zero axis length: file dom/svg/nsSVGLength2.cpp, line 124
4606 [NNNNN] WARNING: No widget found in TabParent::UpdateDimensions: file dom/ipc/TabParent.cpp, line 974
4574 [NNNNN] WARNING: Shouldn't call SchedulePaint in a detached pres context: file layout/generic/nsFrame.cpp, line 5181
3891 [NNNNN] WARNING: Please do not use mouseenter/leave events in chrome. They are slower than mouseover/out!: '!nsContentUtils::IsChromeDoc(d)', file dom/events/EventListenerManager.cpp, line 367
3746 [NNNNN] WARNING: Graph thread slowdown?: 'std::abs(framePosition - CurrentDriver()->StateComputedTime()) < MillisecondsToMediaTime(5)', file dom/media/MediaStreamGraph.cpp, line 1193
3182 [NNNNN] WARNING: Enabling vsync compositor: file gfx/layers/ipc/CompositorParent.cpp, line 674
3042 [NNNNN] WARNING: NS_ENSURE_TRUE(mCallback) failed: file dom/base/nsFrameMessageManager.cpp, line 805
2859 [NNNNN] WARNING: '!mContentCache.CacheEditorRect(this, &aIMENotification)', file widget/PuppetWidget.cpp, line 819
2859 [NNNNN] WARNING: '!editorRectEvent.mSucceeded', file widget/ContentCache.cpp, line 499
2690 [NNNNN] WARNING: nsWindow::GetNativeData not implemented for this type: file widget/PuppetWidget.cpp, line 1058
2527 [NNNNN] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0xNNNNNNNN: file dom/base/nsContentUtils.cpp, line 3712
2526 [NNNNN] WARNING: NS_ENSURE_TRUE(domDoc && target) failed: file dom/base/nsContentUtils.cpp, line 3657
2478 [NNNNN] WARNING: Subdocument container has non-subdocument frame: file layout/base/nsDocumentViewer.cpp, line 2517
2064 [NNNNN] WARNING: NS_ENSURE_TRUE(newRoot) failed: file dom/base/nsRange.cpp, line 1132
2038 [NNNNN] WARNING: NS_ENSURE_TRUE(newRoot) failed: file dom/base/nsRange.cpp, line 1231
1912 [NNNNN] WARNING: NS_ENSURE_TRUE(rootContent) failed: file editor/composer/nsEditorSpellCheck.cpp, line 715
1627 [NNNNN] WARNING: Called close() before start()!: 'mStarted', file dom/workers/MessagePort.cpp, line 215
1612 [NNNNN] WARNING: NS_ENSURE_TRUE(sf) failed: file docshell/base/nsDocShell.cpp, line 6485
1488 [NNNNN] WARNING: NS_ENSURE_TRUE(isFileURI) failed: file dom/base/ThirdPartyUtil.cpp, line 368
1417 [NNNNN] WARNING: NS_ENSURE_SUCCESS(rv, false) failed with result 0xNNNNNNNN: file netwerk/protocol/http/HttpBaseChannel.cpp, line 2077
1417 [NNNNN] WARNING: NS_ENSURE_SUCCESS(result, result) failed with result 0xNNNNNNNN: file docshell/base/nsDocShell.cpp, line 14076
1393 [NNNNN] WARNING: Break suggested inside cluster!: file gfx/thebes/gfxTextRun.cpp, line 220
1288 [NNNNN] WARNING: NS_ENSURE_TRUE(mDisabledJSAndPlugins) failed: file editor/composer/nsEditingSession.cpp, line 209
1175 [NNNNN] WARNING: NS_ENSURE_TRUE(txToRemove) failed: file docshell/shistory/nsSHistory.cpp, line 1318
1151 [NNNNN] WARNING: NS_ENSURE_TRUE(selCon) failed: file editor/libeditor/nsEditor.cpp, line 631
1149 [NNNNN] WARNING: RemoveObserver() called for unregistered observer: file hal/Hal.cpp, line 205
1142 [NNNNN] WARNING: Failed to unlock the wakelock.: '!rv.Failed()', file dom/html/HTMLMediaElement.cpp, line 2341
1113 [NNNNN] WARNING: NS_ENSURE_TRUE(aURI) failed: file netwerk/dns/nsEffectiveTLDService.cpp, line 158

I have patches pending for #1 (bug 1175289) and #5 (bug 1175249) which is also a top offender in the startup warning category.


Top 40 sorted by top level directory:

dom (49710)
9809 [NNNNN] WARNING: No docshells for remote frames!: file dom/base/nsFrameLoader.cpp, line 459
6131 [NNNNN] WARNING: No outer window available!: file dom/base/nsGlobalWindow.cpp, line 3920
5073 [NNNNN] WARNING: zero axis length: file dom/svg/nsSVGLength2.cpp, line 124
4606 [NNNNN] WARNING: No widget found in TabParent::UpdateDimensions: file dom/ipc/TabParent.cpp, line 974
3891 [NNNNN] WARNING: Please do not use mouseenter/leave events in chrome. They are slower than mouseover/out!: '!nsContentUtils::IsChromeDoc(d)', file dom/events/EventListenerManager.cpp, line 367
3746 [NNNNN] WARNING: Graph thread slowdown?: 'std::abs(framePosition - CurrentDriver()->StateComputedTime()) < MillisecondsToMediaTime(5)', file dom/media/MediaStreamGraph.cpp, line 1193
3042 [NNNNN] WARNING: NS_ENSURE_TRUE(mCallback) failed: file dom/base/nsFrameMessageManager.cpp, line 805
2527 [NNNNN] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0xNNNNNNNN: file dom/base/nsContentUtils.cpp, line 3712
2526 [NNNNN] WARNING: NS_ENSURE_TRUE(domDoc && target) failed: file dom/base/nsContentUtils.cpp, line 3657
2064 [NNNNN] WARNING: NS_ENSURE_TRUE(newRoot) failed: file dom/base/nsRange.cpp, line 1132
2038 [NNNNN] WARNING: NS_ENSURE_TRUE(newRoot) failed: file dom/base/nsRange.cpp, line 1231
1627 [NNNNN] WARNING: Called close() before start()!: 'mStarted', file dom/workers/MessagePort.cpp, line 215
1488 [NNNNN] WARNING: NS_ENSURE_TRUE(isFileURI) failed: file dom/base/ThirdPartyUtil.cpp, line 368
1142 [NNNNN] WARNING: Failed to unlock the wakelock.: '!rv.Failed()', file dom/html/HTMLMediaElement.cpp, line 2341

layout (41447)
18012 [NNNNN] WARNING: Subdocument container has no frame: file layout/base/nsDocumentViewer.cpp, line 2520
8929 [NNNNN] WARNING: Someone passed native anonymous content directly into frame construction. Stop doing that!: file layout/base/nsCSSFrameConstructor.cpp, line 6559
7454 [NNNNN] WARNING: anonymous nodes should not be in child lists (bug 439258): file layout/base/RestyleManager.cpp, line 1439
4574 [NNNNN] WARNING: Shouldn't call SchedulePaint in a detached pres context: file layout/generic/nsFrame.cpp, line 5181
2478 [NNNNN] WARNING: Subdocument container has non-subdocument frame: file layout/base/nsDocumentViewer.cpp, line 2517

docshell (19020)
14816 [NNNNN] WARNING: NS_ENSURE_SUCCESS(EnsureScriptEnvironment(), nullptr) failed with result 0xNNNNNNNN: file docshell/base/nsDocShell.cpp, line 4605
1612 [NNNNN] WARNING: NS_ENSURE_TRUE(sf) failed: file docshell/base/nsDocShell.cpp, line 6485
1417 [NNNNN] WARNING: NS_ENSURE_SUCCESS(result, result) failed with result 0xNNNNNNNN: file docshell/base/nsDocShell.cpp, line 14076
1175 [NNNNN] WARNING: NS_ENSURE_TRUE(txToRemove) failed: file docshell/shistory/nsSHistory.cpp, line 1318

embedding (18160)
7760 [NNNNN] WARNING: NS_ENSURE_TRUE(mDocShell) failed: file embedding/browser/nsWebBrowser.cpp, line 363
5207 [NNNNN] WARNING: NS_ENSURE_TRUE(domWindow) failed: file embedding/browser/nsDocShellTreeOwner.cpp, line 83
5193 [NNNNN] WARNING: NS_ENSURE_TRUE(aInBrowser) failed: file embedding/browser/nsDocShellTreeOwner.cpp, line 79

netwerk (15118)
6294 [NNNNN] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0xNNNNNNNN: file netwerk/base/nsFileStreams.cpp, line 492
6294 [NNNNN] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0xNNNNNNNN: file netwerk/base/nsFileStreams.cpp, line 205
1417 [NNNNN] WARNING: NS_ENSURE_SUCCESS(rv, false) failed with result 0xNNNNNNNN: file netwerk/protocol/http/HttpBaseChannel.cpp, line 2077
1113 [NNNNN] WARNING: NS_ENSURE_TRUE(aURI) failed: file netwerk/dns/nsEffectiveTLDService.cpp, line 158

widget (8408)
2859 [NNNNN] WARNING: '!mContentCache.CacheEditorRect(this, &aIMENotification)', file widget/PuppetWidget.cpp, line 819
2859 [NNNNN] WARNING: '!editorRectEvent.mSucceeded', file widget/ContentCache.cpp, line 499
2690 [NNNNN] WARNING: nsWindow::GetNativeData not implemented for this type: file widget/PuppetWidget.cpp, line 1058

storage (8062)
8062 [NNNNN] WARNING: Suboptimal indexes for the SQL statement 0xNNNNNNNN (http://mzl.la/1FuID0j).: file storage/mozStoragePrivateHelpers.cpp, line 109

gfx (4575)
3182 [NNNNN] WARNING: Enabling vsync compositor: file gfx/layers/ipc/CompositorParent.cpp, line 674
1393 [NNNNN] WARNING: Break suggested inside cluster!: file gfx/thebes/gfxTextRun.cpp, line 220

editor (4351)
1912 [NNNNN] WARNING: NS_ENSURE_TRUE(rootContent) failed: file editor/composer/nsEditorSpellCheck.cpp, line 715
1288 [NNNNN] WARNING: NS_ENSURE_TRUE(mDisabledJSAndPlugins) failed: file editor/composer/nsEditingSession.cpp, line 209
1151 [NNNNN] WARNING: NS_ENSURE_TRUE(selCon) failed: file editor/libeditor/nsEditor.cpp, line 631

hal (1149)
1149 [NNNNN] WARNING: RemoveObserver() called for unregistered observer: file hal/Hal.cpp, line 205op doing that!: file layout/base/nsCSSFrameConstructor.cpp, line 6559
7454 [NNNNN] WARNING: anonymous nodes should not be in child lists (bug 439258): file layout/base/RestyleManager.cpp, line 1439
4574 [NNNNN] WARNING: Shouldn't call SchedulePaint in a detached pres context: file layout/generic/nsFrame.cpp, line 5181
2478 [NNNNN] WARNING: Subdocument container has non-subdocument frame: file layout/base/nsDocumentViewer.cpp, line 2517


Log sizes after recent landings:
22M mozilla-central_ubuntu64_vm-debug_test-jsreftest-bm113-tests1-linux64-build6.txt
13M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-2-bm114-tests1-linux64-build27.txt
12M mozilla-central_ubuntu64_vm-debug_test-reftest-e10s-2-bm116-tests1-linux64-build1.txt
12M mozilla-central_ubuntu64_vm-debug_test-reftest-e10s-1-bm68-tests1-linux64-build3.txt
12M mozilla-central_ubuntu64_vm-debug_test-crashtest-bm53-tests1-linux64-build23.txt
9.7M mozilla-central_ubuntu64_vm-debug_test-mochitest-2-bm123-tests1-linux64-build5.txt
7.6M mozilla-central_ubuntu64_vm-debug_test-mochitest-other-bm53-tests1-linux64-build12.txt
7.1M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-3-bm114-tests1-linux64-build4.txt
6.9M mozilla-central_ubuntu64_vm-debug_test-cppunit-bm117-tests1-linux64-build41.txt
6.8M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-2-bm68-tests1-linux64-build3.txt
6.7M mozilla-central_ubuntu64_vm-debug_test-reftest-1-bm113-tests1-linux64-build3.txt
6.5M mozilla-central_ubuntu64_vm-debug_test-reftest-3-bm68-tests1-linux64-build7.txt
6.1M mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-3-bm53-tests1-linux64-build6.txt
6.0M mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-1-bm117-tests1-linux64-build1.txt
5.9M mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-2-bm116-tests1-linux64-build35.txt
5.9M mozilla-central_ubuntu64_vm-debug_test-mochitest-3-bm117-tests1-linux64-build2.txt
5.6M mozilla-central_ubuntu64_vm-debug_test-reftest-2-bm116-tests1-linux64-build11.txt
5.3M mozilla-central_ubuntu64_vm-debug_test-reftest-4-bm122-tests1-linux64-build12.txt
5.2M mozilla-central_ubuntu64_vm-debug_test-mochitest-1-bm122-tests1-linux64-build5.txt
4.7M mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-2-bm68-tests1-linux64-build6.txt
4.6M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-1-bm123-tests1-linux64-build6.txt
4.2M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-4-bm115-tests1-linux64-build18.txt
4.0M mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-1-bm117-tests1-linux64-build31.txt
3.4M mozilla-central_ubuntu64_vm-debug_test-mochitest-5-bm122-tests1-linux64-build0.txt
3.3M mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-4-bm118-tests1-linux64-build12.txt
3.3M mozilla-central_ubuntu64_vm-debug_test-mochitest-4-bm54-tests1-linux64-build6.txt
3.1M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-3-bm54-tests1-linux64-build40.txt
3.0M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-1-bm116-tests1-linux64-build37.txt
2.9M mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-3-bm121-tests1-linux64-build10.txt
2.8M mozilla-central_ubuntu64_vm-debug_test-mochitest-jetpack-bm68-tests1-linux64-build3.txt
2.7M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-5-bm51-tests1-linux64-build6.txt
2.3M mozilla-central_ubuntu64_vm-debug_test-jittest-1-bm115-tests1-linux64-build2.txt
2.2M mozilla-central_ubuntu64_vm-debug_test-jittest-2-bm114-tests1-linux64-build0.txt
1.8M mozilla-central_ubuntu64_vm-debug_test-mochitest-gl-bm123-tests1-linux64-build5.txt
684K mozilla-central_ubuntu64_vm-debug_test-xpcshell-bm52-tests1-linux64-build34.txt
352K mozilla-central_ubuntu64_vm-debug_test-marionette-bm68-tests1-linux64-build4.txt
216K mozilla-central_ubuntu64_vm-debug_test-mochitest-push-bm121-tests1-linux64-build6.txt

And before:
34M mozilla-central_ubuntu64_vm-debug_test-crashtest-bm118-tests1-linux64-build2.txt
33M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-2-bm118-tests1-linux64-build41.txt
23M mozilla-central_ubuntu64_vm-debug_test-reftest-e10s-1-bm122-tests1-linux64-build17.txt
22M mozilla-central_ubuntu64_vm-debug_test-jsreftest-bm116-tests1-linux64-build5.txt
21M mozilla-central_ubuntu64_vm-debug_test-reftest-e10s-2-bm118-tests1-linux64-build29.txt
13M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-1-bm123-tests1-linux64-build2.txt
11M mozilla-central_ubuntu64_vm-debug_test-mochitest-3-bm53-tests1-linux64-build16.txt
9.3M mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-1-bm51-tests1-linux64-build13.txt
8.6M mozilla-central_ubuntu64_vm-debug_test-reftest-1-bm118-tests1-linux64-build5.txt
7.7M mozilla-central_ubuntu64_vm-debug_test-mochitest-other-bm52-tests1-linux64-build4.txt
6.9M mozilla-central_ubuntu64_vm-debug_test-cppunit-bm115-tests1-linux64-build7.txt
6.8M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-3-bm51-tests1-linux64-build4.txt
6.8M mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-3-bm117-tests1-linux64-build2.txt
6.7M mozilla-central_ubuntu64_vm-debug_test-reftest-3-bm123-tests1-linux64-build2.txt
6.4M mozilla-central_ubuntu64_vm-debug_test-reftest-4-bm120-tests1-linux64-build2.txt
5.9M mozilla-central_ubuntu64_vm-debug_test-reftest-2-bm120-tests1-linux64-build30.txt
5.8M mozilla-central_ubuntu64_vm-debug_test-mochitest-2-bm118-tests1-linux64-build11.txt
5.6M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-4-bm118-tests1-linux64-build1.txt
5.6M mozilla-central_ubuntu64_vm-debug_test-mochitest-1-bm123-tests1-linux64-build3.txt
5.4M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-3-bm117-tests1-linux64-build6.txt
5.2M mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-3-bm51-tests1-linux64-build6.txt
5.0M mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-1-bm117-tests1-linux64-build44.txt
4.3M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-1-bm116-tests1-linux64-build31.txt
4.0M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-5-bm121-tests1-linux64-build30.txt
3.9M mozilla-central_ubuntu64_vm-debug_test-mochitest-5-bm120-tests1-linux64-build15.txt
3.4M mozilla-central_ubuntu64_vm-debug_test-mochitest-4-bm67-tests1-linux64-build13.txt
3.3M mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-2-bm115-tests1-linux64-build34.txt
2.9M mozilla-central_ubuntu64_vm-debug_test-mochitest-jetpack-bm117-tests1-linux64-build26.txt
2.4M mozilla-central_ubuntu64_vm-debug_test-mochitest-devtools-chrome-4-bm53-tests1-linux64-build30.txt
2.2M mozilla-central_ubuntu64_vm-debug_test-jittest-2-bm120-tests1-linux64-build30.txt
2.2M mozilla-central_ubuntu64_vm-debug_test-jittest-1-bm68-tests1-linux64-build4.txt
2.0M mozilla-central_ubuntu64_vm-debug_test-mochitest-gl-bm115-tests1-linux64-build16.txt
1.9M mozilla-central_ubuntu64_vm-debug_test-mochitest-browser-chrome-2-bm53-tests1-linux64-build19.txt
1.7M mozilla-central_ubuntu64_vm-debug_test-mochitest-e10s-browser-chrome-2-bm51-tests1-linux64-build6.txt
656K mozilla-central_ubuntu64_vm-debug_test-xpcshell-bm113-tests1-linux64-build2.txt
332K mozilla-central_ubuntu64_vm-debug_test-marionette-bm54-tests1-linux64-build21.txt
196K mozilla-central_ubuntu64_vm-debug_test-mochitest-push-bm54-tests1-linux64-build5.txt

[Nicholas Nethercote]

On Wed, Jun 17, 2015 at 9:29 AM, Eric Rahm <er...@mozilla.com> wrote:
> An update on progress. I've landed bugs which should clean up ~180,000 warnings. I have bugs pending for another ~26,000.

Lovely! Thank you. The log size changes are impressive.

> Top 40 sorted by top level directory:

I couldn't help but notice that js/ does not appear there, simply
because SpiderMonkey doesn't *have* runtime warnings...

Nick

[kgu...@mozilla.com]

> 4606 [NNNNN] WARNING: No widget found in TabParent::UpdateDimensions: file
> dom/ipc/TabParent.cpp, line 974

Do you know if this one occurs on b2g or also on other platforms? I added this warning recently in bug 1125325 after smaug said [1]. It seems to be happening a lot, so we should investigate. If you have a bug on file for it please cc me.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1125325#c134

[Milan Sreckovic]

Do we have a all encompassing meta bug to collect all of the bugs that fix the warnings?
—
- Milan

[Eric Rahm]

On Wednesday, June 17, 2015 at 7:40:14 AM UTC-7, Milan Sreckovic wrote:
> Do we have a all encompassing meta bug to collect all of the bugs that fix the warnings?

> --
> - Milan
>

Bug 765224

[Eric Rahm]

These are all within mochitest-e10s-browser-chrome tests, which AFAICT are not run on B2G, OSX, Windows debug builds. Filed bug 1175631 for the verbose warning.

[ISHIKAWA,chiaki]

On 2015/06/17 8:29, Eric Rahm wrote:
> An update on progress. I've landed bugs which should clean up ~180,000 warnings. I have bugs pending for another ~26,000.
>
> The latest top 40:
>
> *Note: I improved my normalization a bit so it might look a bit different
>

Are there documents explaining why
these warnings are generated?
I mean, say, I see the first warning during the run of C-C TB's |make
mozmill| test and I wonder, what C-C TB is doing wrong to see this
"Subdocument container has no frame"?

Maybe each month, we should pick up the top-10 warnings (without the
description given below) in the frequency, and ask the author of the
patch that inserted the warning to
explain the following: this will help the testers to asses the
situation, and more importantly, help people maintain the legacy code
such C-C TB to figure out what can be possibly wrong in the existing
code and how we can fix them.:

- severity of the message: INFO, DEBUG, WARNING, etc.
- explain why there is a warning (background),
- is there going to be a bad thing happening soon after
the situation occurs? what bad symptoms should we expect?
- what can a caller do to prevent the bad thing happening (I mean
the caller of the invocation chain that eventually leads to the
particular warning.)

Without such information, warnings are not so useful in the log.

I see about 25 of the following warnings in C-C TB |make mozmill| run.
And I have been wondering "OK, if Subdocument container has no frame",
is this bad?, is there going to be something bad?, is this message
printed because C-C TB code invokes document API incorrectly?, if so,
what is the correct procedure, and such.

As a few mentioned that there are simply so many warnings in the log and
we are overwhelmed, but anything that can help us to cope with the tidal
wave of warnings ought to be implemented and tried.

And among the warnings, there *ARE* warnings that lead to real bugs.
e.g.:

> 1627 [NNNNN] WARNING: Called close() before start()!: 'mStarted',
file dom/workers/Message

Obviously, the program is sloppy and calling
some functions in incorrect order. We should fix this.

> 1149 [NNNNN] WARNING: RemoveObserver() called for
unregistered observer: file hal/Hal.cpp, line 205

Again, the program is sloppy and is not keeping track of
its resources and thus calling non-existing, phantom observer
to be removed. This ought to be fixed.

Another real bug is this.
Although it did not seem to be in the top 40, I see a few types of
messages that make me thing that the shutting down of thread are not
properly synchronized at the end of the program termination.
Obviously some threads try to dump data while the subsystem/module that
handles the writing of the data is already closed/shutdown and the
writing fails, etc. This ought to be fixed.

These high volume warnings can be very useful if only the description
about them can be found easily.

I am not claiming the description for EVERY warning, but
we can start with top-most 10 warnings (without such description).