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: