507285 - Filter code leaks with copy then move

Status: RESOLVED FIXED

(MailNews Core :: Filters, defect, minor)

Description

[Kent James (:rkent)]

Attachment: This unit test leaks (minimized functionality to demo leak)

While doing a unit test for bug 448337, I tried to add an extra test that tested a local copy-then-move incoming filter (which is not part of that bug). Although the test works, it leaks. I minimized the functionality to just demonstrate the leak in the attached test here. I will disable the leaking portions of the test in what I submit for bug 448337.

I suspect this is a real leak and not just a testing artifact, but I am not sure.

Comment 1

[David :Bienvenu]

Attachment: proposed fix

yes, this is a real leak - this fixes it. I was a little worried about cycles but it seems to be OK.

Comment 2

[Mark Banner (:standard8)]

Comment on attachment 392727 [details] [diff] [review]
proposed fix

You could potentially remove the assignment to nsnull in the constructor as well.

r/sr=Standard8. Do we want to land the test as well? I haven't reviewed that (yet).

Comment 3

[David :Bienvenu]

I think landing the test wouldn't hurt...but I'll land the fix in the mean time, after removing the assignment, if appropriate.

Comment 4

[David :Bienvenu]

fix checked in.

Comment 5

[David :Bienvenu]

Comment on attachment 391480 [details]
This unit test leaks (minimized functionality to demo leak)

I can review this since I ran it.

Comment 6

[David :Bienvenu]

Comment on attachment 391480 [details]
This unit test leaks (minimized functionality to demo leak)

actually, Kent, does it make sense to try to land this as a separate test, or to put it in the original bug's test case? You have a comment saying you would remove the part that leaked from that patch...

Comment 7

[Kent James (:rkent)]

I did remove the part that leaked from the test case for bug 448337, and that test has already landed. I don't think it makes sense to rework it, as the removed part of the test was not needed to demonstrate the feature that was addressed in that bug.

But what might make sense is test_localToImapFilter.js, which if you recall also leaked, but also had intermittent failures and so is now disabled. It would be worth testing if the fix for this bug also fixes that leak. If so, time would be better spent getting that test to work again, rather than expanding an already working patch.

I never did my prereview check of the unit test I gave here, it was meant as a demonstration rather than a permanent test. But I don't have strong feelings about any of that plans.

Comment 8

[Mark Banner (:standard8)]

Kent/David: do we still want to land this test somewhere, or do we just leave it?

Comment 9

[Kent James (:rkent)]

I don't think we should try to land this. As I said in comment 7, it would be better to spend any effort on test_localToImapFilter.js which also showed the leak. The test was only meant as a demo of a leak.

Comment 10

[David :Bienvenu]

Comment on attachment 391480 [details]
This unit test leaks (minimized functionality to demo leak)

clearing review request per rkent's comment.

Comment 11

[:aceman]

So this seems to be fixed? The leak patch is merged, the test patch is decided as not needed. Anything still to do?

Comment 12

[David :Bienvenu]

yeah, thx, marking fixed.