1068276 - Fail when there's no leak log, for default and plugin processes

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Andrew McCreight [:mccr8]]

It turns out this was already broken at some point in the past on B2G M9, so I think I should try enabling this just for the main process and plugin processes so we can at least catch regressions there.  It will need to be disabled for B2G, because we don't produce logs there, because when we do produce logs there, they have a larger leak than other tests.

Comment 1

[Andrew McCreight [:mccr8]]

Without bug 962871, we fail to produce a leak log on B2G M9.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Fail if there's no leak log for default and plugin processes.

We'll have to see if this still has problems with causing intermittent failures.  The blocking bugs have patches, so hopefully I'll be able to land this at next week at some point.

Comment 3

[Andrew McCreight [:mccr8]]

try run: https://tbpl.mozilla.org/?tree=Try&rev=ac371ef198ed

Comment 4

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8492448 [details] [diff] [review]
Fail if there's no leak log for default and plugin processes.

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

just a few nits.

::: build/automationutils.py
@@ +276,5 @@
> +    elif processType == "tab" or processType == "geckomediaplugin":
> +      # Bug 1051230 - Leak logging does not yet work for content processes on desktop.
> +      # Bug 1070068 - Leak logging does not yet work for content processes on B2G.
> +      # Bug 1065098 - The geckomediaplugin process fails to produce a leak log for some reason.
> +      log.info("TEST-INFO | leakcheck |%s ignoring missing output line for total leaks"

s/|%s/| %s/

@@ +282,2 @@
>      else:
> +      log.info("TEST-UNEXPECTED-FAIL | leakcheck |%s missing output line for total leaks!"

and here also, lets keep spaces on both sides of the |

@@ +284,2 @@
>                 % processString)
> +      log.info("TEST-INFO | leakcheck | missing output line from log file " + leakLogFileName)

can you use %s for the leakLogFileName instead of a + ?  It would keep things consistent.

Comment 5

[Andrew McCreight [:mccr8]]

> s/|%s/| %s/

The way I have it is actually correct, because processString has a leading space.  But yeah, that is confusing, and a holdover from before I changed things in bug 1069689.  I'll write a patch somewhere that gets rid of it.

> can you use %s for the leakLogFileName instead of a + ?  It would keep things consistent.
Good point, I'll do that.

Comment 6

[Andrew McCreight [:mccr8]]

I thought bug 962871 would fix the b2g log failure, but it looks like it just pushed it to the next test suite, which frankly makes more sense.  I'll have to file a bug for that failure, and I'll have to add a way to disable this checking on B2G so we can at least get it working for desktop and Android.

Comment 7

[Andrew McCreight [:mccr8]]

New plan: change this so we can configure whether we should fail when there's no log or not, disable it for B2G and tab processes on desktop.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Allow configuring which type of processes we complain about when there's no leak log.

This adds a new argument to the leak log processing function that is a set of process types that we should ignore errors for. The "set" is implemented as a list because browser chrome was complaining that it couldn't JSON serialize an actual Python set.  There are only a few things in the set so it shouldn't matter.

This sets things up so they match the current sad state of affairs, which is that we never get a leak log for tab or geckomediaplugin processes, and don't always get them for default processes on B2G. When these things are fixed, we'll have to remove them from the list.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Make unexpected failure to produce a leak log an error.

This patch just changes the warning when no log is produced when we weren't expecting it. I'm mostly splitting this into a separate patch in case it turns out to produce intermittent failures so it can be backed out more easily, but I haven't seen any yet, so maybe the basic infrastructure is just better now.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8495386 [details] [diff] [review]
part 1 - Allow configuring which type of processes we complain about when there's no leak log.

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

looks good!

Comment 11

[Joel Maher ( :jmaher ) (UTC -8)]

Comment on attachment 8495389 [details] [diff] [review]
part 2 - Make unexpected failure to produce a leak log an error.

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

nice!!

Comment 12

[Andrew McCreight [:mccr8]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/35e71ffcf8e1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/846faaa68219

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Backed out while mccr8 investigates some Android issues that showed up in the logs.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bc6dfe614be

Comment 14

[Andrew McCreight [:mccr8]]

Thanks for backing these out, Ryan.

Android has its own way to invoke the test harness which needs to set the new options I added:
  https://tbpl.mozilla.org/php/getParsedLog.php?id=48989234&tree=Mozilla-Inbound

Now, we don't actually do leak checking on Android (I think) but I should still fix that.

Comment 15

[Andrew McCreight [:mccr8]]

Attachment: part 1b - Set up dummy defaults for Android reftests for ignoreMissingLeaks.

I'll land this folded into part 1.

None of the errors that caused the backout showed up in this try run.  See bug 1067664 for more details.

try run: https://tbpl.mozilla.org/?tree=Try&rev=409ec0a94407

Comment 16

[Andrew McCreight [:mccr8]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7922f06116c8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/9b544d98846d

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7922f06116c8
https://hg.mozilla.org/mozilla-central/rev/9b544d98846d

Comment 18

[Andrew McCreight [:mccr8]]

Attachment: add some basic defaults for B2G reftests

RyanVM pointed out that b2g reftests are also throwing an exception.  Yay.  We don't do leak checking there either, so just set some basic defaults.  I think this will be the last one...

Comment 19

[Andrew McCreight [:mccr8]]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6e6a4b63628f
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/5bed1b3e2da6

Ted noticed that the intialization of the variables for reftests was indented incorrectly, so it wasn't being set.  He gave me r+ over IRC to fix that.

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6e6a4b63628f
https://hg.mozilla.org/mozilla-central/rev/5bed1b3e2da6