Closed Bug 393329 Opened 12 years ago Closed 12 years ago

numerous xslt leaks visiting http://www.metacafe.com

Categories

(Core :: XSLT, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: sayrer, Assigned: peterv)

References

()

Details

(Keywords: memory-leak)

Attachments

(3 files)

Attached file valgrind report
PCOM_MEM_LEAK_LOG shows many leaked nsTArray objects. valgrind shows more problem areas.

steps to reproduce:

open browser to http://www.metacafe.com
close browser
Assignee: nobody → peterv
Attached patch v1Splinter Review
Adds some virtual destructors where needed. Also adds logging code so a bunch of XSLT classes will show up in the leak logging.
Attachment #277932 - Flags: superreview?(jonas)
Attachment #277932 - Flags: review?(jonas)
Attached patch Cleanup v1Splinter Review
Since I looked at all destructors in content/xslt for attachment 277932 [details] [diff] [review] I also cleaned up a bunch of empty destructors and constructors. I've taken care not to delete virtual destructors in base classes.
Attachment #277933 - Flags: superreview?(jonas)
Attachment #277933 - Flags: review?(jonas)
Attachment #277932 - Flags: superreview?(jonas)
Attachment #277932 - Flags: superreview+
Attachment #277932 - Flags: review?(jonas)
Attachment #277932 - Flags: review+
Wondering whether a static analysis could find missing virtual dtors...

/be
Doesn't sound difficult. What exactly am I supposed to be looking for? 
Is the query classes with base classes which do not mark destructors as virtual?
Hmm.. i suspect we have plenty of classes that have non-virtual destructors and are subclassed, but still are ok since we always destroy the subclass. nsCOMPtr_base and nsTArray_base for example.

I guess what we're looking for is classes |Base| with non-virtual destructors, that are subclassed |Sub : public Base|, and where someone ever destroys |Base|.

Is that doable?
That could be hard. It all depends how you define "someone ever destroys |Base|"

it's easy to catch things like 
Base *foo = ..
delete foo;

It's not so easy to catch destruction of bases when they are within arrays or other indirect methods of destruction. That would require a sophisticated dataflow analysis.

Of course this could be simplified to require |Base| to have a virtual destructor if it has children and is ever used in a certain way... So as long as we define a comprehensive list of ways |Base| can't be used without a virt destructor things could be ok.

Another way would be to do some logging and figure out which destructors are being called without children being called first. I'm not sure if that's possible.
How do you mean "within arrays or other indirect methods of destruction"?
Attachment #277932 - Flags: approval1.9?
Jonas,
I mean it depends on how many ways you can call the destructor.
delete foo; <-- easy to detect
foo->Release(); <-- can pretend that it means the same as delete

However given
class Container {
nsCOMPtr<Base> foo;
}

followed by 
Container *b = ...
b->Release(); <-- causes Base's destructor to be called, but through a few levels of indirection.
Indirect ones are hard.

So I propose that we avoid complicated analyses and instead fix the problem at the type system level. Ie require base classes to have virtual destructors. Is there any downside to that?
Blocks: 393102
We don't want all baseclasses to have virtual dtors since for some cases that would be a significant increase in size if that is the only virtual function. Such as for nsCOMPtr_base or nsTArray_base which both would double in size if they have a vtable pointer.

for foo->Release, wouldn't you see in the implementation of Base::Release that it contains a |delete this| call where |this|. Same thing for the Container example since that is simply a convoluted way of calling Release.

nsAutoPtr<Base> might be worse though since there the templatized class calls delete.
(In reply to comment #9)
> for foo->Release, wouldn't you see in the implementation of Base::Release that
> it contains a |delete this| call where |this|. Same thing for the Container
> example since that is simply a convoluted way of calling Release.

That should say "where |this| has type Base*|.

> nsAutoPtr<Base> might be worse though since there the templatized class calls
> delete.

This code will contain |delete mRawPtr| where mRawPtr is of templetized type, so I'm not sure how doable that is :(
Attachment #277933 - Flags: superreview?(jonas)
Attachment #277933 - Flags: superreview+
Attachment #277933 - Flags: review?(jonas)
Attachment #277933 - Flags: review+
You can't "see" into methods unless you inline methods which wont scale. 

All easy analyses are just really simple pattern matching through code. So if I
look at a method and it's not immediately obvious from the code in the
method(and some basic class hierarchy knowledge) that something smells like
trouble, the analysis is probably not worth the hassle.

Templates are less trouble than they seem because elsa instantiates them for me
so from elsa's perspective they look the same as normal code.

Regarding vtable overhead. Could always restrict the analysis to functions that
already have a vtable of a certain size. If you think that's not worth it, then
there is not much I can do to help here.
What I mean is, wouldn't you detect the problem when analyzing the implementation of Base::Release? I'm not sure what your plan for finding

Base* base = ...
delete base;

was, but wouldn't that catch the delete inside Base::Release too?
To catch the virtual dtor thing, by the way, it helps to have all subclasses log ctor/dtor as well.  Then you'll get leak numbers as soon as you instantiate such a subclass...
Flags: blocking1.9?
Status: NEW → ASSIGNED
Flags: blocking1.9?
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Target Milestone: --- → mozilla1.9 M8
Version: unspecified → Trunk
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
No longer blocks: 393102
Duplicate of this bug: 393102
You need to log in before you can comment on or make changes to this bug.