This adds a static analysis pass which will detect refcounted classes which have public destructors. This has already been attempted using static_assert in bug 1027251, however, that patch does not detect subclasses with public destructors of classes which declare support for refcounting and lack public destructors.

I ran the analysis as a warning, and performed some very sloppy regular expression pruning of the output. The analysis so far has found 2345 reference counted records which have public destructors. I think it would be nice if we could use a rewriter to help get the bulk of these changed to have protected destructors. Namely, the rewriter would either: * If there was an explicit destructor, add `protected:` on the line before it, and `public:` on the line after it. * If there was no explicit destructor, add the following to the struct/class, immediately before the closing brace: protected: ~ClassName() {} There are going to be cases which this doesn't handle, and those will produce errors, but there should be much fewer of them than 2345. That being said, we may not need to do that or use _this specific_ analysis. We could gain much of the same benifits by simply using an analysis which produces an error if a destructor is called on a reference-counted record outside of a method named `Release` on said record. This would not require the massive amount of rewriting which this plugin will require to get to work. I'd like your opinion on what to do next ehsan

Comment on attachment 8630830 [details] [diff] [review] Add an analysis to detect refcounted classes with public destructors Clearing the review flag because I'm not sure if we are going to want to use this plugin.

I think it would be interesting to hack up a small script to see how many of these we can fix wholesale. One nice place to start would be the classes with an existing dtor which can be relatively easily made private... But this is probably going to be a large project.

Unfortunately, mfbt's RefCounted class's Release method is implemented by casting the this pointer to it's subclass, and deleting it. This requires the subclass to have a public destructor such that RefCounted can delete it. This patch is going to have to go on hold until we can remove the use of RefCounted from our code base. I've attached the patch I made to clang-tidy to run the pass to add protected qualifiers to destructors of refcounted objects. This patch is meant to be applied on clang's extra tools repository.

(In reply to Michael Layzell [:mystor] from comment #5) > This patch is going to have to go on hold until we can remove the use of > RefCounted from our code base. Nathan might be interested to add this to his list of RefPtr.h brain damage.

I might be wrong, but I believe RefCounted is gone now that RefPtr has been replaced with nsRefPtr. This might be worth looking into fixing again.

(In reply to Michael Layzell [:mystor] from comment #7) > I might be wrong, but I believe RefCounted is gone now that RefPtr has been > replaced with nsRefPtr. This might be worth looking into fixing again. It continues to exist. :( http://dxr.mozilla.org/mozilla-central/source/mfbt/RefCounted.h#90