Open Bug 1341961 (xpcshell-leaks) Opened 3 years ago Updated 24 days ago

Turn on leak checking in xpcshell tests

Categories

(Testing :: XPCShell Harness, defect, P3)

Version 3
defect

Tracking

(Not tracked)

People

(Reporter: ehsan, Unassigned)

References

(Depends on 4 open bugs)

Details

(Whiteboard: [MemShrink:P2])

Bug 1341954 wasted a lot of time today, all because we had some xpcshell test that was leaking.  We should turn on leak checking on these tests and fix the existing leaks to prevent this kind of thing from biting other developers in the future.
We have had bug 469523 open for a while. It gets a little weird because the xpcshell environment is significantly different than the browser environment, so it doesn't fire all of the same observer service notifications etc, so code may not run cleanup that it would in the browser without additional prodding.

I haven't looked at it in a long time, but I would bet that if a person with experience debugging leaks spent some time on it we could make the existing tests leak-free.
(In reply to Ted Mielczarek [:ted.mielczarek] from comment #1)
> We have had bug 469523 open for a while. It gets a little weird because the
> xpcshell environment is significantly different than the browser
> environment, so it doesn't fire all of the same observer service
> notifications etc, so code may not run cleanup that it would in the browser
> without additional prodding.

So we should fix it in cases where the divergence causes leaks.  :-)

> I haven't looked at it in a long time, but I would bet that if a person with
> experience debugging leaks spent some time on it we could make the existing
> tests leak-free.

We don't necessarily need a lot of experience, anyone should be able to follow https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Refcount_tracing_and_balancing to find the cause of the leaks and fix them.  It's actually much easier for someone who understands the underlying code to do that than the random developer such as myself (I had to spend hours double checking everything with the HTTP redirect code to make sure the refcount doesn't get imbalanced there, would have probably taken minutes if a better person was asked to do that work.)
Ted, how can we make it so that this gets worked on and fixed?
Flags: needinfo?(ted)
Sorry if it's going to be unfeasible, but at this stage wouldn't it be more important to disallow new leaks from appearing, than to wait for all the existing ones to be fixed? The risk is another instance of bug 469523 that starts looking at the issue but never completes.
I'm wondering if it wouldn't be possible to already enable leak checking but provide to it a whitelist of tests that are known to be leaking. That would prevent from adding new leaking tests and once old tests are fixed they can just be removed from the WL. There should be a clear rule for the WL, either no addition or only if reviewed by someone specific.
(In reply to :Ehsan Akhgari from comment #3)
> Ted, how can we make it so that this gets worked on and fixed?

I don't have any answers for you, I'm afraid. The best thing we can do is probably along the lines of what Marco says--carve out some beachhead of known failures and disallow other failures. We do have test manifests nowadays, so it's fairly easy to add per-test annotations in support of something like this.
Flags: needinfo?(ted)
Yes, I wasn't really asking about how it can be done (Marco's suggestion is definitely the way to go), more about whether you can help find someone on the A-Team who can own it...  Preferably without me having to spend time on it.  I would gladly do that (as I did a bit since it blocked me from landing bug 1340710) but I can't justify working on non-Quantum projects right now.  :/
Flags: needinfo?(ted)
I don't think we have anyone on A-Team with time to do this unless it's blocking progress on major items like Quantum.
Flags: needinfo?(ted)
Priority: -- → P3
Depends on: 1445392
Depends on: 883720
Duplicate of this bug: 469523
Whiteboard: [MemShrink]
Depends on: 449240
Alias: xpcshell-leaks
I couldn't figure out how to set an environment variable in XPCShell, so I edited nsTraceRefcnt.cpp to always create a bloat log. When I ran a few tests, we leaked a single nsTArray_base, but nothing more, so getting this wouldn't be a hopeless affair. I don't have any idea who would actually work on it, though.
I'm assuming you only care about setting the environment for xpcshell processes running in the xpcshell test harness, in which case you can do that here:
https://dxr.mozilla.org/mozilla-central/rev/0cd106a2eb78aa04fd481785257e6f4f9b94707b/testing/xpcshell/runxpcshelltests.py#960
Whiteboard: [MemShrink] → [MemShrink:P2]
Depends on: 1570603
Depends on: 1570666
You need to log in before you can comment on or make changes to this bug.