PSA: Refcounted classes should have a non-public destructor & should be MOZ_FINAL where possible

[Daniel Holbert]

Hi dev-platform,

PSA: if you are adding a concrete class with AddRef/Release
implementations (e.g. via NS_INLINE_DECL_REFCOUNTING), please be aware
of the following best-practices:

 (a) Your class should have an explicitly-declared non-public
destructor. (should be 'private' or 'protected')

 (b) Your class should be labeled as MOZ_FINAL (or, see below).


WHY THIS IS A GOOD IDEA
===
We'd like to ensure that refcounted objects are *only* deleted via their
::Release() methods.  Otherwise, we're potentially susceptible to
double-free bugs.

We can go a long way towards enforcing this rule at compile-time by
giving these classes non-public destructors.  This prevents a whole
category of double-free bugs.

In particular: if your class has a public destructor (the default), then
it's easy for you or someone else to accidentally declare an instance on
the stack or as a member-variable in another class, like so:
MyClass foo;
This is *extremely* dangerous. If any code wraps 'foo' in a nsRefPtr
(say, if some function that we pass 'foo' or '&foo' into declares a
nsRefPtr to it for some reason), then we'll get a double-free. The
object will be freed when the nsRefPtr goes out of scope, and then again
when the MyClass instance goes out of scope. But if we give MyClass a
non-public destructor, then it'll make it a compile error (in most code)
to declare a MyClass instance on the stack or as a member-variable.  So
we'd catch this bug immediately, at compile-time.

So, that explains why a non-public destructor is a good idea. But why
MOZ_FINAL?  If your class isn't MOZ_FINAL, then that opens up another
route to trigger the same sort of bug -- someone can come along and add
a subclass, perhaps not realizing that they're subclassing a refcounted
class, and the subclass will (by default) have a public destructor,
which means then that anyone can declare
  MySubclass foo;
and run into the exact same problem with the subclass.  A MOZ_FINAL
annotation will prevent that by keeping people from naively adding
subclasses.

BUT WHAT IF I NEED SUBCLASSES
=
First, if your class is abstract, then it shouldn't have AddRef/Release
implementations to begin with.  Those belong on the concrete subclasses
-- not on your abstract base class.

But if your class is concrete and refcounted and needs to have
subclasses, then:
 - Your base class *and each of its subclasses* should have virtual,
protected destructors, to prevent the "MySubclass foo;" problem
mentioned above.
 - Your subclasses themselves should also probably be declared as
"MOZ_FINAL", to keep someone from naively adding another subclass
without recognizing the above.
 - Your subclasses should definitely *not* declare their own
AddRef/Release methods. (They should share the base class's methods &
refcount.)

For more information, see
https://bugzilla.mozilla.org/show_bug.cgi?id=984786 , where I've fixed
this sort of thing in a bunch of existing classes.  I definitely didn't
catch everything there, so please feel encouraged to continue this work
in other bugs. (And if you catch any cases that look like potential
double-frees, mark them as security-sensitive.)

Thanks!
~Daniel
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: Refcounted classes should have a non-public destructor & should be MOZ_FINAL where possible

[Benoit Jacob]

Awesome work!

By the way, I just figured a way that you could static_assert so that at
least on supporting C++11 compilers, we would automatically catch this.

The basic C++11 tool here is std::is_destructible from , but
it has a problem: it only returns false if the destructor is deleted, it
doesn't return false if the destructor is private. However, the example
below shows how we can still achieve what we want by using wrapping the
class that we are interested in as a member of a helper templated struct:



#include 
#include 

class ClassWithDeletedDtor {
  ~ClassWithDeletedDtor() = delete;
};

class ClassWithPrivateDtor {
  ~ClassWithPrivateDtor() {}
};

class ClassWithPublicDtor {
public:
  ~ClassWithPublicDtor() {}
};

template 
class IsDestructorPrivateOrDeletedHelper {
  T x;
};

template 
struct IsDestructorPrivateOrDeleted
{
  static const bool value =
!std::is_destructible>::value;
};

int main() {
#define PRINT(x) std::cerr << #x " = " << (x) << std::endl;

  PRINT(std::is_destructible::value);
  PRINT(std::is_destructible::value);
  PRINT(std::is_destructible::value);

  std::cerr << std::endl;

  PRINT(IsDestructorPrivateOrDeleted::value);
  PRINT(IsDestructorPrivateOrDeleted::value);
  PRINT(IsDestructorPrivateOrDeleted::value);
}


Output:


std::is_destructible::value = 0
std::is_destructible::value = 0
std::is_destructible::value = 1

IsDestructorPrivateOrDeleted::value = 1
IsDestructorPrivateOrDeleted::value = 1
IsDestructorPrivateOrDeleted::value = 0


If you also want to require classes to be final, C++11  also
has std::is_final for that.

Cheers,
Benoit


2014-05-28 16:24 GMT-04:00 Daniel Holbert :

> Hi dev-platform,
>
> PSA: if you are adding a concrete class with AddRef/Release
> implementations (e.g. via NS_INLINE_DECL_REFCOUNTING), please be aware
> of the following best-practices:
>
>  (a) Your class should have an explicitly-declared non-public
> destructor. (should be 'private' or 'protected')
>
>  (b) Your class should be labeled as MOZ_FINAL (or, see below).
>
>
> WHY THIS IS A GOOD IDEA
> ===
> We'd like to ensure that refcounted objects are *only* deleted via their
> ::Release() methods.  Otherwise, we're potentially susceptible to
> double-free bugs.
>
> We can go a long way towards enforcing this rule at compile-time by
> giving these classes non-public destructors.  This prevents a whole
> category of double-free bugs.
>
> In particular: if your class has a public destructor (the default), then
> it's easy for you or someone else to accidentally declare an instance on
> the stack or as a member-variable in another class, like so:
> MyClass foo;
> This is *extremely* dangerous. If any code wraps 'foo' in a nsRefPtr
> (say, if some function that we pass 'foo' or '&foo' into declares a
> nsRefPtr to it for some reason), then we'll get a double-free. The
> object will be freed when the nsRefPtr goes out of scope, and then again
> when the MyClass instance goes out of scope. But if we give MyClass a
> non-public destructor, then it'll make it a compile error (in most code)
> to declare a MyClass instance on the stack or as a member-variable.  So
> we'd catch this bug immediately, at compile-time.
>
> So, that explains why a non-public destructor is a good idea. But why
> MOZ_FINAL?  If your class isn't MOZ_FINAL, then that opens up another
> route to trigger the same sort of bug -- someone can come along and add
> a subclass, perhaps not realizing that they're subclassing a refcounted
> class, and the subclass will (by default) have a public destructor,
> which means then that anyone can declare
>   MySubclass foo;
> and run into the exact same problem with the subclass.  A MOZ_FINAL
> annotation will prevent that by keeping people from naively adding
> subclasses.
>
> BUT WHAT IF I NEED SUBCLASSES
> =
> First, if your class is abstract, then it shouldn't have AddRef/Release
> implementations to begin with.  Those belong on the concrete subclasses
> -- not on your abstract base class.
>
> But if your class is concrete and refcounted and needs to have
> subclasses, then:
>  - Your base class *and each of its subclasses* should have virtual,
> protected destructors, to prevent the "MySubclass foo;" problem
> mentioned above.
>  - Your subclasses themselves should also probably be declared as
> "MOZ_FINAL", to keep someone from naively adding another subclass
> without recognizing the above.
>  - Your subclasses should definitely *not* declare their own
> AddRef/Release methods. (They should share the base class's methods &
> refcount.)
>
> For more information, see
> https://bugzilla.mozilla.org/show_bug.cgi?id=984786 , where I've fixed
> this sort of thing in a bunch of existing classes.  I definitely didn't
> catch everything there, so please feel encouraged to continue this work
> in other bugs. (And if you catch any cases that look like potential
> double-frees, mark them as security-sensitive.)
>
> Thanks!
> ~Daniel
> _

Re: PSA: Refcounted classes should have a non-public destructor & should be MOZ_FINAL where possible

[Benoit Jacob]

Actually that test program contradicts what I said --- my
IsDestructorPrivateOrDeleted produces exactly the same result as
!is_destructible,  and is_destructible does return 0 for the class with
private destructor. So you could just use that!

Benoit


2014-05-28 16:51 GMT-04:00 Benoit Jacob :

> Awesome work!
>
> By the way, I just figured a way that you could static_assert so that at
> least on supporting C++11 compilers, we would automatically catch this.
>
> The basic C++11 tool here is std::is_destructible from , but
> it has a problem: it only returns false if the destructor is deleted, it
> doesn't return false if the destructor is private. However, the example
> below shows how we can still achieve what we want by using wrapping the
> class that we are interested in as a member of a helper templated struct:
>
>
>
> #include 
> #include 
>
> class ClassWithDeletedDtor {
>   ~ClassWithDeletedDtor() = delete;
> };
>
> class ClassWithPrivateDtor {
>   ~ClassWithPrivateDtor() {}
> };
>
> class ClassWithPublicDtor {
> public:
>   ~ClassWithPublicDtor() {}
> };
>
> template 
> class IsDestructorPrivateOrDeletedHelper {
>   T x;
> };
>
> template 
> struct IsDestructorPrivateOrDeleted
> {
>   static const bool value =
> !std::is_destructible>::value;
> };
>
> int main() {
> #define PRINT(x) std::cerr << #x " = " << (x) << std::endl;
>
>   PRINT(std::is_destructible::value);
>   PRINT(std::is_destructible::value);
>   PRINT(std::is_destructible::value);
>
>   std::cerr << std::endl;
>
>   PRINT(IsDestructorPrivateOrDeleted::value);
>   PRINT(IsDestructorPrivateOrDeleted::value);
>   PRINT(IsDestructorPrivateOrDeleted::value);
> }
>
>
> Output:
>
>
> std::is_destructible::value = 0
> std::is_destructible::value = 0
> std::is_destructible::value = 1
>
> IsDestructorPrivateOrDeleted::value = 1
> IsDestructorPrivateOrDeleted::value = 1
> IsDestructorPrivateOrDeleted::value = 0
>
>
> If you also want to require classes to be final, C++11  also
> has std::is_final for that.
>
> Cheers,
> Benoit
>
>
> 2014-05-28 16:24 GMT-04:00 Daniel Holbert :
>
> Hi dev-platform,
>>
>> PSA: if you are adding a concrete class with AddRef/Release
>> implementations (e.g. via NS_INLINE_DECL_REFCOUNTING), please be aware
>> of the following best-practices:
>>
>>  (a) Your class should have an explicitly-declared non-public
>> destructor. (should be 'private' or 'protected')
>>
>>  (b) Your class should be labeled as MOZ_FINAL (or, see below).
>>
>>
>> WHY THIS IS A GOOD IDEA
>> ===
>> We'd like to ensure that refcounted objects are *only* deleted via their
>> ::Release() methods.  Otherwise, we're potentially susceptible to
>> double-free bugs.
>>
>> We can go a long way towards enforcing this rule at compile-time by
>> giving these classes non-public destructors.  This prevents a whole
>> category of double-free bugs.
>>
>> In particular: if your class has a public destructor (the default), then
>> it's easy for you or someone else to accidentally declare an instance on
>> the stack or as a member-variable in another class, like so:
>> MyClass foo;
>> This is *extremely* dangerous. If any code wraps 'foo' in a nsRefPtr
>> (say, if some function that we pass 'foo' or '&foo' into declares a
>> nsRefPtr to it for some reason), then we'll get a double-free. The
>> object will be freed when the nsRefPtr goes out of scope, and then again
>> when the MyClass instance goes out of scope. But if we give MyClass a
>> non-public destructor, then it'll make it a compile error (in most code)
>> to declare a MyClass instance on the stack or as a member-variable.  So
>> we'd catch this bug immediately, at compile-time.
>>
>> So, that explains why a non-public destructor is a good idea. But why
>> MOZ_FINAL?  If your class isn't MOZ_FINAL, then that opens up another
>> route to trigger the same sort of bug -- someone can come along and add
>> a subclass, perhaps not realizing that they're subclassing a refcounted
>> class, and the subclass will (by default) have a public destructor,
>> which means then that anyone can declare
>>   MySubclass foo;
>> and run into the exact same problem with the subclass.  A MOZ_FINAL
>> annotation will prevent that by keeping people from naively adding
>> subclasses.
>>
>> BUT WHAT IF I NEED SUBCLASSES
>> =
>> First, if your class is abstract, then it shouldn't have AddRef/Release
>> implementations to begin with.  Those belong on the concrete subclasses
>> -- not on your abstract base class.
>>
>> But if your class is concrete and refcounted and needs to have
>> subclasses, then:
>>  - Your base class *and each of its subclasses* should have virtual,
>> protected destructors, to prevent the "MySubclass foo;" problem
>> mentioned above.
>>  - Your subclasses themselves should also probably be declared as
>> "MOZ_FINAL", to keep someone from naively adding another subclass
>> without recognizing the above.
>>  - Your subclasses should definitely *not* declare their 

Re: PSA: Refcounted classes should have a non-public destructor & should be MOZ_FINAL where possible

[Daniel Holbert]

Nice!  So it sounds like we could use std::is_destructible to harden our
refcounting-impl macros (like NS_INLINE_DECL_REFCOUNTING), by having
those macros include a static_assert which enforces that they're only
invoked by classes with private/protected destructors.

(I'll bet that this static_assert wouldn't transfer to subclasses - e.g.
if class Foo has NS_INLINE_DECL_REFCOUNTING and Foo has a subclass Bar,
I'll bet the static_assert would only check Foo, and not Bar.
Fortunately, it's rare to have concrete refcounted classes with
subclasses, so this shouldn't be a huge source of trouble.)

For now, our code isn't clean enough for this sort of static_assert to
be doable. :-/ And we have at least one instance of a refcounted class
that's semi-intentionally (albeit carefully) declared on the stack:
gfxContext, per https://bugzilla.mozilla.org/show_bug.cgi?id=742100

Still, the static_assert could be a good way of finding (in a local
build) all the existing refcounted classes that want a non-public
destructor, I suppose.

~Daniel

On 05/28/2014 01:51 PM, Benoit Jacob wrote:
> Awesome work!
> 
> By the way, I just figured a way that you could static_assert so that at
> least on supporting C++11 compilers, we would automatically catch this.
> 
> The basic C++11 tool here is std::is_destructible from ,
> but it has a problem: it only returns false if the destructor is
> deleted, it doesn't return false if the destructor is private. However,
> the example below shows how we can still achieve what we want by using
> wrapping the class that we are interested in as a member of a helper
> templated struct:
> 
> 
> 
> #include 
> #include 
> 
> class ClassWithDeletedDtor {
>   ~ClassWithDeletedDtor() = delete;
> };
> 
> class ClassWithPrivateDtor {
>   ~ClassWithPrivateDtor() {}
> };
> 
> class ClassWithPublicDtor {
> public:
>   ~ClassWithPublicDtor() {}
> };
> 
> template 
> class IsDestructorPrivateOrDeletedHelper {
>   T x;
> };
> 
> template 
> struct IsDestructorPrivateOrDeleted
> {
>   static const bool value =
> !std::is_destructible>::value;
> };
> 
> int main() {
> #define PRINT(x) std::cerr << #x " = " << (x) << std::endl;
> 
>   PRINT(std::is_destructible::value);
>   PRINT(std::is_destructible::value);
>   PRINT(std::is_destructible::value);
> 
>   std::cerr << std::endl;
> 
>   PRINT(IsDestructorPrivateOrDeleted::value);
>   PRINT(IsDestructorPrivateOrDeleted::value);
>   PRINT(IsDestructorPrivateOrDeleted::value);
> }
> 
> 
> Output:
> 
> 
> std::is_destructible::value = 0
> std::is_destructible::value = 0
> std::is_destructible::value = 1
> 
> IsDestructorPrivateOrDeleted::value = 1
> IsDestructorPrivateOrDeleted::value = 1
> IsDestructorPrivateOrDeleted::value = 0
> 
> 
> If you also want to require classes to be final, C++11 
> also has std::is_final for that.
> 
> Cheers,
> Benoit
> 
> 
> 2014-05-28 16:24 GMT-04:00 Daniel Holbert  >:
> 
> Hi dev-platform,
> 
> PSA: if you are adding a concrete class with AddRef/Release
> implementations (e.g. via NS_INLINE_DECL_REFCOUNTING), please be aware
> of the following best-practices:
> 
>  (a) Your class should have an explicitly-declared non-public
> destructor. (should be 'private' or 'protected')
> 
>  (b) Your class should be labeled as MOZ_FINAL (or, see below).
> 
> 
> WHY THIS IS A GOOD IDEA
> ===
> We'd like to ensure that refcounted objects are *only* deleted via their
> ::Release() methods.  Otherwise, we're potentially susceptible to
> double-free bugs.
> 
> We can go a long way towards enforcing this rule at compile-time by
> giving these classes non-public destructors.  This prevents a whole
> category of double-free bugs.
> 
> In particular: if your class has a public destructor (the default), then
> it's easy for you or someone else to accidentally declare an instance on
> the stack or as a member-variable in another class, like so:
> MyClass foo;
> This is *extremely* dangerous. If any code wraps 'foo' in a nsRefPtr
> (say, if some function that we pass 'foo' or '&foo' into declares a
> nsRefPtr to it for some reason), then we'll get a double-free. The
> object will be freed when the nsRefPtr goes out of scope, and then again
> when the MyClass instance goes out of scope. But if we give MyClass a
> non-public destructor, then it'll make it a compile error (in most code)
> to declare a MyClass instance on the stack or as a member-variable.  So
> we'd catch this bug immediately, at compile-time.
> 
> So, that explains why a non-public destructor is a good idea. But why
> MOZ_FINAL?  If your class isn't MOZ_FINAL, then that opens up another
> route to trigger the same sort of bug -- someone can come along and add
> a subclass, perhaps not realizing that they're subclassing a refcounted
> class, and the subclass will (by default) have a public destructor,
> which

Re: PSA: Refcounted classes should have a non-public destructor & should be MOZ_FINAL where possible

[Aryeh Gregor]

On Thu, May 29, 2014 at 12:27 AM, Daniel Holbert  wrote:
> For now, our code isn't clean enough for this sort of static_assert to
> be doable. :-/ And we have at least one instance of a refcounted class
> that's semi-intentionally (albeit carefully) declared on the stack:
> gfxContext, per https://bugzilla.mozilla.org/show_bug.cgi?id=742100
>
> Still, the static_assert could be a good way of finding (in a local
> build) all the existing refcounted classes that want a non-public
> destructor, I suppose.

You could also specifically list the exceptions in the static_assert
expression, so that it would catch any new additions unless they were
added to the list of exceptions.  (It would be good hygiene to also
static_assert that the exceptions *are* destructible, so that if
that's ever fixed they can be removed from the exception list.)
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: Refcounted classes should have a non-public destructor & should be MOZ_FINAL where possible

[Benoit Jacob]

Here's an update on this front.

In Bug 1027251  we
added a static_assert as discussed in this thread, which discovered all
remaining instances, and we fixed the easy ones, which were the majority.

The harder ones have been temporarily whitelisted. See
HasDangerousPublicDestructor.

There are 11 such classes. Follow-up bugs have been filed for each of them,
blocking the tracking Bug 1028132
.

Help is very welcome to fix these 11 classes! I won't have more time to
work on this for now.

The trickiest one is probably going to be mozilla::ipc::SharedMemory, which
is refcounted but to which IPDL-generated code takes nsAutoPtr's... so if
you have data that you care about, don't put it in a SharedMemory, for
now... the bug for this one is Bug 1028148
.

This is only about nsISupportsImpl.h refcounting. We considered doing the
same for MFBT RefCounted (Bug 1028122
) but we can't,
because as C++ base classes have no access to protected derived class
members, RefCounted inherently forces making destructors public (unless we
befriend everywhere), which is also the reason why we had concluded earlier
that RefCounted is a bad idea.

We haven't started checking final-ness yet. It's an open question AFAIK how
we would enforce that, as there are legitimate and widespread uses for
non-final refcounting. We would probably have to offer separate _NONFINAL
refcounting macros, or something like that.

Thanks,
Benoit




2014-05-28 16:24 GMT-04:00 Daniel Holbert :

> Hi dev-platform,
>
> PSA: if you are adding a concrete class with AddRef/Release
> implementations (e.g. via NS_INLINE_DECL_REFCOUNTING), please be aware
> of the following best-practices:
>
>  (a) Your class should have an explicitly-declared non-public
> destructor. (should be 'private' or 'protected')
>
>  (b) Your class should be labeled as MOZ_FINAL (or, see below).
>
>
> WHY THIS IS A GOOD IDEA
> ===
> We'd like to ensure that refcounted objects are *only* deleted via their
> ::Release() methods.  Otherwise, we're potentially susceptible to
> double-free bugs.
>
> We can go a long way towards enforcing this rule at compile-time by
> giving these classes non-public destructors.  This prevents a whole
> category of double-free bugs.
>
> In particular: if your class has a public destructor (the default), then
> it's easy for you or someone else to accidentally declare an instance on
> the stack or as a member-variable in another class, like so:
> MyClass foo;
> This is *extremely* dangerous. If any code wraps 'foo' in a nsRefPtr
> (say, if some function that we pass 'foo' or '&foo' into declares a
> nsRefPtr to it for some reason), then we'll get a double-free. The
> object will be freed when the nsRefPtr goes out of scope, and then again
> when the MyClass instance goes out of scope. But if we give MyClass a
> non-public destructor, then it'll make it a compile error (in most code)
> to declare a MyClass instance on the stack or as a member-variable.  So
> we'd catch this bug immediately, at compile-time.
>
> So, that explains why a non-public destructor is a good idea. But why
> MOZ_FINAL?  If your class isn't MOZ_FINAL, then that opens up another
> route to trigger the same sort of bug -- someone can come along and add
> a subclass, perhaps not realizing that they're subclassing a refcounted
> class, and the subclass will (by default) have a public destructor,
> which means then that anyone can declare
>   MySubclass foo;
> and run into the exact same problem with the subclass.  A MOZ_FINAL
> annotation will prevent that by keeping people from naively adding
> subclasses.
>
> BUT WHAT IF I NEED SUBCLASSES
> =
> First, if your class is abstract, then it shouldn't have AddRef/Release
> implementations to begin with.  Those belong on the concrete subclasses
> -- not on your abstract base class.
>
> But if your class is concrete and refcounted and needs to have
> subclasses, then:
>  - Your base class *and each of its subclasses* should have virtual,
> protected destructors, to prevent the "MySubclass foo;" problem
> mentioned above.
>  - Your subclasses themselves should also probably be declared as
> "MOZ_FINAL", to keep someone from naively adding another subclass
> without recognizing the above.
>  - Your subclasses should definitely *not* declare their own
> AddRef/Release methods. (They should share the base class's methods &
> refcount.)
>
> For more information, see
> https://bugzilla.mozilla.org/show_bug.cgi?id=984786 , where I've fixed
> this sort of thing in a bunch of existing classes.  I definitely didn't
> catch everything there, so please feel encouraged to continue this work
> in other bugs. (And if you catch any cases that look like potential
> double-frees, mark them as security-

Re: PSA: Refcounted classes should have a non-public destructor & should be MOZ_FINAL where possible

[Neil]

Daniel Holbert wrote:


(a) Your class should have an explicitly-declared non-public destructor. 
(should be 'private' or 'protected')
 

Except for refcounted base classes (which as you note need to have a 
protected virtual destructor), is there a correct style as to whether 
the destructor should be private or protected and virtual or nonvirtual?



First, if your class is abstract, then it shouldn't have AddRef/Release 
implementations to begin with.  Those belong on the concrete subclasses -- not 
on your abstract base class.

What's correct code for abstract class Foo (implementing interfaces 
IFoo1 and IFoo2) with derived classes Bar and Baz (implementing 
interfaces IBar1 and IBar2 or IBaz1 and IBaz2 respectively)?


--
Warning: May contain traces of nuts.
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: Refcounted classes should have a non-public destructor & should be MOZ_FINAL where possible

[Daniel Holbert]

On 07/10/2014 02:48 AM, Neil wrote:
> Except for refcounted base classes (which as you note need to have a
> protected virtual destructor), is there a correct style as to whether
> the destructor should be private or protected and virtual or nonvirtual?

IMO, the destructor should be nonvirtual, since we're only expecting to
be destroyed via the concrete class's ::Release() method.  So we
shouldn't need a virtual destructor.  (I'm not sure how best to *ensure*
that we're never destroyed via "delete pointerOfSuperclassType"
though... We could give all of the superclasses protected destructors,
which would help, but not be 100% foolproof.)

As for protected vs. private --  in the concrete class, I don't think it
particularly matters, since the keywords have the same effect, as long
as the class is MOZ_FINAL.

IIRC, when I was actively working on this cleanup, if we already had an
already-existing "protected" section like below...

class Foo MOZ_FINAL : public Bar {
 public:
   [...]
 protected:
   // overrides of protected methods from superclass
   // (in section labeled as "protected" for consistency w/ superclass)
   [...]
};

...then I just used the existing "protected" area, for conciseness,
rather than adding a new "private" area.  But if there wasn't any
consistency-with-my-superclass argument to be made for using
"protected", then I'd just use private, to avoid the misleading
implication that we have subclasses which can see *our* protected stuff.

>> First, if your class is abstract, then it shouldn't have
>> AddRef/Release implementations to begin with.  Those belong on the
>> concrete subclasses -- not on your abstract base class.
>>
> What's correct code for abstract class Foo (implementing interfaces
> IFoo1 and IFoo2) with derived classes Bar and Baz (implementing
> interfaces IBar1 and IBar2 or IBaz1 and IBaz2 respectively)?

Shouldn't the refcounting still be on the concrete classes? Or does that
not work for some reason?  Based on your question, I'm guessing it
doesn't. Anyway, I'm not sure what's supposed to happen there. :)

~Daniel
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: Refcounted classes should have a non-public destructor & should be MOZ_FINAL where possible

[Benjamin Smedberg]

On 7/10/2014 10:46 AM, Daniel Holbert wrote:



First, if your class is abstract, then it shouldn't have
AddRef/Release implementations to begin with.  Those belong on the
concrete subclasses -- not on your abstract base class.


What's correct code for abstract class Foo (implementing interfaces
IFoo1 and IFoo2) with derived classes Bar and Baz (implementing
interfaces IBar1 and IBar2 or IBaz1 and IBaz2 respectively)?

Shouldn't the refcounting still be on the concrete classes?

Why?

This happens for example with nsRunnable: nsRunnable does the 
refcounting, and all the derivations of nsRunnable only have to worry 
about overriding Run because there's a virtual destructor.


--BDS

___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform

Re: PSA: Refcounted classes should have a non-public destructor & should be MOZ_FINAL where possible

[Daniel Holbert]

On 07/10/2014 08:03 AM, Benjamin Smedberg wrote:
> On 7/10/2014 10:46 AM, Daniel Holbert wrote:
>> Shouldn't the refcounting still be on the concrete classes?
> Why?
> 
> This happens for example with nsRunnable: nsRunnable does the
> refcounting, and all the derivations of nsRunnable only have to worry
> about overriding Run because there's a virtual destructor.

Oh, good point -- the refcounting can go on an abstract base class, if
that base class has a virtual destructor.

So, I retract my "if your class is abstract, then it shouldn't have
AddRef/Release implementations to begin with" statement from the initial
email on this thread.  (I think the rest of my "BUT WHAT IF I NEED
SUBCLASSES" section from that message still applies, but just with the
abstract/concrete distinction removed.)

~Daniel
___
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform