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

RESOLVED FIXED in Firefox -esr52

Status

()

Core
XPCOM
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

unspecified
mozilla55
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 fixed, firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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

a year ago
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.
(Assignee)

Comment 2

a year ago
(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 hidden (mozreview-request)

Comment 5

a year ago
(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 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?
(Assignee)

Comment 7

a year ago
(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 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`.
Attachment #8844145 - Flags: review?(erahm) → review+

Comment 9

a year ago
Pushed by amccreight@mozilla.com:
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

a year ago
Pushed by eakhgari@mozilla.com:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f248e1f814d1
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
Flags: needinfo?(erahm)
Flags: needinfo?(continuation)
(Assignee)

Comment 14

a year ago
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
Flags: needinfo?(erahm)
Flags: needinfo?(continuation)
Attachment #8844145 - Flags: approval-mozilla-esr52?
Attachment #8844145 - Flags: approval-mozilla-beta?
Attachment #8844145 - Flags: approval-mozilla-aurora?
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox-esr52: --- → affected
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+.
Attachment #8844145 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 16

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/e8fdbc7507a7
status-firefox54: affected → fixed
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+
Attachment #8844145 - Flags: approval-mozilla-esr52?
Attachment #8844145 - Flags: approval-mozilla-esr52+
Attachment #8844145 - Flags: approval-mozilla-beta?
Attachment #8844145 - Flags: approval-mozilla-beta+

Comment 18

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/d5d60e222153
status-firefox53: affected → fixed

Comment 19

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/470747bf4481
status-firefox-esr52: affected → fixed
Setting qe-verify- based on Andrew's assessment on manual testing needs (see Comment 14) and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.