16762 - delete <interface pointer>

Status: RESOLVED DUPLICATE of bug 1181412

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Because nsISupports and other interfaces do not have virtual destructors, using
operator delete on an interface pointer can cause resource leaks.

Typical pattern:

nsISupports *inst = new Foo;

:

delete inst;

LXR shows many instances of these if you seach for 'delete inst'. You can catch
more by adding

protected:
  ~nsISupports() {}

to nsISupports interface. Best way might be to add private operator delete to
nsISupports, but because people have introduced their own operator deletes this
might be tricky. See

http://www.develop.com/dbox/cxx/InterfacePtr.htm
http://www.develop.com/dbox/cxx/SmartPtr.htm

for more details.

This can be fixed by

  a) Not using nsISupports above, i.e. it becomes

nsFoo *inst = new Foo;

:

delete inst;

    which is safe.

  b) Avoid delete, use AddRef and Release instead.

nsISupports *inst = new Foo;

:

NS_ADDREF(inst);

nsresult rv = inst->QueryInterafce(...);

NS_RELEASE(inst);

Comment 1

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I have fixed most of these for Win. I am creating patch, but the network seems
to be really slow...

Comment 2

[Suresh Duddi (gone)]

Thanks heikki.

Comment 3

[Suresh Duddi (gone)]

If you send me a patch, I will fix it sooner.

Comment 4

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Fixed most of these on Windows NT

Comment 5

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I posted some explanation of the patch in n.p.m.patch.

Comment 6

[Heikki Toivonen (remove -bugzilla when emailing directly)]

By the way, this AddRef/Release trick can be achieved with nsCOMPtr as well:

nsCOMPtr<nsISupports> inst;

inst = new Foo; // AddRef called

inst->QueryInterface(...)

} // Release called

Comment 7

[Suresh Duddi (gone)]

Right. shaver posted that too.

COMPtr, if it can be used is the best implementation. but since most are doing
something like new MyObject() using a comptr is like casting this right. I guess
that is ok since this is the only time we can really cast at all. We know the
object and who its base classes are.

Comment 8

[Suresh Duddi (gone)]

I am compiling with your patch on unix. There seems to be unix specific files
that I need to fix. That is causing a lot of good cleanup of factories to
happen.

After that I need to get this compiling on the mac too. Scott can you help me
out with this.

Comment 9

[Scott Collins]

One thing I'm concerned about, though, is that the code pattern in the
bug is not type-correct.  If the |nsISupports| that a |Foo| would
return is not the same one that C++ implicitly casts to, then
|nsCOMPtr| asserts after the assignment.  It is easier to use the most
derived interface, to get the right thing, e.g.,

    // given
  class nsFooImpl : public nsIFoo, public nsIBar, ... { ... }


    // use...
  {
    nsCOMPtr<nsIFoo> foo = new nsFooImpl;
      // there's only one |nsIFoo| in a |nsFooImpl|, so no ambiguity
      //  and prefer construction to construction+assignment

    nsresult rv = foo->QueryInterface(...);
  }

Comment 10

[Scott Collins]

OK, unfortunately, the trick of making a potected destructor only makes a warning
on the Mac, it doesn't stop the build.  Same for a protected operator delete.  A
private destructor or operator delete make it impossible to build at all, because
it denies even legal usage.  I'll have to search through the warnings with a
script.  This is a compiler bug that needs to be reported to Metrowerks.

Comment 11

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I realized nsCOMPtr probably should not be used in situations like this. Take a
closer look at what happens on line:

inst = new Foo; // AddRef called

1) new object is created
2) QueryInterface is called

Now you see the problem: if QI fails, we leak.

Comment 12

[Scott Collins]

|QueryInterface()| is not called implicitly by |nsCOMPtr|.  In your example, |
QueryInterface| is not called at all.  To make |QueryInterface| be called, you
must explicitly ask for it, e.g.,

  inst = do_QueryInterface( new Foo );

which we all knew not to ask for.  That's one of the reasons in my example I
asked for a thing which I knew C++ could cast to _without_ needing a QI to get it
right.  It's OK not to use |nsCOMPtr| here if you don't want to, but I wanted to
clear up the notion that it is automatically calling |QueryInterface|.  It is
not.

Comment 13

[Scott Collins]

I have checked in the Mac specific fixes.  The tree is clean, for the moment,
with respect to this bug, for as well as we can detect it.  We would probably
find more if we instrumented XPIDL to (for this purpose only) add empty protected
destructors to _every_ interface class.  I'll try that and see what I get.

Comment 14

[Heikki Toivonen (remove -bugzilla when emailing directly)]

scc, could you please check again that:

inst = new nsFoo;

does not call QI, because when I tested it it DID call QI...

Comment 15

[Suresh Duddi (gone)]

Plus I am going to mark this one closed. Once in a while we can go do this
exercise. Plus people copy code. If we eliminate all code that they copy from,
we just eliminated the possibility of this creeping in too.

So let us not spend more time on this.

Thanks scc and heikki. A lot more people will benefit from your QI being called
if that was in a newsgroup.

Comment 16

[Heikki Toivonen (remove -bugzilla when emailing directly)]

scc replied in private email so I am just going to put this for all to see:

The QI I saw happens only in DEBUG builds and the assignment is still safe. So
you can use the nsCOMPtr way to fix these bugs.

Comment 17

[Heikki Toivonen (remove -bugzilla when emailing directly)]

I suggest we move the milestone of this bug forward, and should actually keep it 
open as some other reminder bugs. After all, there is nothing preventing anyone 
from making more of these mistakes.

scc, did you instrument xpidl to catch these? If we introduced some debug flag 
to XPIDL and set up a separate tinderbox machine building with this flag we woul 
d be much better off. 

Still that would not solve the whole problem, because not all interfaces have 
been specified in XPIDL. I guess some special tool to add the protected 
destructor to all classes specified in files in dist/include and a new build 
(can you do this without overwriting the include dir?) would be needed to catch 
all cases.

I am trying to find time to take a second round of this to see if we have 
introduced any new instances of this bug.

Comment 18

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Reopened bug and cleared milestone.

I hacked the xpidl compiler to emit protected destructor and tried to compile. 
As you can guess, it failed. I will create a patch for Win NT.

Comment 19

[Suresh Duddi (gone)]

Scc is a better owner for this bug.

Comment 20

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Fix for Windows, includes xpidl hack

Comment 21

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Attachment: Perl utility to add protected dtor to C++ headers

Comment 22

[Scott Collins]

Don't know if this is possible, but this is another good thing to look for with 
the [XP]COM lint script (tell me what the real name is going to be).

Comment 23

[Scott Collins]

mass re-assigning to my new bugzilla account

Comment 24

[Dawn Endico]

dp is no longer @netscape.com. changing qa contact to default for this product

Comment 25

[Thomas]

Heikki, I saw you created 2 extra fixes, but I noticed action on this bug
stopped pretty soon afterwards. Do you know if your patches were applied into
the code?
Is this problem still available?

Comment 26

[Benjamin Smedberg]

I think we could do this with static analysis as a destructor which may only be called from a subclass destructor.

Comment 27

[Tom Ritter [:tjr] (OOTO until 4/30 at least)]

I built with a protected: ~nsISupports() {} successfully. Does that imply this is fixed?

Comment 28

[Nika Layzell [:nika] (ni? for response)]

This is bug 1181412 I think - closing as dup.