1353858 - LSAN leak checking is broken on Fx55

Status: RESOLVED FIXED

(Testing :: Mochitest, defect)

Description

[Ryan VanderMeulen [:RyanVM]]

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

Comment 1

[Ryan VanderMeulen [:RyanVM]]

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

Comment 2

[James Graham [:jgraham]]

Attachment: Bug 1353858 - Fix ASAN leak detection in mochitest,

The gecko messages are now in the "process_output" action, rather than
in the "log" action (except for a few legacy cases), so examine both
when looking for LSAN messages.

Review commit: https://reviewboard.mozilla.org/r/127226/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/127226/

Comment 3

[Marcia Knous [:marcia]]

Tracking 55+ for this regression.

Comment 4

[Andreas Tolfsen ❲:ato❳]

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?

Comment 5

[James Graham [:jgraham]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/127226/diff/1-2/

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/228aac2de997
Fix ASAN leak detection in mochitest,  r=ato

Comment 7

[James Graham [:jgraham]]

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.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Indeed it is! Looks good, James :)

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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