Turn on leak checking in xpcshell tests

NEW
Unassigned

Status

Testing
XPCShell Harness
a year ago
a year ago

People

(Reporter: Ehsan, Unassigned)

Tracking

(Depends on: 1 bug)

Version 3
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Reporter)

Description

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

Comment 2

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

Updated

a year ago
(Reporter)

Comment 3

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

Comment 6

a year ago
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)
You need to log in before you can comment on or make changes to this bug.