Here's an update on this front. In Bug 1027251 <https://bugzilla.mozilla.org/show_bug.cgi?id=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<T>. There are 11 such classes. Follow-up bugs have been filed for each of them, blocking the tracking Bug 1028132 <https://bugzilla.mozilla.org/show_bug.cgi?id=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 <https://bugzilla.mozilla.org/show_bug.cgi?id=1028148>. This is only about nsISupportsImpl.h refcounting. We considered doing the same for MFBT RefCounted (Bug 1028122 <https://bugzilla.mozilla.org/show_bug.cgi?id=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 <dholb...@mozilla.com>: > 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 > _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
