Warn for non-virtual protected destructors of RefCounted classes

This would catch bugs like crbug.com/171760

BUG=

Review URL: https://codereview.chromium.org/12061005

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

4 files changed, 104 insertions, 79 deletions

diff --git a/tools/clang/plugins/FindBadConstructs.cpp b/tools/clang/plugins/FindBadConstructs.cpp
index 59d4d6e..80b2d61 100644
--- a/tools/clang/plugins/FindBadConstructs.cpp
+++ b/tools/clang/plugins/FindBadConstructs.cpp
@@ -33,14 +33,19 @@ const char kNoExplicitDtor[] =
     "[chromium-style] Classes that are ref-counted should have explicit "
     "destructors that are declared protected or private.";
 const char kPublicDtor[] =
-    "[chromium-style] Classes that are ref-counted should not have "
-    "public destructors.";
+    "[chromium-style] Classes that are ref-counted should have "
+    "destructors that are declared protected or private.";
+const char kProtectedNonVirtualDtor[] =
+    "[chromium-style] Classes that are ref-counted and have non-private "
+    "destructors should declare their destructor virtual.";
 const char kNoteInheritance[] =
     "[chromium-style] %0 inherits from %1 here";
 const char kNoteImplicitDtor[] =
     "[chromium-style] No explicit destructor for %0 defined";
 const char kNotePublicDtor[] =
     "[chromium-style] Public destructor declared here";
+const char kNoteProtectedNonVirtualDtor[] =
+    "[chromium-style] Protected non-virtual destructor declared here";
 
 bool TypeHasNonTrivialDtor(const Type* type) {
   if (const CXXRecordDecl* cxx_r = type->getPointeeCXXRecordDecl())
@@ -74,6 +79,8 @@ class FindBadConstructsConsumer : public ChromeClassTester {
         getErrorLevel(), kNoExplicitDtor);
     diag_public_dtor_ = diagnostic().getCustomDiagID(
         getErrorLevel(), kPublicDtor);
+    diag_protected_non_virtual_dtor_ = diagnostic().getCustomDiagID(
+        getErrorLevel(), kProtectedNonVirtualDtor);
 
     // Registers notes to make it easier to interpret warnings.
     diag_note_inheritance_ = diagnostic().getCustomDiagID(
@@ -82,6 +89,8 @@ class FindBadConstructsConsumer : public ChromeClassTester {
         DiagnosticsEngine::Note, kNoteImplicitDtor);
     diag_note_public_dtor_ = diagnostic().getCustomDiagID(
         DiagnosticsEngine::Note, kNotePublicDtor);
+    diag_note_protected_non_virtual_dtor_ = diagnostic().getCustomDiagID(
+        DiagnosticsEngine::Note, kNoteProtectedNonVirtualDtor);
   }
 
   virtual void CheckChromeClass(SourceLocation record_location,
@@ -118,9 +127,11 @@ class FindBadConstructsConsumer : public ChromeClassTester {
 
   unsigned diag_no_explicit_dtor_;
   unsigned diag_public_dtor_;
+  unsigned diag_protected_non_virtual_dtor_;
   unsigned diag_note_inheritance_;
   unsigned diag_note_implicit_dtor_;
   unsigned diag_note_public_dtor_;
+  unsigned diag_note_protected_non_virtual_dtor_;
 
   // Prints errors if the constructor/destructor weight is too heavy.
   void CheckCtorDtorWeight(SourceLocation record_location,
@@ -459,6 +470,18 @@ class FindBadConstructsConsumer : public ChromeClassTester {
     }
   }
 
+  unsigned DiagnosticForIssue(RefcountIssue issue) {
+    switch (issue) {
+      case ImplicitDestructor:
+        return diag_no_explicit_dtor_;
+      case PublicDestructor:
+        return diag_public_dtor_;
+      case None:
+        assert(false && "Do not call DiagnosticForIssue with issue None");
+        return 0;
+    }
+  }
+
   // Check |record| to determine if it has any problematic refcounting
   // issues and, if so, print them as warnings/errors based on the current
   // value of getErrorLevel().
@@ -485,15 +508,19 @@ class FindBadConstructsConsumer : public ChromeClassTester {
     // Easy check: Check to see if the current type is problematic.
     SourceLocation loc;
     RefcountIssue issue = CheckRecordForRefcountIssue(record, loc);
-    if (issue == ImplicitDestructor) {
-      diagnostic().Report(loc, diag_no_explicit_dtor_);
+    if (issue != None) {
+      diagnostic().Report(loc, DiagnosticForIssue(issue));
       PrintInheritanceChain(refcounted_path.front());
       return;
     }
-    if (issue == PublicDestructor) {
-      diagnostic().Report(loc, diag_public_dtor_);
-      PrintInheritanceChain(refcounted_path.front());
-      return;
+    if (CXXDestructorDecl* dtor =
+        refcounted_path.begin()->back().Class->getDestructor()) {
+      if (dtor->getAccess() == AS_protected &&
+          !dtor->isVirtual()) {
+        loc = dtor->getInnerLocStart();
+        diagnostic().Report(loc, diag_protected_non_virtual_dtor_);
+        return;
+      }
     }
 
     // Long check: Check all possible base classes for problematic
diff --git a/tools/clang/plugins/tests/base_refcounted.cpp b/tools/clang/plugins/tests/base_refcounted.cpp
index 364a3e8..698bf7b 100644
--- a/tools/clang/plugins/tests/base_refcounted.cpp
+++ b/tools/clang/plugins/tests/base_refcounted.cpp
@@ -10,10 +10,17 @@ namespace {
 
 // Unsafe; should error.
 class AnonymousDerivedProtectedToPublicInImpl
-    : public ProtectedRefCountedDtorInHeader {
+    : public ProtectedRefCountedVirtualDtorInHeader {
  public:
   AnonymousDerivedProtectedToPublicInImpl() {}
-  ~AnonymousDerivedProtectedToPublicInImpl() {}
+  virtual ~AnonymousDerivedProtectedToPublicInImpl() {}
+};
+
+// Unsafe; but we should only warn on the base class.
+class AnonymousDerivedProtectedOnDerived
+    : public ProtectedRefCountedDtorInHeader {
+ protected:
+  ~AnonymousDerivedProtectedOnDerived() {}
 };
 
 }  // namespace
@@ -56,7 +63,7 @@ int main() {
   PublicRefCountedDtorInHeader bad;
   PublicRefCountedDtorInImpl also_bad;
 
-  ProtectedRefCountedDtorInHeader* protected_ok = NULL;
+  ProtectedRefCountedDtorInHeader* even_badder = NULL;
   PrivateRefCountedDtorInHeader* private_ok = NULL;
 
   DerivedProtectedToPublicInHeader still_bad;
diff --git a/tools/clang/plugins/tests/base_refcounted.h b/tools/clang/plugins/tests/base_refcounted.h
index 840009d..4b4077c 100644
--- a/tools/clang/plugins/tests/base_refcounted.h
+++ b/tools/clang/plugins/tests/base_refcounted.h
@@ -63,7 +63,7 @@ class PublicRefCountedThreadSafeDtorInHeader
       PublicRefCountedThreadSafeDtorInHeader>;
 };
 
-// Safe; should not have errors.
+// Unsafe; should error.
 class ProtectedRefCountedDtorInHeader
     : public base::RefCounted<ProtectedRefCountedDtorInHeader> {
  public:
@@ -76,6 +76,20 @@ class ProtectedRefCountedDtorInHeader
   friend class base::RefCounted<ProtectedRefCountedDtorInHeader>;
 };
 
+// Safe; should not have errors
+class ProtectedRefCountedVirtualDtorInHeader
+    : public base::RefCounted<ProtectedRefCountedVirtualDtorInHeader> {
+ public:
+  ProtectedRefCountedVirtualDtorInHeader() {}
+
+ protected:
+  virtual ~ProtectedRefCountedVirtualDtorInHeader() {}
+
+ private:
+  friend class base::RefCounted<ProtectedRefCountedVirtualDtorInHeader>;
+};
+
+
 // Safe; should not have errors.
 class PrivateRefCountedDtorInHeader
     : public base::RefCounted<PrivateRefCountedDtorInHeader> {
@@ -90,16 +104,16 @@ class PrivateRefCountedDtorInHeader
 // Unsafe; A grandchild class ends up exposing their parent and grandparent's
 // destructors.
 class DerivedProtectedToPublicInHeader
-    : public ProtectedRefCountedDtorInHeader {
+    : public ProtectedRefCountedVirtualDtorInHeader {
  public:
   DerivedProtectedToPublicInHeader() {}
-  ~DerivedProtectedToPublicInHeader() {}
+  virtual ~DerivedProtectedToPublicInHeader() {}
 };
 
 // Unsafe; A grandchild ends up implicitly exposing their parent and
 // grantparent's destructors.
 class ImplicitDerivedProtectedToPublicInHeader
-    : public ProtectedRefCountedDtorInHeader {
+    : public ProtectedRefCountedVirtualDtorInHeader {
  public:
   ImplicitDerivedProtectedToPublicInHeader() {}
 };
diff --git a/tools/clang/plugins/tests/base_refcounted.txt b/tools/clang/plugins/tests/base_refcounted.txt
index 9156ffb..20c0cdf 100644
--- a/tools/clang/plugins/tests/base_refcounted.txt
+++ b/tools/clang/plugins/tests/base_refcounted.txt
@@ -1,110 +1,87 @@
 In file included from base_refcounted.cpp:5:
-./base_refcounted.h:47:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
+./base_refcounted.h:47:3: warning: [chromium-style] Classes that are ref-counted should have destructors that are declared protected or private.
   ~PublicRefCountedDtorInHeader() {}
   ^
 ./base_refcounted.h:44:7: note: [chromium-style] 'PublicRefCountedDtorInHeader' inherits from 'base::RefCounted<PublicRefCountedDtorInHeader>' here
     : public base::RefCounted<PublicRefCountedDtorInHeader> {
       ^
-./base_refcounted.h:59:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
+./base_refcounted.h:59:3: warning: [chromium-style] Classes that are ref-counted should have destructors that are declared protected or private.
   ~PublicRefCountedThreadSafeDtorInHeader() {}
   ^
 ./base_refcounted.h:55:7: note: [chromium-style] 'PublicRefCountedThreadSafeDtorInHeader' inherits from 'base::RefCountedThreadSafe<PublicRefCountedThreadSafeDtorInHeader>' here
     : public base::RefCountedThreadSafe<
       ^
-./base_refcounted.h:96:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
-  ~DerivedProtectedToPublicInHeader() {}
+./base_refcounted.h:73:3: warning: [chromium-style] Classes that are ref-counted and have non-private destructors should declare their destructor virtual.
+  ~ProtectedRefCountedDtorInHeader() {}
   ^
-./base_refcounted.h:93:7: note: [chromium-style] 'DerivedProtectedToPublicInHeader' inherits from 'ProtectedRefCountedDtorInHeader' here
-    : public ProtectedRefCountedDtorInHeader {
+./base_refcounted.h:110:3: warning: [chromium-style] Classes that are ref-counted should have destructors that are declared protected or private.
+  virtual ~DerivedProtectedToPublicInHeader() {}
+  ^
+./base_refcounted.h:107:7: note: [chromium-style] 'DerivedProtectedToPublicInHeader' inherits from 'ProtectedRefCountedVirtualDtorInHeader' here
+    : public ProtectedRefCountedVirtualDtorInHeader {
       ^
-./base_refcounted.h:68:7: note: [chromium-style] 'ProtectedRefCountedDtorInHeader' inherits from 'base::RefCounted<ProtectedRefCountedDtorInHeader>' here
-    : public base::RefCounted<ProtectedRefCountedDtorInHeader> {
+./base_refcounted.h:81:7: note: [chromium-style] 'ProtectedRefCountedVirtualDtorInHeader' inherits from 'base::RefCounted<ProtectedRefCountedVirtualDtorInHeader>' here
+    : public base::RefCounted<ProtectedRefCountedVirtualDtorInHeader> {
       ^
-./base_refcounted.h:101:7: warning: [chromium-style] Classes that are ref-counted should have explicit destructors that are declared protected or private.
+./base_refcounted.h:115:7: warning: [chromium-style] Classes that are ref-counted should have explicit destructors that are declared protected or private.
 class ImplicitDerivedProtectedToPublicInHeader
       ^
-./base_refcounted.h:102:7: note: [chromium-style] 'ImplicitDerivedProtectedToPublicInHeader' inherits from 'ProtectedRefCountedDtorInHeader' here
-    : public ProtectedRefCountedDtorInHeader {
+./base_refcounted.h:116:7: note: [chromium-style] 'ImplicitDerivedProtectedToPublicInHeader' inherits from 'ProtectedRefCountedVirtualDtorInHeader' here
+    : public ProtectedRefCountedVirtualDtorInHeader {
       ^
-./base_refcounted.h:68:7: note: [chromium-style] 'ProtectedRefCountedDtorInHeader' inherits from 'base::RefCounted<ProtectedRefCountedDtorInHeader>' here
-    : public base::RefCounted<ProtectedRefCountedDtorInHeader> {
+./base_refcounted.h:81:7: note: [chromium-style] 'ProtectedRefCountedVirtualDtorInHeader' inherits from 'base::RefCounted<ProtectedRefCountedVirtualDtorInHeader>' here
+    : public base::RefCounted<ProtectedRefCountedVirtualDtorInHeader> {
       ^
-./base_refcounted.h:131:1: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
+./base_refcounted.h:145:1: warning: [chromium-style] Classes that are ref-counted should have destructors that are declared protected or private.
 class ImplementsAPublicInterface
 ^
-./base_refcounted.h:133:7: note: [chromium-style] 'ImplementsAPublicInterface' inherits from 'base::RefCounted<ImplementsAPublicInterface>' here
+./base_refcounted.h:147:7: note: [chromium-style] 'ImplementsAPublicInterface' inherits from 'base::RefCounted<ImplementsAPublicInterface>' here
       public base::RefCounted<ImplementsAPublicInterface> {
       ^
-./base_refcounted.h:125:3: note: [chromium-style] Public destructor declared here
+./base_refcounted.h:139:3: note: [chromium-style] Public destructor declared here
   virtual ~APublicInterface() {}
   ^
-./base_refcounted.h:132:7: note: [chromium-style] 'ImplementsAPublicInterface' inherits from 'APublicInterface' here
+./base_refcounted.h:146:7: note: [chromium-style] 'ImplementsAPublicInterface' inherits from 'APublicInterface' here
     : public APublicInterface,
       ^
-./base_refcounted.h:150:1: warning: [chromium-style] Classes that are ref-counted should have explicit destructors that are declared protected or private.
+./base_refcounted.h:164:1: warning: [chromium-style] Classes that are ref-counted should have explicit destructors that are declared protected or private.
 class ImplementsAnImplicitInterface
 ^
-./base_refcounted.h:152:7: note: [chromium-style] 'ImplementsAnImplicitInterface' inherits from 'base::RefCounted<ImplementsAnImplicitInterface>' here
+./base_refcounted.h:166:7: note: [chromium-style] 'ImplementsAnImplicitInterface' inherits from 'base::RefCounted<ImplementsAnImplicitInterface>' here
       public base::RefCounted<ImplementsAnImplicitInterface> {
       ^
-./base_refcounted.h:144:7: note: [chromium-style] No explicit destructor for 'AnImplicitInterface' defined
+./base_refcounted.h:158:7: note: [chromium-style] No explicit destructor for 'AnImplicitInterface' defined
 class AnImplicitInterface {
       ^
-./base_refcounted.h:151:7: note: [chromium-style] 'ImplementsAnImplicitInterface' inherits from 'AnImplicitInterface' here
+./base_refcounted.h:165:7: note: [chromium-style] 'ImplementsAnImplicitInterface' inherits from 'AnImplicitInterface' here
     : public AnImplicitInterface,
       ^
-./base_refcounted.h:194:1: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
-class UnsafeInheritanceChain
-^
-./base_refcounted.h:197:7: note: [chromium-style] 'UnsafeInheritanceChain' inherits from 'RefcountedType' here
-      public RefcountedType {
-      ^
-./base_refcounted.h:188:24: note: [chromium-style] 'RefcountedType' inherits from 'base::RefCounted<RefcountedType>' here
-class RefcountedType : public base::RefCounted<RefcountedType> {
-                       ^
-./base_refcounted.h:176:3: note: [chromium-style] Public destructor declared here
-  virtual ~BaseInterface() {}
+./base_refcounted.h:204:3: warning: [chromium-style] Classes that are ref-counted and have non-private destructors should declare their destructor virtual.
+  ~RefcountedType() {}
   ^
-./base_refcounted.h:195:7: note: [chromium-style] 'UnsafeInheritanceChain' inherits from 'DerivedInterface' here
-    : public DerivedInterface,
-      ^
-./base_refcounted.h:179:26: note: [chromium-style] 'DerivedInterface' inherits from 'BaseInterface' here
-class DerivedInterface : public BaseInterface {
-                         ^
-./base_refcounted.h:194:1: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
-class UnsafeInheritanceChain
-^
-./base_refcounted.h:197:7: note: [chromium-style] 'UnsafeInheritanceChain' inherits from 'RefcountedType' here
-      public RefcountedType {
-      ^
-./base_refcounted.h:188:24: note: [chromium-style] 'RefcountedType' inherits from 'base::RefCounted<RefcountedType>' here
-class RefcountedType : public base::RefCounted<RefcountedType> {
-                       ^
-./base_refcounted.h:185:3: note: [chromium-style] Public destructor declared here
-  virtual ~SomeOtherInterface() {}
-  ^
-./base_refcounted.h:196:7: note: [chromium-style] 'UnsafeInheritanceChain' inherits from 'SomeOtherInterface' here
-      public SomeOtherInterface,
-      ^
-base_refcounted.cpp:16:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
-  ~AnonymousDerivedProtectedToPublicInImpl() {}
+./base_refcounted.h:204:3: warning: [chromium-style] Classes that are ref-counted and have non-private destructors should declare their destructor virtual.
+base_refcounted.cpp:16:3: warning: [chromium-style] Classes that are ref-counted should have destructors that are declared protected or private.
+  virtual ~AnonymousDerivedProtectedToPublicInImpl() {}
   ^
-base_refcounted.cpp:13:7: note: [chromium-style] 'AnonymousDerivedProtectedToPublicInImpl' inherits from 'ProtectedRefCountedDtorInHeader' here
-    : public ProtectedRefCountedDtorInHeader {
+base_refcounted.cpp:13:7: note: [chromium-style] 'AnonymousDerivedProtectedToPublicInImpl' inherits from 'ProtectedRefCountedVirtualDtorInHeader' here
+    : public ProtectedRefCountedVirtualDtorInHeader {
       ^
-./base_refcounted.h:68:7: note: [chromium-style] 'ProtectedRefCountedDtorInHeader' inherits from 'base::RefCounted<ProtectedRefCountedDtorInHeader>' here
-    : public base::RefCounted<ProtectedRefCountedDtorInHeader> {
+./base_refcounted.h:81:7: note: [chromium-style] 'ProtectedRefCountedVirtualDtorInHeader' inherits from 'base::RefCounted<ProtectedRefCountedVirtualDtorInHeader>' here
+    : public base::RefCounted<ProtectedRefCountedVirtualDtorInHeader> {
       ^
-base_refcounted.cpp:26:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
+./base_refcounted.h:73:3: warning: [chromium-style] Classes that are ref-counted and have non-private destructors should declare their destructor virtual.
+  ~ProtectedRefCountedDtorInHeader() {}
+  ^
+base_refcounted.cpp:33:3: warning: [chromium-style] Classes that are ref-counted should have destructors that are declared protected or private.
   ~PublicRefCountedDtorInImpl() {}
   ^
-base_refcounted.cpp:23:7: note: [chromium-style] 'PublicRefCountedDtorInImpl' inherits from 'base::RefCounted<PublicRefCountedDtorInImpl>' here
+base_refcounted.cpp:30:7: note: [chromium-style] 'PublicRefCountedDtorInImpl' inherits from 'base::RefCounted<PublicRefCountedDtorInImpl>' here
     : public base::RefCounted<PublicRefCountedDtorInImpl> {
       ^
-base_refcounted.cpp:52:3: warning: [chromium-style] Classes that are ref-counted should not have public destructors.
+base_refcounted.cpp:59:3: warning: [chromium-style] Classes that are ref-counted should have destructors that are declared protected or private.
   ~UnsafeTypedefChainInImpl() {}
   ^
-base_refcounted.cpp:49:34: note: [chromium-style] 'UnsafeTypedefChainInImpl' inherits from 'Baz::MyLocalTypedef' (aka 'RefCounted<Foo::BarInterface>') here
+base_refcounted.cpp:56:34: note: [chromium-style] 'UnsafeTypedefChainInImpl' inherits from 'Baz::MyLocalTypedef' (aka 'RefCounted<Foo::BarInterface>') here
 class UnsafeTypedefChainInImpl : public Baz::MyLocalTypedef {
                                  ^
-11 warnings generated.
+13 warnings generated.