Ryan VanderMeulen [:RyanVM] Reporter
	Description • 7 years ago

[Tracking Requested - why for this release]: Broken leak checking on 55.

After enabling e10s-multi on Aurora yesterday, we were hit by a permaleak in browser_devices_get_user_media_multi_process.js. After further investigation, it turns out that the leaks are also hitting on trunk but aren't causing the run to go orange. The most obvious suspect is bug 1344346, where the logging format changed.

Example failing run from Aurora:
https://queue.taskcluster.net/v1/task/V5CrMtgORf-HN2uuv1PNPA/runs/0/artifacts/public/logs/live_backing.log

Trunk log showing the same failures on a green push:
https://public-artifacts.taskcluster.net/Zc6EhGGhRHKlCFOYdRrtgg/0/public/logs/live_backing.log

James, can you please take a look at this since ahal is out? And is there any way for us to add tests for this so we don't break it again? This isn't the first time logging changes have broken LSAN, sadly.

	Ryan VanderMeulen [:RyanVM] Reporter
	Comment 1 • 7 years ago

FYI, I just skipped browser_devices_get_user_media_multi_process.js over in bug 1347625, but you can revert the below commit if you need a way to test any changes you make for effectiveness.

https://hg.mozilla.org/integration/mozilla-inbound/rev/72c867371b300dc90dc5dfbe6bbf40377af6644e

	Marcia Knous [:marcia]
	Comment 3 • 7 years ago

Tracking 55+ for this regression.

	Andreas Tolfsen ❲:ato❳
	Comment 4 • 7 years ago
mozreview-review

Comment on attachment 8855375 [details]
Bug 1353858 - Fix ASAN leak detection in mochitest,

https://reviewboard.mozilla.org/r/127226/#review129976

There’s no try run associated with this bug, but assuming it doesn’t break anything, this looks sensible.  r+ if you’re confident the removal of the self.lsanLeaks existence check is safe and it doesn’t break the build.

::: commit-message-facaf:1
(Diff revision 1)
> +Bug 1353858 - Fix ASAN leak detection in mochitest,  r=ato

I guess this commit message will be rewritten by Otto Länd, but there is a superfluous after the comma (?) in the first line.

::: testing/mochitest/runtests.py:2724
(Diff revision 1)
>                      'status'] == 'FAIL':
>                  self.harness.dumpScreen(self.utilityPath)
>              return message
>  
>          def trackLSANLeaks(self, message):
> -            if self.lsanLeaks and message['action'] == 'log':
> +            if message['action'] in ('log', 'process_output'):

Other methods in this class check that self.lsanLeaks is set, but you remove this check.  Is this a safe change?

	Pulsebot
	Comment 6 • 7 years ago

Pushed by james@hoppipolla.co.uk:
https://hg.mozilla.org/integration/autoland/rev/228aac2de997
Fix ASAN leak detection in mochitest,  r=ato

	James Graham [:jgraham] Assignee
	Comment 7 • 7 years ago

I couldn't actually reproduce the issue (not sure if I had the right try options), but I'm 95% sure this is the right fix, and certainly not worse than the pre-patch state. Since 95% isn't really all that sure, I would appreciate it if someone who has used this before could verify for me.

	Ryan VanderMeulen [:RyanVM] Reporter
	Comment 8 • 7 years ago

Try push with the two other fixes recently landed on Autoland backed out to see what happens. With any luck, it'll be leaktastic.

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

	Ryan VanderMeulen [:RyanVM] Reporter
	Comment 9 • 7 years ago

(In reply to Ryan VanderMeulen [:RyanVM] from comment #8)
> With any luck, it'll be leaktastic.

Indeed it is! Looks good, James :)

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 10 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/228aac2de997