72179 - Remove memory leaks in various source files

Status: VERIFIED FIXED

(Core :: XSLT, defect, P3)

Description

[Walter Schmid]

See attachment for our modifications - it probably makes sense to integrate 
them into the original code.

Comment 1

[Walter Schmid]

Attachment: Proposed fixes for various memory leaks

Comment 2

[Axel Hecht]

we need patches, not essays.
If you delete objects, please tell why that pointer owns the object. I don't
see that at least in the first part.

Feel free to tell us what you do and why, put give a seperate patch, at least
I need this to quickly understand what you're doing.
Unified diffs against the tip are preffered, cvs -z3 diff -u

Axel

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

These are some good comments anyways and points out fixes that we defenetly 
need (though far from all). However at first glance I notice a couple of 
problems:

* NodeSets never own thier nodes, they are always owned by the documents
* NamedMaps needs to check if it owns it's items before deleteing

Comment 4

[Keith Visco]

RelationalExpr and PredicateList need the fixes. I am against the changes 
to NodeSet, and yes NamedMap needs to know if it has control over deletion 
before deleting any object pointers.

Comment 5

[Walter Schmid]

The motivation for our memory investigations was to make the standalone XSLT 
transformiix source run as a continously available service component to our ORB.
And as we don't use any of your utilities in our environment (neither CVS, nor 
any others), I had to choose the "essays"-way to provide my suggestions.
I'm sorry about that!

NodeSet.cpp and NodeSet.h have been modified since the last download mid of 
February, so these modifications are not actual any more. We will reinvestigate 
that. The flag setting in XSLTProcessor.cpp is directly dependant of whether 
the modifications in the NodeSet files are going to be implemented or not.
In StringFunctionCall.cpp the "ListIterator* iter" in CASE:TRANSLATE may not be 
free'ed in some situations. Our modification makes it behave like all the other 
CASE's of that SWITCH-statement.
I can't comment on NamedMap.cpp as you all know the code much better than I do.

Comment 6

[Axel Hecht]

sorry for my harsh comment.
that was overdone.
A few things: working in mozilla's vicinity makes installing at least a cvs
client a good investment. Only put those things as attachement to bugzilla,
that make sense as separate file. Like diffs, as those are used as input to 
patch. If you just describe the steps and places to change code, just comment
on the bug. No problem if that is lengthy. Instead of quoting alot of code,
give line numbers.

Axel

Comment 7

[Håkan Waara]

If Keith Visco isn't considering to fix this in Mozilla, then I suggest someone
reassigns this bug.

Comment 8

[Keith Visco]

Hakan,

Comments like yours are unappreciated, at least by me, and only cause spam.

First of all, I think you need to re-read my response. I agreed with some of the 
proposed changes, and disagreed with others. 

Secondly, if the developers of a module don't agree, collectively, with a 
suggested patch or feature request, well, then you are out of luck, so it 
doesn't matter who you assign the bug to.

We try very hard to handle all bug/feature requests. Sometimes a request might 
not be appropriate, for example, modifying NodeSet to handle deletion doesn't 
make sense for us, as the Document Node owns all of it's child nodes and handles 
their deletion respectively. If we feel a suggestion is not appropriate it will 
not be implemented/fixed.

So I hope you understand the contributors of this module, which includes myself, 
sometimes have to make decisions that you might not like. In which case I 
apologize in advance for having to make such decisions.

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: Proposed patches as a diff

Comment 10

[Peter Van der Beken [:peterv]]

We all agree :-).
Walter: thanks for your contribution. Essays do give us more work, but we can 
handle them too. As you already changed the source, making a diff would be 
pretty easy and give us something we can check-in immediately. Anyway, I've 
implemented the changes, except for the NodeSet stuff. Walter, where you seeing 
real leaks of the nodes in that NodeSet, or was this found through code-
inspection?
Hwaara: please inform yourself before making (useless) comments, I highly agree 
with Keith here. Three people commented on the bug, two of them are regular 
contributors to Transformiix and the third (Keith) is the module owner for 
Transformiix. We all want these changes to go in, except those that are wrong. 
Do you disagree with that?

Comment 11

[Peter Van der Beken [:peterv]]

Turning some knobs, and snatching this away from Keith.

Comment 12

[Walter Schmid]

We were using Rational's tool "Purify", which, on return from a call to the 
XSLT component searches the entire address space looking for allocated memory 
to which there are no pointers . Based on its output we 've made our 
modifications to the Mozilla source files. But as TransforMiix is going to be 
improved continously, our modifications to the Nodeset files, which originally 
were based on M16/M18, are probably outdated. We will repeat our tests with the 
next official release from Mozilla. Thanks anyway for discussing and 
integrating the suggestions !

Comment 13

[Walter Schmid]

We fully agree - our modifications in Nodeset.cpp/h make no sense any more. Tks.

Comment 14

[Peter Van der Beken [:peterv]]

Fix checked in.

Comment 15

[Axel Hecht]

get peterv off my back, being a verification bitch