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