Closed Bug 507285 Opened 10 years ago Closed 8 years ago

Filter code leaks with copy then move

Categories

(MailNews Core :: Filters, defect, minor)

x86
Windows XP
defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 3.0b4

People

(Reporter: rkent, Assigned: Bienvenu)

References

Details

(Keywords: memory-leak)

Attachments

(2 files)

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.
Attachment #391480 - Attachment mime type: text/x-c → text/plain
Attached patch proposed fixSplinter Review
yes, this is a real leak - this fixes it. I was a little worried about cycles but it seems to be OK.
Assignee: nobody → bienvenu
Attachment #392727 - Flags: superreview?(bugzilla)
Attachment #392727 - Flags: review?(bugzilla)
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Keywords: mlk
Attachment #392727 - Flags: superreview?(bugzilla)
Attachment #392727 - Flags: superreview+
Attachment #392727 - Flags: review?(bugzilla)
Attachment #392727 - Flags: review+
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).
I think landing the test wouldn't hurt...but I'll land the fix in the mean time, after removing the assignment, if appropriate.
fix checked in.
Target Milestone: --- → Thunderbird 3.0b4
Comment on attachment 391480 [details]
This unit test leaks (minimized functionality to demo leak)

I can review this since I ran it.
Attachment #391480 - Flags: review?(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...
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.
Blocks: 387361
Severity: normal → minor
Kent/David: do we still want to land this test somewhere, or do we just leave it?
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 on attachment 391480 [details]
This unit test leaks (minimized functionality to demo leak)

clearing review request per rkent's comment.
Attachment #391480 - Flags: review?(bienvenu)
So this seems to be fixed? The leak patch is merged, the test patch is decided as not needed. Anything still to do?
yeah, thx, marking fixed.
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.