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

Status: RESOLVED FIXED

(Core :: XSLT, defect, P1)

Description

[Robert Sayre]

Attachment: 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

Comment 1

[Peter Van der Beken [:peterv]]

Attachment: v1

Adds some virtual destructors where needed. Also adds logging code so a bunch of XSLT classes will show up in the leak logging.

Comment 2

[Peter Van der Beken [:peterv]]

Attachment: Cleanup v1

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.

Comment 3

[Brendan Eich [:brendan]]

Wondering whether a static analysis could find missing virtual dtors...

/be

Comment 4

[(dormant account)]

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?

Comment 5

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

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?

Comment 6

[(dormant account)]

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.

Comment 7

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

How do you mean "within arrays or other indirect methods of destruction"?

Comment 8

[(dormant account)]

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?

Comment 9

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

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.

Comment 10

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

(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 :(

Comment 11

[(dormant account)]

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.

Comment 12

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

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?

Comment 13

[Boris Zbarsky [:bzbarsky]]

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...

Comment 14

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

Checked in both patches.