"unsafe CPOW usage" warnings are too noisy in automation test logs

RESOLVED FIXED in Firefox 40

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: mconley, Assigned: mconley)

Tracking

Trunk
mozilla40
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(2 attachments, 1 obsolete attachment)

Our tree sheriffs often look at test logs for e10s, and unfortunately, many of our tests trigger unsafe CPOW usage warnings because they haven't been ported to use async messages yet.

Most of these have been filed as blocking bug 1109869, but in the mean time, our sheriffs have to deal with a ton of log spam, and that really sucks.

As an interim solution, we would like to investigate filtering out "unsafe CPOW usage" warnings from the log viewer. RyanVM has told me that this would be an OK interim solution until we drill bug 1109869 into the ground.
I don't think this is something that we can easily do (or would want to do), particularly with the current log viewer. At some point in the future, when we use the json (aka structured) logs with a new log viewer, it can perhaps do collapsing by test or subtest/filtering/... - however even then, I don't think we'd want to hide things by default - it would be up to the user to filter each time - which doesn't really help this use case. In addition, it still leaves the full log and local dev console pretty cluttered.

As such, I think for now this is best dealt with at the console/logging/harness level instead - eg "only report the first N warnings for each file/type" or summarise them "55 warnings in file X". So this either belongs in an e10s component, or a testing/harness component. Another option would be to only show the warnings in local jobs, or to say ask the mozharness people to add another log level that we can get the log viewer to ignore.
OS: Mac OS X → All
Hardware: x86 → All
Summary: Filter "unsafe CPOW usage" warnings → "unsafe CPOW usage" warnings are too noisy in automation test logs
(Assignee)

Comment 2

4 years ago
Alternatively, and this is something RyanVM just suggested to me, we could disable all unsafe CPOW usage warnings for test runs except for a particular project branch (like Holly, which we already use for e10s tests, I believe).

What do you think of that last idea, jimm? RyanVM suspects that filtering out the unsafe CPOW usage entries from the log view the treeherder logviewer is non-trivial. Do you know if we have any run or build-time prefs that we can use to disable the warnings just for test runs?
(Assignee)

Updated

4 years ago
Flags: needinfo?(jmathies)
Component: Treeherder: Log Viewer → General
Product: Tree Management → Testing
Version: --- → Trunk
I'm not aware of any test specific environment variable we currently set that could be used here, but you can add one.. check out testing/mochitest/runtests.py - 

http://mxr.mozilla.org/mozilla-central/source/testing/mochitest/runtests.py#1456
Flags: needinfo?(jmathies)
(Assignee)

Comment 4

4 years ago
Created attachment 8590972 [details]
MozReview Request: bz://1152864/mconley (r+'d by jimm and ahal)

/r/6901 - Bug 1152864 - Add DISABLE_UNSAFE_CPOW_WARNINGS environment variable for our test harnesses. r=?
/r/6903 - Bug 1152864 - Disable unsafe CPOW warnings when running mochitests by default, and add option to re-enable. r=?

Pull down these commits:

hg pull -r 19243372d98892edb673d4855938b86020ef7428 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8590972 - Flags: review?(jmathies)
Attachment #8590972 - Flags: review?(ahalberstadt)
(Assignee)

Comment 5

4 years ago
So this patch will do the job of suppressing all unsafe CPOW warnings when running mochitests, and adds a --enable-CPOW-warnings argument to the mach mochitest-* commands (and runtests.py) to turn them back on.

I tested this locally with browser/base/content/test/general/browser_bug575561.js as a test case, and the warnings are gone (unless I flip them on with --enable-CPOW-warnings).
(Assignee)

Comment 6

4 years ago
Comment on attachment 8590972 [details]
MozReview Request: bz://1152864/mconley (r+'d by jimm and ahal)

/r/6901 - Bug 1152864 - Add DISABLE_UNSAFE_CPOW_WARNINGS environment variable for our test harnesses. r=?
/r/6903 - Bug 1152864 - Disable unsafe CPOW warnings when running mochitests by default, and add option to re-enable. r=?

Pull down these commits:

hg pull -r 19243372d98892edb673d4855938b86020ef7428 https://reviewboard-hg.mozilla.org/gecko/
https://reviewboard.mozilla.org/r/6903/#review5761

::: testing/mochitest/mach_commands.py
(Diff revision 1)
> +    enableCPOWWarnings = CommandArgument(
> +        '--enable-CPOW-warnings',
> +        action='store_true',
> +        help='Run tests with unsafe CPOW usage warnings enabled.')
> +    func = enableCPOWWarnings(func)

This is fine for now, but how common do you think enabling this locally is going to be? I'm trying to improve the UX of running tests a bit, and part of that will be cleaning rarely used options out of the command line. These rarer options will still be configurable.. just more out of the way.
(Assignee)

Comment 8

4 years ago
(In reply to Andrew Halberstadt [:ahal] from comment #7)
> https://reviewboard.mozilla.org/r/6903/#review5761
> 
> ::: testing/mochitest/mach_commands.py
> (Diff revision 1)
> > +    enableCPOWWarnings = CommandArgument(
> > +        '--enable-CPOW-warnings',
> > +        action='store_true',
> > +        help='Run tests with unsafe CPOW usage warnings enabled.')
> > +    func = enableCPOWWarnings(func)
> 
> This is fine for now, but how common do you think enabling this locally is
> going to be?

Very uncommon to never. The only use-case I can think of, to enable this for tests, is for the e10s team to sample the logs to determine if there are still places where we're using unsafe CPOWs. At this stage, I don't see us doing that very often, though that might change in the next quarter (Milestone 8 of e10s involves us drilling into a bunch of these unsafe CPOW usage warnings).
(Assignee)

Comment 9

4 years ago
https://reviewboard.mozilla.org/r/6903/#review5763

> This is fine for now, but how common do you think enabling this locally is going to be? I'm trying to improve the UX of running tests a bit, and part of that will be cleaning rarely used options out of the command line. These rarer options will still be configurable.. just more out of the way.

Ah, whoops, responded in the bug: https://bugzilla.mozilla.org/show_bug.cgi?id=1152864#c8
https://reviewboard.mozilla.org/r/6903/#review5765

Ok I'll likely hide this from the help in the near future, though it'll still be an option should the e10s team need it for debugging. In the meantime, this looks good to me!

Updated

4 years ago
Attachment #8590972 - Flags: review?(jmathies) → review+
(Assignee)

Updated

4 years ago
Attachment #8590972 - Attachment description: MozReview Request: bz://1152864/mconley → MozReview Request: bz://1152864/mconley (r+'d by jimm and ahal)
Attachment #8590972 - Flags: review?(ahalberstadt) → review+
(Assignee)

Comment 12

4 years ago
Thanks! I switched the argument to --enable-cpow-warnings as jimm suggested.

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/93f617ada149
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/40a21375b99b
https://hg.mozilla.org/mozilla-central/rev/93f617ada149
https://hg.mozilla.org/mozilla-central/rev/40a21375b99b
Assignee: nobody → mconley
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
(Assignee)

Comment 14

4 years ago
Comment on attachment 8590972 [details]
MozReview Request: bz://1152864/mconley (r+'d by jimm and ahal)
Attachment #8590972 - Attachment is obsolete: true
Attachment #8620010 - Flags: review+
Attachment #8620011 - Flags: review+
(Assignee)

Comment 15

4 years ago
Created attachment 8620010 [details]
MozReview Request: Bug 1152864 - Add DISABLE_UNSAFE_CPOW_WARNINGS environment variable for our test harnesses. r=?
(Assignee)

Comment 16

4 years ago
Created attachment 8620011 [details]
MozReview Request: Bug 1152864 - Disable unsafe CPOW warnings when running mochitests by default, and add option to re-enable. r=?
You need to log in before you can comment on or make changes to this bug.