Closed Bug 104042 Opened 23 years ago Closed 22 years ago

get some pampers, 'cause we leak

Categories

(Core :: XSLT, defect)

Product:
Core
Component:
XSLT
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
VERIFIED FIXED

People

(Reporter: axel, Assigned: keith)

Details

(Keywords: memory-leak)

Attachments

(5 obsolete files)

I'll add some leaks, and hopefully some patches as I go. Got the grand new workshop compiler, with a leak detection tool. ProcessorState::ImportFrame::~ImportFrame leaks templKeys ProcessorState::~ProcessorState doesn't clear() the (NamedMap*) variableSets.pop(); Element::setAttribute leaks new attributes, no idea if I plug that in nsdoom. There are more reports, but I can't trace those to source code right now. Axel
about the variable Bindings: We leak the globalVars from ::initialize, and we have to clear() the several NamedMaps that we use for localBindings or bindings in XSLTProcessor. Axel
Adding "mlk" keyword. Axel: Please include this keyword in any future bug reports on memory leaks.
Keywords: mlk
The localBindings should be ok since they have .setObjectDeletion(MB_TRUE), however we might very well leak the global variables, but i'm genually afraid of that code :-)
Just realized that we sometimes leak local variables. If a template is instantiated and you send a parameter "foo" to the template and the template doesn't contain a parameter "foo", but a variable "foo". Then the variable is leaked.
I think i found why we leak variable bindings. It turns out that my fix for bug 101946 wasn't entierly correct. What I did was to remove the bindings from the local variables if the variable was copied from the params map. However i just noticed that it's not the binding that is copied over from the params map, just the ExprResult contained in the binding. So what I do now is to remove the ExprResult contained in the local variable binding instead of the entire binding. I also added a check to make sure that the ExprResult is indeed the one copied from the params map. Again, this is patching existing logic, the RightThing fix is to rewrite the entire variables code, but that is left as an exercise to the reader ;-)
ok, this bug is two-fold. You're moving the delete stuff from remove to the destructor. And NamedMap::remove doesn't pay attention to the ownership. That's why we didn't crash and leaked instead. Cool. So now you're unsetting the value, and really delete the binding. Did I get that right? Would you know if we rely on ::remove in other places?
Comment on attachment 54059 [details] [diff] [review] fix templKeys leak r=me
Attachment #54059 - Flags: review+
Comment on attachment 54384 [details] [diff] [review] fix variable bindings leak r=me, assuming you think you did what I summarized above. Anyway, it works ;-)
Attachment #54384 - Flags: review+
Not sure that we're 100% on the same page, so just to clarify: The bug is two-fold: 1. Instead of removing the entire variable-binding from the variables-map i just remove the bound ExprResult, since that is all that is shared with the parameters-map 2. Before removing i check that it actually is the ExprResult shared with the parameters-map, since in some cases it's not. (Specifically if we ignored a parameter sent to the template, but created a variable with the same name)
Comment on attachment 54420 [details] [diff] [review] I introduced a leak in txNamespaceManager::Shutdown :-( r=sicking
Attachment #54420 - Flags: review+
The ::remove was never intended to do any deleting. It was done so that we would *not* delete the variable once the variable-map went away. The reason for this is that we didn't really own the variable, we had just "borrowed" it from the params-map. But it turns out that we just borrowed the ExprResult, not the entire binding, so that is what i remove now. We use NamedMap::remove to remove sourceDoc and the xslDoc out from the loadedDocuments map so that they won't get deleted along with the other documents. So we are relying elsewhere on ::remove not deleting.
Comment on attachment 54484 [details] [diff] [review] plug the leak of an inputstream in main, and the sub-exprResult in UnionExpr::evaluate (iterator cleanup, too) could you add a nullcheck of exprResult after expr->evaluate. with that r=sicking
Attachment #54484 - Flags: review+
r=peterv on templKeys (attachment 54059 [details] [diff] [review]), txNamespaceManager (attachment 54420 [details] [diff] [review]) and inputstream/UnionExpr with sicking's comment addressed (attachment 54484 [details] [diff] [review]).
Comment on attachment 54384 [details] [diff] [review] fix variable bindings leak >+ while((key = keyIter.next())) { Space after the while please. r=peterv.
Comment on attachment 54059 [details] [diff] [review] fix templKeys leak sr=jst
Attachment #54059 - Flags: superreview+
Comment on attachment 54384 [details] [diff] [review] fix variable bindings leak sr=jst
Attachment #54384 - Flags: superreview+
Comment on attachment 54420 [details] [diff] [review] I introduced a leak in txNamespaceManager::Shutdown :-( sr=jst
Attachment #54420 - Flags: superreview+
Comment on attachment 54484 [details] [diff] [review] plug the leak of an inputstream in main, and the sub-exprResult in UnionExpr::evaluate (iterator cleanup, too) sr=jst
Attachment #54484 - Flags: superreview+
Comment on attachment 54059 [details] [diff] [review] fix templKeys leak checked in
Attachment #54059 - Attachment is obsolete: true
Comment on attachment 54384 [details] [diff] [review] fix variable bindings leak checked in
Attachment #54384 - Attachment is obsolete: true
Comment on attachment 54420 [details] [diff] [review] I introduced a leak in txNamespaceManager::Shutdown :-( checked in
Attachment #54420 - Attachment is obsolete: true
Comment on attachment 54484 [details] [diff] [review] plug the leak of an inputstream in main, and the sub-exprResult in UnionExpr::evaluate (iterator cleanup, too) checked in
Attachment #54484 - Attachment is obsolete: true
how do we unleak result tree fragments? Currently, code like XSLTProcessor::processVariable creates a result tree fragment by creating a document fragment and adding that to a nodeset. That leaks the document fragment.
Comment on attachment 54420 [details] [diff] [review] I introduced a leak in txNamespaceManager::Shutdown :-( really in now
Comment on attachment 54850 [details] [diff] [review] others copied and pasted the leaks, I copy and paste the fix sr=jst
Attachment #54850 - Flags: superreview+
Comment on attachment 54850 [details] [diff] [review] others copied and pasted the leaks, I copy and paste the fix r=peterv
Attachment #54850 - Flags: review+
Comment on attachment 54850 [details] [diff] [review] others copied and pasted the leaks, I copy and paste the fix patch checked in
Attachment #54850 - Attachment is obsolete: true
do we still have any known leaks? or could/should this be closed? The only leak that I know of is that we leak RTFs in standalone, but fixing that requires implementing "real" RTFs which should IMHO be done in a separate bug (I was thinking of using bug 116755 for that)
we are faster these days. Lets close this and open bugs on specific issues when we find them
ok, the correct comment should be: We have plugged all currently known leaks. Lets close this and open bugs on specific issues when we find them
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
mass verifying
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: