1344848 - Don't check for atom leaks unless we're checking for other leaks

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

It doesn't make sense to check for atom leaks unless we're checking for other leaks, or random things can set it off. For instance, we don't check for leaks in xpcshell. The existing code tries to deal with this by only running if we're in a shutdown GC, which only run in debug builds, but we don't do leak checking for all debug tests, such as xpcshell, so we can hit leaks we don't care about. I think a better thing to check for would be to see if XPCOM_MEM_BLOAT_LOG is set.

Comment 1

[(no longer active)]

Can we do this in a way that it would only affect *test runs* and not other runs of xpcshell?  I'm asking because if we did this blindly, it would have masked bug 1343863 which happened during mach package where we run xpcshell.  That would be unfortunate as that turned out to be a real leak in our code that would be possible to be hit by a real user.

Comment 2

[Andrew McCreight [:mccr8]]

(In reply to :Ehsan Akhgari from comment #1)
> Can we do this in a way that it would only affect *test runs* and not other
> runs of xpcshell?

No. It just happens that in your case the leak was useful information, but it could have just as easily been a false positive, like you were seeing in a number of other tests.

Comment 3

[Andrew McCreight [:mccr8]]

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

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Bug 1344848 - Don't check for atom leaks unless we're checking for other leaks.

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

Comment 5

[(no longer active)]

(In reply to Andrew McCreight [:mccr8] from comment #2)
> (In reply to :Ehsan Akhgari from comment #1)
> > Can we do this in a way that it would only affect *test runs* and not other
> > runs of xpcshell?
> 
> No. It just happens that in your case the leak was useful information, but
> it could have just as easily been a false positive, like you were seeing in
> a number of other tests.

OK, fair enough.

Comment 6

[Eric Rahm [:erahm]]

Comment on attachment 8844145 [details]
Bug 1344848 - Don't check for atom leaks unless we're checking for other leaks.

https://reviewboard.mozilla.org/r/117682/#review119412

::: xpcom/ds/nsAtomTable.cpp:394
(Diff revision 1)
>        i.Remove();
>        delete atom;
>        ++removedCount;
> -    } else if (aKind == GCKind::Shutdown) {
> -      // We only perform these kind of GCs in leak-checking builds.  If
> -      // something is anomalous, then we'll report an error here, and crash
> +    }
> +#ifdef NS_FREE_PERMANENT_DATA
> +    else if (aKind == GCKind::Shutdown && PR_GetEnv("XPCOM_MEM_BLOAT_LOG")) {

Is `XPCOM_MEM_BLOAT_LOG` really the only leak checking env var we have?

Comment 7

[Andrew McCreight [:mccr8]]

(In reply to Eric Rahm [:erahm] from comment #6)
> Is `XPCOM_MEM_BLOAT_LOG` really the only leak checking env var we have?

There are a hodge-podge of others, which you can see in InitTraceLog(). BLOAT_LOG is the only one we use in testing, I think.

Comment 8

[Eric Rahm [:erahm]]

Comment on attachment 8844145 [details]
Bug 1344848 - Don't check for atom leaks unless we're checking for other leaks.

https://reviewboard.mozilla.org/r/117682/#review119416

Discussed in IRC, it should be fine to be less spammy in non-`XPCOM_MEM_BLOAT_LOG` runs. As noted in the comment our assertion still runs and does not rely on `nonZeroRefcountAtoms`.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7818d19d769d
Don't check for atom leaks unless we're checking for other leaks. r=erahm

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f248e1f814d1
Don't check for atom leaks unless we're checking for other leaks. r=erahm

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f248e1f814d1

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7818d19d769d

Comment 13

[Jorg K (CEST = GMT+2)]

Any chance to get this uplifted? That atom leak check was introduced in FF 54 and uplifted to 53 and 52 (incl. 52 ESR). Since despite our best efforts we still have some leaks in TB, we still have failing tests due to this (we fixed 10 of initially 14 broken tests). So uplift at far as possible would be appreciated. It doesn't look like a risky patch at all.

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 8844145 [details]
Bug 1344848 - Don't check for atom leaks unless we're checking for other leaks.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1276669
[User impact if declined]: No direct user impact, but this will fix Thunderbird test failures.
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: This just weakens a debug-only check.
[String changes made/needed]: none

Comment 15

[Gerry Chang [:gchang]]

Comment on attachment 8844145 [details]
Bug 1344848 - Don't check for atom leaks unless we're checking for other leaks.

Take this to help fix Thunderbird test failures. Aurora54+.

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e8fdbc7507a7

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8844145 [details]
Bug 1344848 - Don't check for atom leaks unless we're checking for other leaks.

disable a debug-only check in some cases, beta53+, esr52+

Comment 18

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

https://hg.mozilla.org/releases/mozilla-beta/rev/d5d60e222153

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/470747bf4481

Comment 20

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Andrew's assessment on manual testing needs (see Comment 14) and the fact that this fix has automated coverage.