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.

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

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.

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

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.

> 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.

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.

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.

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.

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 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 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!!

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

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

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.

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

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

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

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...

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.

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