1279609 - nsRunnableMethodImpl has a public destructor

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Andrew McCreight [:mccr8]]

Having a public destructor for a refcounted class lets you call delete on it directly, which could lead to bad things. We have a static_assert to check this, but it only works on classes that actually implement refcounting (bug 1181412 is about adding a static analysis). This probably is not causing bug 1271182, but who knows. I'll see if anything turns up when I make this dtor protected.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: nsRunnableMethodImpl should use a protected destructor.

Fortunately it looks like nobody was calling it.

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8762164 [details] [diff] [review]
nsRunnableMethodImpl should use a protected destructor.

Review of attachment 8762164 [details] [diff] [review]:
-----------------------------------------------------------------

Do all the subclasses have appropriate destructors, too?  I assume that's why you made it protected, rather than private.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbc1a4dabe05
nsRunnableMethodImpl should use a non-public destructor. r=froydnj

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Nathan Froyd [:froydnj] from comment #2)
> Do all the subclasses have appropriate destructors, too?  I assume that's
> why you made it protected, rather than private.

I made it private, though it turns out that at least as of now the class is final so it shouldn't matter. I was just using protected because that's what we use everywhere for refcounted dtors.

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dbc1a4dabe05