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
