get some pampers, 'cause we leak

VERIFIED FIXED

Status

()

VERIFIED FIXED
17 years ago
16 years ago

People

(Reporter: axel, Assigned: keith)

Tracking

({memory-leak})

Trunk
memory-leak
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 obsolete attachments)

(Reporter)

Description

17 years ago
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
(Reporter)

Comment 1

17 years ago
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 ;-)
(Reporter)

Comment 8

17 years ago
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?
(Reporter)

Comment 9

17 years ago
Comment on attachment 54059 [details] [diff] [review]
fix templKeys leak

r=me
Attachment #54059 - Flags: review+
(Reporter)

Comment 10

17 years ago
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+
(Reporter)

Comment 11

17 years ago
Created attachment 54420 [details] [diff] [review]
I introduced a leak in txNamespaceManager::Shutdown :-(
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.
(Reporter)

Comment 15

17 years ago
Created attachment 54484 [details] [diff] [review]
plug the leak of an inputstream in main, and the sub-exprResult in UnionExpr::evaluate (iterator cleanup, too)
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+
(Reporter)

Comment 23

17 years ago
Comment on attachment 54059 [details] [diff] [review]
fix templKeys leak

checked in
Attachment #54059 - Attachment is obsolete: true
(Reporter)

Comment 24

17 years ago
Comment on attachment 54384 [details] [diff] [review]
fix variable bindings leak

checked in
Attachment #54384 - Attachment is obsolete: true
(Reporter)

Comment 25

17 years ago
Comment on attachment 54420 [details] [diff] [review]
I introduced a leak in txNamespaceManager::Shutdown :-(

checked in
Attachment #54420 - Attachment is obsolete: true
(Reporter)

Comment 26

17 years ago
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
(Reporter)

Comment 27

17 years ago
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.
(Reporter)

Comment 28

17 years ago
Comment on attachment 54420 [details] [diff] [review]
I introduced a leak in txNamespaceManager::Shutdown :-(

really in now
(Reporter)

Comment 29

17 years ago
Created attachment 54850 [details] [diff] [review]
others copied and pasted the leaks, I copy and paste the fix
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+
(Reporter)

Comment 32

17 years ago
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
Last Resolved: 16 years ago
Resolution: --- → FIXED
(Reporter)

Comment 36

16 years ago
mass verifying
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.