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

Review URL: https://chromiumcodereview.appspot.com/10005022

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@132500 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 502 insertions, 155 deletions

diff --git a/tools/clang/plugins/ChromeClassTester.cpp b/tools/clang/plugins/ChromeClassTester.cpp
index dd983bd..7a3b89e 100644
--- a/tools/clang/plugins/ChromeClassTester.cpp
+++ b/tools/clang/plugins/ChromeClassTester.cpp
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -40,72 +40,6 @@ ChromeClassTester::ChromeClassTester(CompilerInstance& instance)
   BuildBannedLists();
 }
 
-void ChromeClassTester::BuildBannedLists() {
-  banned_namespaces_.push_back("std");
-  banned_namespaces_.push_back("__gnu_cxx");
-  banned_namespaces_.push_back("WebKit");
-
-  banned_directories_.push_back("third_party/");
-  banned_directories_.push_back("native_client/");
-  banned_directories_.push_back("breakpad/");
-  banned_directories_.push_back("courgette/");
-  banned_directories_.push_back("pdf/");
-  banned_directories_.push_back("ppapi/");
-  banned_directories_.push_back("usr/");
-  banned_directories_.push_back("testing/");
-  banned_directories_.push_back("googleurl/");
-  banned_directories_.push_back("v8/");
-  banned_directories_.push_back("dart/");
-  banned_directories_.push_back("sdch/");
-  banned_directories_.push_back("icu4c/");
-  banned_directories_.push_back("frameworks/");
-
-  // Don't check autogenerated headers.
-  // Make puts them below $(builddir_name)/.../gen and geni.
-  // Ninja puts them below OUTPUT_DIR/.../gen
-  // Xcode has a fixed output directory for everything.
-  banned_directories_.push_back("gen/");
-  banned_directories_.push_back("geni/");
-  banned_directories_.push_back("xcodebuild/");
-
-  // You are standing in a mazy of twisty dependencies, all resolved by
-  // putting everything in the header.
-  banned_directories_.push_back("automation/");
-
-  // Don't check system headers.
-  banned_directories_.push_back("/Developer/");
-
-  // Used in really low level threading code that probably shouldn't be out of
-  // lined.
-  ignored_record_names_.insert("ThreadLocalBoolean");
-
-  // A complicated pickle derived struct that is all packed integers.
-  ignored_record_names_.insert("Header");
-
-  // Part of the GPU system that uses multiple included header
-  // weirdness. Never getting this right.
-  ignored_record_names_.insert("Validators");
-
-  // Has a UNIT_TEST only constructor. Isn't *terribly* complex...
-  ignored_record_names_.insert("AutocompleteController");
-  ignored_record_names_.insert("HistoryURLProvider");
-
-  // Because of chrome frame
-  ignored_record_names_.insert("ReliabilityTestSuite");
-
-  // Used over in the net unittests. A large enough bundle of integers with 1
-  // non-pod class member. Probably harmless.
-  ignored_record_names_.insert("MockTransaction");
-
-  // Used heavily in ui_unittests and once in views_unittests. Fixing this
-  // isn't worth the overhead of an additional library.
-  ignored_record_names_.insert("TestAnimationDelegate");
-
-  // Part of our public interface that nacl and friends use. (Arguably, this
-  // should mean that this is a higher priority but fixing this looks hard.)
-  ignored_record_names_.insert("PluginVersionInfo");
-}
-
 ChromeClassTester::~ChromeClassTester() {}
 
 void ChromeClassTester::HandleTagDeclDefinition(TagDecl* tag) {
@@ -146,7 +80,8 @@ void ChromeClassTester::HandleTagDeclDefinition(TagDecl* tag) {
   }
 }
 
-void ChromeClassTester::emitWarning(SourceLocation loc, const char* raw_error) {
+void ChromeClassTester::emitWarning(SourceLocation loc,
+                                    const char* raw_error) {
   FullSourceLoc full(loc, instance().getSourceManager());
   std::string err;
   err = "[chromium-style] ";
@@ -159,10 +94,6 @@ void ChromeClassTester::emitWarning(SourceLocation loc, const char* raw_error) {
   DiagnosticBuilder B = diagnostic().Report(full, id);
 }
 
-bool ChromeClassTester::InTestingNamespace(const Decl* record) {
-  return GetNamespace(record).find("testing") != std::string::npos;
-}
-
 bool ChromeClassTester::InBannedNamespace(const Decl* record) {
   std::string n = GetNamespace(record);
   if (n != "") {
@@ -177,8 +108,88 @@ std::string ChromeClassTester::GetNamespace(const Decl* record) {
   return GetNamespaceImpl(record->getDeclContext(), "");
 }
 
+bool ChromeClassTester::InImplementationFile(SourceLocation record_location) {
+  std::string filename;
+  if (!GetFilename(record_location, &filename)) {
+    return false;
+  }
+
+  if (ends_with(filename, ".cc") || ends_with(filename, ".cpp") ||
+      ends_with(filename, ".mm")) {
+    return true;
+  }
+
+  return false;
+}
+
+void ChromeClassTester::BuildBannedLists() {
+  banned_namespaces_.push_back("std");
+  banned_namespaces_.push_back("__gnu_cxx");
+  banned_namespaces_.push_back("WebKit");
+
+  banned_directories_.push_back("third_party/");
+  banned_directories_.push_back("native_client/");
+  banned_directories_.push_back("breakpad/");
+  banned_directories_.push_back("courgette/");
+  banned_directories_.push_back("pdf/");
+  banned_directories_.push_back("ppapi/");
+  banned_directories_.push_back("usr/");
+  banned_directories_.push_back("testing/");
+  banned_directories_.push_back("googleurl/");
+  banned_directories_.push_back("v8/");
+  banned_directories_.push_back("dart/");
+  banned_directories_.push_back("sdch/");
+  banned_directories_.push_back("icu4c/");
+  banned_directories_.push_back("frameworks/");
+
+  // Don't check autogenerated headers.
+  // Make puts them below $(builddir_name)/.../gen and geni.
+  // Ninja puts them below OUTPUT_DIR/.../gen
+  // Xcode has a fixed output directory for everything.
+  banned_directories_.push_back("gen/");
+  banned_directories_.push_back("geni/");
+  banned_directories_.push_back("xcodebuild/");
+
+  // You are standing in a mazy of twisty dependencies, all resolved by
+  // putting everything in the header.
+  banned_directories_.push_back("automation/");
+
+  // Don't check system headers.
+  banned_directories_.push_back("/Developer/");
+
+  // Used in really low level threading code that probably shouldn't be out of
+  // lined.
+  ignored_record_names_.insert("ThreadLocalBoolean");
+
+  // A complicated pickle derived struct that is all packed integers.
+  ignored_record_names_.insert("Header");
+
+  // Part of the GPU system that uses multiple included header
+  // weirdness. Never getting this right.
+  ignored_record_names_.insert("Validators");
+
+  // Has a UNIT_TEST only constructor. Isn't *terribly* complex...
+  ignored_record_names_.insert("AutocompleteController");
+  ignored_record_names_.insert("HistoryURLProvider");
+
+  // Because of chrome frame
+  ignored_record_names_.insert("ReliabilityTestSuite");
+
+  // Used over in the net unittests. A large enough bundle of integers with 1
+  // non-pod class member. Probably harmless.
+  ignored_record_names_.insert("MockTransaction");
+
+  // Used heavily in ui_unittests and once in views_unittests. Fixing this
+  // isn't worth the overhead of an additional library.
+  ignored_record_names_.insert("TestAnimationDelegate");
+
+  // Part of our public interface that nacl and friends use. (Arguably, this
+  // should mean that this is a higher priority but fixing this looks hard.)
+  ignored_record_names_.insert("PluginVersionInfo");
+}
+
 std::string ChromeClassTester::GetNamespaceImpl(const DeclContext* context,
-                                                std::string candidate) {
+                                                const std::string& candidate) {
   switch (context->getDeclKind()) {
     case Decl::TranslationUnit: {
       return candidate;
@@ -201,60 +212,49 @@ std::string ChromeClassTester::GetNamespaceImpl(const DeclContext* context,
 }
 
 bool ChromeClassTester::InBannedDirectory(SourceLocation loc) {
-  const SourceManager &SM = instance_.getSourceManager();
-  SourceLocation spelling_location = SM.getSpellingLoc(loc);
-  PresumedLoc ploc = SM.getPresumedLoc(spelling_location);
-  std::string buffer_name;
-  if (ploc.isInvalid()) {
-    // If we're in an invalid location, we're looking at things that aren't
-    // actually stated in the source; treat this as a banned location instead
-    // of going through our full lookup process.
+  std::string filename;
+  if (!GetFilename(loc, &filename)) {
+    // If the filename cannot be determined, simply treat this as a banned
+    // location, instead of going through the full lookup process.
     return true;
-  } else {
-    std::string b = ploc.getFilename();
-
-    // We need to special case scratch space; which is where clang does its
-    // macro expansion. We explicitly want to allow people to do otherwise bad
-    // things through macros that were defined due to third party libraries.
-    if (b == "<scratch space>")
-      return true;
-
-    // Don't complain about these things in implementation files.
-    if (ends_with(b, ".cc") || ends_with(b, ".cpp") || ends_with(b, ".mm")) {
-      return true;
-    }
+  }
 
-    // Don't complain about autogenerated protobuf files.
-    if (ends_with(b, ".pb.h")) {
-      return true;
-    }
+  // We need to special case scratch space; which is where clang does its
+  // macro expansion. We explicitly want to allow people to do otherwise bad
+  // things through macros that were defined due to third party libraries.
+  if (filename == "<scratch space>")
+    return true;
 
-    // We need to munge the paths so that they are relative to the repository
-    // srcroot. We first resolve the symlinktastic relative path and then
-    // remove our known srcroot from it if needed.
-    char resolvedPath[MAXPATHLEN];
-    if (realpath(b.c_str(), resolvedPath)) {
-      b = resolvedPath;
-    }
+  // Don't complain about autogenerated protobuf files.
+  if (ends_with(filename, ".pb.h")) {
+    return true;
+  }
+
+  // We need to munge the paths so that they are relative to the repository
+  // srcroot. We first resolve the symlinktastic relative path and then
+  // remove our known srcroot from it if needed.
+  char resolvedPath[MAXPATHLEN];
+  if (realpath(filename.c_str(), resolvedPath)) {
+    filename = resolvedPath;
+  }
 
-    // On linux, chrome is often checked out to /usr/local/google. Due to the
-    // "usr" rule in banned_directories_, all diagnostics would be suppressed
-    // in that case. As a workaround, strip that prefix.
-    b = lstrip(b, "/usr/local/google");
-
-    for (std::vector<std::string>::const_iterator it =
-             banned_directories_.begin();
-         it != banned_directories_.end(); ++it) {
-      // If we can find any of the banned path components in this path, then
-      // this file is rejected.
-      size_t index = b.find(*it);
-      if (index != std::string::npos) {
-        bool matches_full_dir_name = index == 0 || b[index - 1] == '/';
-        if ((*it)[0] == '/')
-          matches_full_dir_name = true;
-        if (matches_full_dir_name)
-          return true;
-      }
+  // On linux, chrome is often checked out to /usr/local/google. Due to the
+  // "usr" rule in banned_directories_, all diagnostics would be suppressed
+  // in that case. As a workaround, strip that prefix.
+  filename = lstrip(filename, "/usr/local/google");
+
+  for (std::vector<std::string>::const_iterator it =
+           banned_directories_.begin();
+       it != banned_directories_.end(); ++it) {
+    // If we can find any of the banned path components in this path, then
+    // this file is rejected.
+    size_t index = filename.find(*it);
+    if (index != std::string::npos) {
+      bool matches_full_dir_name = index == 0 || filename[index - 1] == '/';
+      if ((*it)[0] == '/')
+        matches_full_dir_name = true;
+      if (matches_full_dir_name)
+        return true;
     }
   }
 
@@ -264,3 +264,18 @@ bool ChromeClassTester::InBannedDirectory(SourceLocation loc) {
 bool ChromeClassTester::IsIgnoredType(const std::string& base_name) {
   return ignored_record_names_.find(base_name) != ignored_record_names_.end();
 }
+
+bool ChromeClassTester::GetFilename(SourceLocation loc,
+                                    std::string* filename) {
+  const SourceManager &SM = instance_.getSourceManager();
+  SourceLocation spelling_location = SM.getSpellingLoc(loc);
+  PresumedLoc ploc = SM.getPresumedLoc(spelling_location);
+  if (ploc.isInvalid()) {
+    // If we're in an invalid location, we're looking at things that aren't
+    // actually stated in the source.
+    return false;
+  }
+
+  *filename = ploc.getFilename();
+  return true;
+}
diff --git a/tools/clang/plugins/ChromeClassTester.h b/tools/clang/plugins/ChromeClassTester.h
index 5004f56..5994a71 100644
--- a/tools/clang/plugins/ChromeClassTester.h
+++ b/tools/clang/plugins/ChromeClassTester.h
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -21,8 +21,6 @@ class ChromeClassTester : public clang::ASTConsumer {
   explicit ChromeClassTester(clang::CompilerInstance& instance);
   virtual ~ChromeClassTester();
 
-  void BuildBannedLists();
-
   // ASTConsumer:
   virtual void HandleTagDeclDefinition(clang::TagDecl* tag);
 
@@ -34,29 +32,38 @@ class ChromeClassTester : public clang::ASTConsumer {
   // printing.
   void emitWarning(clang::SourceLocation loc, const char* error);
 
-  // Utility method for subclasses to check if testing details are in this
-  // class. Some tests won't care if a class has a ::testing member and others
-  // will.
-  bool InTestingNamespace(const clang::Decl* record);
-
   // Utility method for subclasses to check if this class is in a banned
   // namespace.
   bool InBannedNamespace(const clang::Decl* record);
 
+  // Utility method for subclasses to determine the namespace of the
+  // specified record, if any. Unnamed namespaces will be identified as
+  // "<anonymous namespace>".
+  std::string GetNamespace(const clang::Decl* record);
+
+  // Utility method for subclasses to check if this class is within an
+  // implementation (.cc, .cpp, .mm) file.
+  bool InImplementationFile(clang::SourceLocation location);
+
  private:
+  void BuildBannedLists();
+
   // Filtered versions of tags that are only called with things defined in
   // chrome header files.
-  virtual void CheckChromeClass(const clang::SourceLocation& record_location,
+  virtual void CheckChromeClass(clang::SourceLocation record_location,
                                 clang::CXXRecordDecl* record) = 0;
 
   // Utility methods used for filtering out non-chrome classes (and ones we
   // deliberately ignore) in HandleTagDeclDefinition().
-  std::string GetNamespace(const clang::Decl* record);
   std::string GetNamespaceImpl(const clang::DeclContext* context,
-                               std::string candidate);
+                               const std::string& candidate);
   bool InBannedDirectory(clang::SourceLocation loc);
   bool IsIgnoredType(const std::string& base_name);
 
+  // Attempts to determine the filename for the given SourceLocation.
+  // Returns false if the filename could not be determined.
+  bool GetFilename(clang::SourceLocation loc, std::string* filename);
+
   clang::CompilerInstance& instance_;
   clang::DiagnosticsEngine& diagnostic_;
 
diff --git a/tools/clang/plugins/FindBadConstructs.cpp b/tools/clang/plugins/FindBadConstructs.cpp
index 4b2f800..193fc84 100644
--- a/tools/clang/plugins/FindBadConstructs.cpp
+++ b/tools/clang/plugins/FindBadConstructs.cpp
@@ -1,4 +1,4 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
@@ -10,14 +10,13 @@
 // - Missing "virtual" keywords on methods that should be virtual.
 // - Non-annotated overriding virtual methods.
 // - Virtual methods with nonempty implementations in their headers.
-//
-// Things that are still TODO:
-// - Deriving from base::RefCounted and friends should mandate non-public
-//   destructors.
+// - Classes that derive from base::RefCounted / base::RefCountedThreadSafe
+//   should have protected or private destructors.
 
 #include "clang/Frontend/FrontendPluginRegistry.h"
 #include "clang/AST/ASTConsumer.h"
 #include "clang/AST/AST.h"
+#include "clang/AST/CXXInheritance.h"
 #include "clang/AST/TypeLoc.h"
 #include "clang/Basic/SourceManager.h"
 #include "clang/Frontend/CompilerInstance.h"
@@ -36,20 +35,109 @@ bool TypeHasNonTrivialDtor(const Type* type) {
   return false;
 }
 
+// Returns the underlying Type for |type| by expanding typedefs and removing
+// any namespace qualifiers.
+const Type* UnwrapType(const Type* type) {
+  if (const ElaboratedType* elaborated = dyn_cast<ElaboratedType>(type))
+    return UnwrapType(elaborated->getNamedType().getTypePtr());
+  if (const TypedefType* typedefed = dyn_cast<TypedefType>(type))
+    return UnwrapType(typedefed->desugar().getTypePtr());
+  return type;
+}
+
 // Searches for constructs that we know we don't want in the Chromium code base.
 class FindBadConstructsConsumer : public ChromeClassTester {
  public:
-  FindBadConstructsConsumer(CompilerInstance& instance)
-      : ChromeClassTester(instance) {}
+  FindBadConstructsConsumer(CompilerInstance& instance,
+                            bool check_refcounted_dtors)
+      : ChromeClassTester(instance),
+        check_refcounted_dtors_(check_refcounted_dtors) {
+  }
 
-  virtual void CheckChromeClass(const SourceLocation& record_location,
+  virtual void CheckChromeClass(SourceLocation record_location,
                                 CXXRecordDecl* record) {
-    CheckCtorDtorWeight(record_location, record);
-    CheckVirtualMethods(record_location, record);
+    bool implementation_file = InImplementationFile(record_location);
+
+    if (!implementation_file) {
+      // Only check for "heavy" constructors/destructors in header files;
+      // within implementation files, there is no performance cost.
+      CheckCtorDtorWeight(record_location, record);
+
+      // Check that all virtual methods are marked accordingly with both
+      // virtual and OVERRIDE.
+      CheckVirtualMethods(record_location, record);
+    }
+
+    if (check_refcounted_dtors_)
+      CheckRefCountedDtors(record_location, record);
+  }
+
+ private:
+  bool check_refcounted_dtors_;
+
+  // Returns true if |base| specifies one of the Chromium reference counted
+  // classes (base::RefCounted / base::RefCountedThreadSafe). |user_data| is
+  // ignored.
+  static bool IsRefCountedCallback(const CXXBaseSpecifier* base,
+                                   CXXBasePath& path,
+                                   void* user_data) {
+    FindBadConstructsConsumer* self =
+        static_cast<FindBadConstructsConsumer*>(user_data);
+
+    const TemplateSpecializationType* base_type =
+        dyn_cast<TemplateSpecializationType>(
+            UnwrapType(base->getType().getTypePtr()));
+    if (!base_type) {
+      // Base-most definition is not a template, so this cannot derive from
+      // base::RefCounted. However, it may still be possible to use with a
+      // scoped_refptr<> and support ref-counting, so this is not a perfect
+      // guarantee of safety.
+      return false;
+    }
+
+    TemplateName name = base_type->getTemplateName();
+    if (TemplateDecl* decl = name.getAsTemplateDecl()) {
+      std::string base_name = decl->getNameAsString();
+
+      // Check for both base::RefCounted and base::RefCountedThreadSafe.
+      if (base_name.compare(0, 10, "RefCounted") == 0 &&
+          self->GetNamespace(decl) == "base") {
+        return true;
+      }
+    }
+    return false;
+  }
+
+  // Prints errors if the destructor of a RefCounted class is public.
+  void CheckRefCountedDtors(SourceLocation record_location,
+                            CXXRecordDecl* record) {
+    // Skip anonymous structs.
+    if (record->getIdentifier() == NULL)
+      return;
+
+    CXXBasePaths paths;
+    if (!record->lookupInBases(
+            &FindBadConstructsConsumer::IsRefCountedCallback, this, paths)) {
+      return;  // Class does not derive from a ref-counted base class.
+    }
+
+    if (!record->hasUserDeclaredDestructor()) {
+      emitWarning(
+          record_location,
+          "Classes that are ref-counted should have explicit "
+          "destructors that are protected or private.");
+    } else if (CXXDestructorDecl* dtor = record->getDestructor()) {
+      if (dtor->getAccess() == AS_public) {
+        emitWarning(
+            dtor->getInnerLocStart(),
+            "Classes that are ref-counted should not have "
+            "public destructors.");
+      }
+    }
   }
 
   // Prints errors if the constructor/destructor weight is too heavy.
-  void CheckCtorDtorWeight(const SourceLocation& record_location,
+  void CheckCtorDtorWeight(SourceLocation record_location,
                            CXXRecordDecl* record) {
     // We don't handle anonymous structs. If this record doesn't have a
     // name, it's of the form:
@@ -161,6 +249,10 @@ class FindBadConstructsConsumer : public ChromeClassTester {
     }
   }
 
+  bool InTestingNamespace(const Decl* record) {
+    return GetNamespace(record).find("testing") != std::string::npos;
+  }
+
   bool IsMethodInBannedNamespace(const CXXMethodDecl* method) {
     if (InBannedNamespace(method))
       return true;
@@ -196,7 +288,7 @@ class FindBadConstructsConsumer : public ChromeClassTester {
   // trick to get around that. If a class has member variables whose types are
   // in the "testing" namespace (which is how gmock works behind the scenes),
   // there's a really high chance we won't care about these errors
-  void CheckVirtualMethods(const SourceLocation& record_location,
+  void CheckVirtualMethods(SourceLocation record_location,
                            CXXRecordDecl* record) {
     for (CXXRecordDecl::field_iterator it = record->field_begin();
          it != record->field_end(); ++it) {
@@ -283,16 +375,32 @@ class FindBadConstructsConsumer : public ChromeClassTester {
 };
 
 class FindBadConstructsAction : public PluginASTAction {
+ public:
+  FindBadConstructsAction() : check_refcounted_dtors_(true) {}
+
  protected:
   ASTConsumer* CreateASTConsumer(CompilerInstance &CI, llvm::StringRef ref) {
-    return new FindBadConstructsConsumer(CI);
+    return new FindBadConstructsConsumer(CI, check_refcounted_dtors_);
   }
 
   bool ParseArgs(const CompilerInstance &CI,
                  const std::vector<std::string>& args) {
-    // We don't take any additional arguments here.
-    return true;
+    bool parsed = true;
+
+    for (size_t i = 0; i < args.size() && parsed; ++i) {
+      if (args[i] == "skip-refcounted-dtors") {
+        check_refcounted_dtors_ = false;
+      } else {
+        parsed = false;
+        llvm::errs() << "Unknown argument: " << args[i] << "\n";
+      }
+    }
+
+    return parsed;
   }
+
+ private:
+  bool check_refcounted_dtors_;
 };
 
 }  // namespace
diff --git a/tools/clang/plugins/tests/base_refcounted.cpp b/tools/clang/plugins/tests/base_refcounted.cpp
new file mode 100644
index 0000000..364a3e8
--- /dev/null
+++ b/tools/clang/plugins/tests/base_refcounted.cpp
@@ -0,0 +1,72 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base_refcounted.h"
+
+#include <cstddef>
+
+namespace {
+
+// Unsafe; should error.
+class AnonymousDerivedProtectedToPublicInImpl
+    : public ProtectedRefCountedDtorInHeader {
+ public:
+  AnonymousDerivedProtectedToPublicInImpl() {}
+  ~AnonymousDerivedProtectedToPublicInImpl() {}
+};
+
+}  // namespace
+
+// Unsafe; should error.
+class PublicRefCountedDtorInImpl
+    : public base::RefCounted<PublicRefCountedDtorInImpl> {
+ public:
+  PublicRefCountedDtorInImpl() {}
+  ~PublicRefCountedDtorInImpl() {}
+
+ private:
+  friend class base::RefCounted<PublicRefCountedDtorInImpl>;
+};
+
+class Foo {
+ public:
+  class BarInterface {
+   protected:
+    virtual ~BarInterface() {}
+  };
+
+  typedef base::RefCounted<BarInterface> RefCountedBar;
+  typedef RefCountedBar AnotherTypedef;
+};
+
+class Baz {
+ public:
+  typedef typename Foo::AnotherTypedef MyLocalTypedef;
+};
+
+// Unsafe; should error.
+class UnsafeTypedefChainInImpl : public Baz::MyLocalTypedef {
+ public:
+  UnsafeTypedefChainInImpl() {}
+  ~UnsafeTypedefChainInImpl() {}
+};
+
+int main() {
+  PublicRefCountedDtorInHeader bad;
+  PublicRefCountedDtorInImpl also_bad;
+
+  ProtectedRefCountedDtorInHeader* protected_ok = NULL;
+  PrivateRefCountedDtorInHeader* private_ok = NULL;
+
+  DerivedProtectedToPublicInHeader still_bad;
+  PublicRefCountedThreadSafeDtorInHeader another_bad_variation;
+  AnonymousDerivedProtectedToPublicInImpl and_this_is_bad_too;
+  ImplicitDerivedProtectedToPublicInHeader bad_yet_again;
+  UnsafeTypedefChainInImpl and_again_this_is_bad;
+
+  WebKitPublicDtorInHeader ignored;
+  WebKitDerivedPublicDtorInHeader still_ignored;
+
+  return 0;
+}
diff --git a/tools/clang/plugins/tests/base_refcounted.h b/tools/clang/plugins/tests/base_refcounted.h
new file mode 100644
index 0000000..1e53215
--- /dev/null
+++ b/tools/clang/plugins/tests/base_refcounted.h
@@ -0,0 +1,121 @@
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_REFCOUNTED_H_
+#define BASE_REFCOUNTED_H_
+
+namespace base {
+
+template <typename T>
+class RefCounted {
+ public:
+  RefCounted() {}
+  ~RefCounted() {}
+};
+
+template <typename T>
+class RefCountedThreadSafe {
+ public:
+  RefCountedThreadSafe() {}
+  ~RefCountedThreadSafe() {}
+};
+
+}  // namespace base
+
+// Ignore classes whose inheritance tree ends in WebKit's RefCounted base
+// class. Though prone to error, this pattern is very prevalent in WebKit
+// code, so do not issue any warnings.
+namespace WebKit {
+
+template <typename T>
+class RefCounted {
+ public:
+  RefCounted() {}
+  ~RefCounted() {}
+};
+
+}  // namespace WebKit
+
+// Unsafe; should error.
+class PublicRefCountedDtorInHeader
+    : public base::RefCounted<PublicRefCountedDtorInHeader> {
+ public:
+  PublicRefCountedDtorInHeader() {}
+  ~PublicRefCountedDtorInHeader() {}
+
+ private:
+  friend class base::RefCounted<PublicRefCountedDtorInHeader>;
+};
+
+// Unsafe; should error.
+class PublicRefCountedThreadSafeDtorInHeader
+    : public base::RefCountedThreadSafe<
+          PublicRefCountedThreadSafeDtorInHeader> {
+ public:
+  PublicRefCountedThreadSafeDtorInHeader() {}
+  ~PublicRefCountedThreadSafeDtorInHeader() {}
+
+ private:
+  friend class base::RefCountedThreadSafe<
+      PublicRefCountedThreadSafeDtorInHeader>;
+};
+
+// Safe; should not have errors.
+class ProtectedRefCountedDtorInHeader
+    : public base::RefCounted<ProtectedRefCountedDtorInHeader> {
+ public:
+  ProtectedRefCountedDtorInHeader() {}
+
+ protected:
+  ~ProtectedRefCountedDtorInHeader() {}
+
+ private:
+  friend class base::RefCounted<ProtectedRefCountedDtorInHeader>;
+};
+
+// Safe; should not have errors.
+class PrivateRefCountedDtorInHeader
+    : public base::RefCounted<PrivateRefCountedDtorInHeader> {
+ public:
+  PrivateRefCountedDtorInHeader() {}
+
+ private:
+  ~PrivateRefCountedDtorInHeader() {}
+  friend class base::RefCounted<PrivateRefCountedDtorInHeader>;
+};
+
+// Unsafe; A grandchild class ends up exposing their parent and grandparent's
+// destructors.
+class DerivedProtectedToPublicInHeader
+    : public ProtectedRefCountedDtorInHeader {
+ public:
+  DerivedProtectedToPublicInHeader() {}
+  ~DerivedProtectedToPublicInHeader() {}
+};
+
+// Unsafe; A grandchild ends up implicitly exposing their parent and
+// grantparent's destructors.
+class ImplicitDerivedProtectedToPublicInHeader
+    : public ProtectedRefCountedDtorInHeader {
+ public:
+  ImplicitDerivedProtectedToPublicInHeader() {}
+};
+
+// Unsafe-but-ignored; should not have errors.
+class WebKitPublicDtorInHeader
+    : public WebKit::RefCounted<WebKitPublicDtorInHeader> {
+ public:
+  WebKitPublicDtorInHeader() {}
+  ~WebKitPublicDtorInHeader() {}
+};
+
+// Unsafe-but-ignored; should not have errors.
+class WebKitDerivedPublicDtorInHeader
+    : public WebKitPublicDtorInHeader {
+ public:
+  WebKitDerivedPublicDtorInHeader() {}
+  ~WebKitDerivedPublicDtorInHeader() {}
+};
+
+#endif  // BASE_REFCOUNTED_H_
diff --git a/tools/clang/plugins/tests/base_refcounted.txt b/tools/clang/plugins/tests/base_refcounted.txt
new file mode 100644
index 0000000..4626424
--- /dev/null
+++ b/tools/clang/plugins/tests/base_refcounted.txt
@@ -0,0 +1,23 @@
+In file included from base_refcounted.cpp:5:
+./base_refcounted.h:45:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
+  ~PublicRefCountedDtorInHeader() {}
+  ^
+./base_refcounted.h:57:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
+  ~PublicRefCountedThreadSafeDtorInHeader() {}
+  ^
+./base_refcounted.h:94:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
+  ~DerivedProtectedToPublicInHeader() {}
+  ^
+./base_refcounted.h:99:1: warning: [chromium-style] Classes that are ref-counted should have explicit destructors that are protected or private.
+class ImplicitDerivedProtectedToPublicInHeader
+^
+base_refcounted.cpp:16:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
+  ~AnonymousDerivedProtectedToPublicInImpl() {}
+  ^
+base_refcounted.cpp:26:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
+  ~PublicRefCountedDtorInImpl() {}
+  ^
+base_refcounted.cpp:52:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
+  ~UnsafeTypedefChainInImpl() {}
+  ^
+7 warnings generated.
diff --git a/tools/clang/plugins/tests/inline_ctor.h b/tools/clang/plugins/tests/inline_ctor.h
index ce2685e..d053b2f 100644
--- a/tools/clang/plugins/tests/inline_ctor.h
+++ b/tools/clang/plugins/tests/inline_ctor.h
@@ -1,9 +1,9 @@
-// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
-#ifndef INCLINE_CTOR_H_
-#define INCLINE_CTOR_H_
+#ifndef INLINE_CTOR_H_
+#define INLINE_CTOR_H_
 
 #include <string>
 #include <vector>
@@ -18,4 +18,4 @@ class InlineCtorsArentOKInHeader {
   std::vector<std::string> two_;
 };
 
-#endif  // INCLINE_CTOR_H_
+#endif  // INLINE_CTOR_H_
diff --git a/tools/clang/scripts/plugin_flags.sh b/tools/clang/scripts/plugin_flags.sh
index ec3eed6c..bd9f547 100755
--- a/tools/clang/scripts/plugin_flags.sh
+++ b/tools/clang/scripts/plugin_flags.sh
@@ -1,5 +1,5 @@
 #!/bin/bash
-# Copyright (c) 2011 The Chromium Authors. All rights reserved.
+# Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 
@@ -17,4 +17,5 @@ else
 fi
 
 echo -Xclang -load -Xclang $CLANG_LIB_PATH/libFindBadConstructs.$LIBSUFFIX \
-  -Xclang -add-plugin -Xclang find-bad-constructs
+  -Xclang -add-plugin -Xclang find-bad-constructs \
+  -Xclang -plugin-arg-find-bad-constructs -Xclang skip-refcounted-dtors