Closed
Bug 507285
Opened 12 years ago
Closed 10 years ago
Filter code leaks with copy then move
Categories
(MailNews Core :: Filters, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 3.0b4
People
(Reporter: rkent, Assigned: Bienvenu)
References
Details
(Keywords: memory-leak)
Attachments
(2 files)
5.17 KB,
text/plain
|
Details | |
1.33 KB,
patch
|
standard8
:
review+
standard8
:
superreview+
|
Details | Diff | Splinter Review |
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.
Updated•12 years ago
|
Attachment #391480 -
Attachment mime type: text/x-c → text/plain
Assignee | ||
Comment 1•12 years ago
|
||
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)
Assignee | ||
Updated•12 years ago
|
Updated•12 years ago
|
Attachment #392727 -
Flags: superreview?(bugzilla)
Attachment #392727 -
Flags: superreview+
Attachment #392727 -
Flags: review?(bugzilla)
Attachment #392727 -
Flags: review+
Comment 2•12 years ago
|
||
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).
Assignee | ||
Comment 3•12 years ago
|
||
I think landing the test wouldn't hurt...but I'll land the fix in the mean time, after removing the assignment, if appropriate.
Assignee | ||
Comment 5•12 years ago
|
||
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)
Assignee | ||
Comment 6•12 years ago
|
||
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...
Reporter | ||
Comment 7•12 years ago
|
||
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.
Updated•11 years ago
|
Severity: normal → minor
Comment 8•11 years ago
|
||
Kent/David: do we still want to land this test somewhere, or do we just leave it?
Reporter | ||
Comment 9•11 years ago
|
||
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.
Assignee | ||
Comment 10•11 years ago
|
||
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)
![]() |
||
Comment 11•10 years ago
|
||
So this seems to be fixed? The leak patch is merged, the test patch is decided as not needed. Anything still to do?
Assignee | ||
Comment 12•10 years ago
|
||
yeah, thx, marking fixed.
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•