1341961 - (xpcshell-leaks) Turn on leak checking in xpcshell tests

Status: NEW

(Testing :: XPCShell Harness, defect, P3)

Description

[(no longer active)]

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.

Comment 1

[(not currently active) Ted Mielczarek]

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.

Comment 2

[(no longer active)]

(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.)

Comment 3

[(no longer active)]

Ted, how can we make it so that this gets worked on and fixed?

Comment 4

[Marco Bonardo [:mak]]

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.

Comment 5

[(not currently active) Ted Mielczarek]

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

Comment 6

[(no longer active)]

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.  :/

Comment 7

[(not currently active) Ted Mielczarek]

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.

Comment 9

[Andrew McCreight [:mccr8] (PTO until 12-2)]

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.

Comment 10

[(not currently active) Ted Mielczarek]

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