Check for public dtors on base::RefCounted types

Check and warn if public destructors are found on types that derive from base::RefCounted or base::RefCountedThreadSafe.

Having public destructors is dangerous, as it allows the ref-counted object to be stack allocated and have references that attempt to outlive the stack, or to allow callers to explicitly delete it while references are still held. Both patterns result in use-after-free, hence their prohibition.

In doing so, update the Chromium plugin to be able to scan both headers and implementation files, and enable the public destructor check for both types with an optional flag ( -Xclang -plugin-arg-find-bad-constructs -Xclang skip-refcounted-dtors )

BUG=123295
TEST=none

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=132500

[Ryan Sleevi, 2012-04-05 22:10:40]

Nico: This is what I've been/am/will be using to pick up the bad patterns.

I'll work to extract the two different behaviour changes (RC/RCTS vs OVERRIDE),
with the latter depending ont the former, but I wanted to make sure the design
was sound and to address any review feedback holistically. 

My biggest concern is how expensive the call to (rediscover) the filename is, in
the IsImplementationFile call. I haven't profiled this yet to know whether or
not those concerns are valid (premature optimization and all that), but it's
just a smell worth mentioning to see if you had any feedback.

[Nico, 2012-04-05 23:08:13]

This looks good.

To check performance, please do three clobber builds of a big target ('chrome',
or 'browser_tests') with and without your change and measure build times. It's
probably fine.

Once this is in, please update
http://dev.chromium.org/developers/coding-style/chromium-style-checker-errors
with the new warning.

http://codereview.chromium.org/10005022/diff/1/tools/clang/plugins/FindBadCon...
File tools/clang/plugins/FindBadConstructs.cpp (right):

http://codereview.chromium.org/10005022/diff/1/tools/clang/plugins/FindBadCon...
tools/clang/plugins/FindBadConstructs.cpp:55: std::string base_name =
decl->getNameAsString();
might want to check the namespace too

[Ryan Sleevi, 2012-04-11 20:30:25]

On 2012/04/05 23:08:13, Nico wrote:
> This looks good.
> 
> To check performance, please do three clobber builds of a big target
('chrome',
> or 'browser_tests') with and without your change and measure build times. It's
> probably fine.

Not accounting for hot-cache/cold-cache, it's < 1%

With the change, build times were 24m57.937, 24m8.678, and 24m8.438s
Without the change, build times were 24m7.330, 24m19.816, 24m19.490s

The large spike (24m57.937s) was the first compile out of all of them.

So it seems fine, and I'll upload a new CL with just the RefCounted changes

> 
> Once this is in, please update
> http://dev.chromium.org/developers/coding-style/chromium-style-checker-errors
> with the new warning.
> 
>
http://codereview.chromium.org/10005022/diff/1/tools/clang/plugins/FindBadCon...
> File tools/clang/plugins/FindBadConstructs.cpp (right):
> 
>
http://codereview.chromium.org/10005022/diff/1/tools/clang/plugins/FindBadCon...
> tools/clang/plugins/FindBadConstructs.cpp:55: std::string base_name =
> decl->getNameAsString();
> might want to check the namespace too

[Ryan Sleevi, 2012-04-11 22:18:57]

Nico: Want to take a look at the updated (Just the RefCounted) changes?

This also includes some style nits, which I can happily revert if there was some
Clang style trying to be followed here.

[Nico, 2012-04-11 22:27:26]

Code looks great, but please add a test as well. There's a ghetto 'runtests'
script to run the existing tests (they get executed before every clang push, to
check the plugin didn't break)

http://codereview.chromium.org/10005022/diff/9001/tools/clang/plugins/ChromeC...
File tools/clang/plugins/ChromeClassTester.cpp (right):

http://codereview.chromium.org/10005022/diff/9001/tools/clang/plugins/ChromeC...
tools/clang/plugins/ChromeClassTester.cpp:149: void
ChromeClassTester::emitWarning(const SourceLocation& loc,
ditto

http://codereview.chromium.org/10005022/diff/9001/tools/clang/plugins/ChromeC...
tools/clang/plugins/ChromeClassTester.cpp:215: bool
ChromeClassTester::InBannedDirectory(const SourceLocation& loc) {
ditto

http://codereview.chromium.org/10005022/diff/9001/tools/clang/plugins/ChromeC...
tools/clang/plugins/ChromeClassTester.cpp:271: const SourceManager& SM =
instance_.getSourceManager();
This file tries to follow clang style (it's copied into the clang tree for being
built), so it's " &" not "& " (yeah, confusing)

http://codereview.chromium.org/10005022/diff/9001/tools/clang/plugins/ChromeC...
File tools/clang/plugins/ChromeClassTester.h (right):

http://codereview.chromium.org/10005022/diff/9001/tools/clang/plugins/ChromeC...
tools/clang/plugins/ChromeClassTester.h:60: bool InBannedDirectory(const
clang::SourceLocation& loc);
a sourcelocation is an int, passing these by value is fine

http://codereview.chromium.org/10005022/diff/9001/tools/clang/plugins/ChromeC...
tools/clang/plugins/ChromeClassTester.h:65: bool GetFilename(const
clang::SourceLocation& loc,
ditto

[Ryan Sleevi, 2012-04-12 04:21:53]

Nico, PTAL once more - now with tests and proper handling of typedefs and
qualified-ids.

I'll be re-running this building All to see if I missed any targets.

[Ryan Sleevi, 2012-04-13 22:16:04]

Nico: Per IRC, I added a flag so that this is disabled by default. Please take a
look, as I'll follow up with another flag for the virtual+OVERRIDE in
implementation files

[Nico, 2012-04-13 22:40:55]

Few nits

http://codereview.chromium.org/10005022/diff/18001/tools/clang/plugins/FindBa...
File tools/clang/plugins/FindBadConstructs.cpp (right):

http://codereview.chromium.org/10005022/diff/18001/tools/clang/plugins/FindBa...
tools/clang/plugins/FindBadConstructs.cpp:40: const Type* UnwrapType(const Type*
type) {
Does Type::getDesugaredType() do some of this?

If not: I'd write this function as:

const Type* UnwrapType(const Type* type) {
  if (ElaboratedType* elaborated = dyn_cast<ElaboratedType>(type))
    return UnwrapType(elaborated->getNamedType().getTypePtr());
  if (TypedefType typedefd = dyn_cast(<TypedefType>(type))
    return UnwrapType(typedefd->desugar().getTypePtr());
  return type;

http://codereview.chromium.org/10005022/diff/18001/tools/clang/plugins/FindBa...
tools/clang/plugins/FindBadConstructs.cpp:81: if (!skip_refcounted_dtors_)
double negation like this is confusing. Call the variable
"check_refcounted_dtors". You can keep the flag name as is if you want though.

http://codereview.chromium.org/10005022/diff/18001/tools/clang/plugins/tests/...
File tools/clang/plugins/tests/base_refcounted.h (right):

http://codereview.chromium.org/10005022/diff/18001/tools/clang/plugins/tests/...
tools/clang/plugins/tests/base_refcounted.h:10: // Define a 'dummy' set of
refcounting templates
useless comment, remove

http://codereview.chromium.org/10005022/diff/18001/tools/clang/plugins/tests/...
tools/clang/plugins/tests/base_refcounted.h:27: // However, WebKit ref-counted
classes should be ignored.
You might want to explain somewhere why (say, in the CL description)

http://codereview.chromium.org/10005022/diff/18001/tools/clang/plugins/tests/...
File tools/clang/plugins/tests/inline_ctor.h (right):

http://codereview.chromium.org/10005022/diff/18001/tools/clang/plugins/tests/...
tools/clang/plugins/tests/inline_ctor.h:5: #ifndef INLINE_CTOR_H_
:-)

[Ryan Sleevi, 2012-04-14 00:16:29]

http://codereview.chromium.org/10005022/diff/18001/tools/clang/plugins/FindBa...
File tools/clang/plugins/FindBadConstructs.cpp (right):

http://codereview.chromium.org/10005022/diff/18001/tools/clang/plugins/FindBa...
tools/clang/plugins/FindBadConstructs.cpp:40: const Type* UnwrapType(const Type*
type) {
On 2012/04/13 22:40:55, Nico wrote:
> Does Type::getDesugaredType() do some of this?
> 
> If not: I'd write this function as:
> 
> const Type* UnwrapType(const Type* type) {
>   if (ElaboratedType* elaborated = dyn_cast<ElaboratedType>(type))
>     return UnwrapType(elaborated->getNamedType().getTypePtr());
>   if (TypedefType typedefd = dyn_cast(<TypedefType>(type))
>     return UnwrapType(typedefd->desugar().getTypePtr());
>   return type;

As far as I can tell, it does too much or not enough.

Desugaring all the types ends up leading to RecordTypes, which are the fully
qualified templates (so != TemplateSpecializationType). I haven't figured out
how to walk back up from the TemplateSpecializationType.

getUnqualifiedType doesn't delve into the fully de-sugared typedef.

The alternative (and perhaps more correct) would be to singleStepDesugar through
each of the sugared types and then compare if it was a template. However, the
only other sugared type I'm missing, that I can tell, is C++ Autos, and we won't
see those in class decls, so this is probably fine.

[Ryan Sleevi, 2012-04-14 00:17:22]

alrighty, I'm feelin lucky. Any more nits?

[Nico, 2012-04-14 00:48:55]

lgtm

[commit-bot: I haz the power, 2012-04-14 01:08:45]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10005022/21002

[Nico, 2012-04-14 01:14:25]

No need for cq, that tests nothing for this patch

[commit-bot: I haz the power, 2012-04-14 03:44:24]

Try job failure for 10005022-21002 (retry) on mac_rel for step "browser_tests".
It's a second try, previously, step "browser_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...