Bug 1156084 - Disallow AddRef() and Release() calls on the return value of methods returning XPCOM objects; r=jrmuizel

When a method returns type D derived from RefCounted type B, there is an
ImplicitCastExpr (or an ExplicitCastExpr, if there is an explicit cast
to the base type in the code) in the AST between the CallExpr and
MemberExpr, which we didn't take into account before.  This caused the
analysis to not work on common patterns such as
nsCOMPtr<nsIXPCOMInterface>.

build/clang-plugin/clang-plugin.cpp

@@ -639,11 +639,19 @@ DiagnosticsMatcher::DiagnosticsMatcher()
       )).bind("node"),
     &nanExprChecker);
 
+  // First, look for direct parents of the MemberExpr.
   astMatcher.addMatcher(callExpr(callee(functionDecl(hasNoAddRefReleaseOnReturnAttr()).bind("func")),
                                  hasParent(memberExpr(isAddRefOrRelease(),
                                                       hasParent(callExpr())).bind("member")
       )).bind("node"),
     &noAddRefReleaseOnReturnChecker);
+  // Then, look for MemberExpr that need to be casted to the right type using
+  // an intermediary CastExpr before we get to the CallExpr.
+  astMatcher.addMatcher(callExpr(callee(functionDecl(hasNoAddRefReleaseOnReturnAttr()).bind("func")),
+                                 hasParent(castExpr(hasParent(memberExpr(isAddRefOrRelease(),
+                                                                         hasParent(callExpr())).bind("member"))))
+      ).bind("node"),
+    &noAddRefReleaseOnReturnChecker);
 
   astMatcher.addMatcher(lambdaExpr(
             hasDescendant(declRefExpr(hasType(pointerType(pointee(isRefCounted())))).bind("node"))

build/clang-plugin/tests/TestNoAddRefReleaseOnReturn.cpp

@@ -6,12 +6,20 @@ struct Test {
   void foo();
 };
 
+struct TestD : Test {};
+
 struct S {
   Test* f() MOZ_NO_ADDREF_RELEASE_ON_RETURN;
   Test& g() MOZ_NO_ADDREF_RELEASE_ON_RETURN;
   Test  h() MOZ_NO_ADDREF_RELEASE_ON_RETURN;
 };
 
+struct SD {
+  TestD* f() MOZ_NO_ADDREF_RELEASE_ON_RETURN;
+  TestD& g() MOZ_NO_ADDREF_RELEASE_ON_RETURN;
+  TestD  h() MOZ_NO_ADDREF_RELEASE_ON_RETURN;
+};
+
 template<class T>
 struct X {
   T* f() MOZ_NO_ADDREF_RELEASE_ON_RETURN;
@@ -28,6 +36,10 @@ Test* f() MOZ_NO_ADDREF_RELEASE_ON_RETURN;
 Test& g() MOZ_NO_ADDREF_RELEASE_ON_RETURN;
 Test  h() MOZ_NO_ADDREF_RELEASE_ON_RETURN;
 
+TestD* fd() MOZ_NO_ADDREF_RELEASE_ON_RETURN;
+TestD& gd() MOZ_NO_ADDREF_RELEASE_ON_RETURN;
+TestD  hd() MOZ_NO_ADDREF_RELEASE_ON_RETURN;
+
 void test() {
   S s;
   s.f()->AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'f'}}
@@ -39,6 +51,16 @@ void test() {
   s.h().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'h'}}
   s.h().Release(); // expected-error{{'Release' cannot be called on the return value of 'h'}}
   s.h().foo();
+  SD sd;
+  sd.f()->AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'f'}}
+  sd.f()->Release(); // expected-error{{'Release' cannot be called on the return value of 'f'}}
+  sd.f()->foo();
+  sd.g().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'g'}}
+  sd.g().Release(); // expected-error{{'Release' cannot be called on the return value of 'g'}}
+  sd.g().foo();
+  sd.h().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'h'}}
+  sd.h().Release(); // expected-error{{'Release' cannot be called on the return value of 'h'}}
+  sd.h().foo();
   X<Test> x;
   x.f()->AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'f'}}
   x.f()->Release(); // expected-error{{'Release' cannot be called on the return value of 'f'}}
@@ -49,10 +71,24 @@ void test() {
   x.h().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'h'}}
   x.h().Release(); // expected-error{{'Release' cannot be called on the return value of 'h'}}
   x.h().foo();
+  X<TestD> xd;
+  xd.f()->AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'f'}}
+  xd.f()->Release(); // expected-error{{'Release' cannot be called on the return value of 'f'}}
+  xd.f()->foo();
+  xd.g().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'g'}}
+  xd.g().Release(); // expected-error{{'Release' cannot be called on the return value of 'g'}}
+  xd.g().foo();
+  xd.h().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'h'}}
+  xd.h().Release(); // expected-error{{'Release' cannot be called on the return value of 'h'}}
+  xd.h().foo();
   SP<Test> sp;
   sp->AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'operator->'}}
   sp->Release(); // expected-error{{'Release' cannot be called on the return value of 'operator->'}}
   sp->foo();
+  SP<TestD> spd;
+  spd->AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'operator->'}}
+  spd->Release(); // expected-error{{'Release' cannot be called on the return value of 'operator->'}}
+  spd->foo();
   f()->AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'f'}}
   f()->Release(); // expected-error{{'Release' cannot be called on the return value of 'f'}}
   f()->foo();
@@ -62,4 +98,13 @@ void test() {
   h().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'h'}}
   h().Release(); // expected-error{{'Release' cannot be called on the return value of 'h'}}
   h().foo();
+  fd()->AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'fd'}}
+  fd()->Release(); // expected-error{{'Release' cannot be called on the return value of 'fd'}}
+  fd()->foo();
+  gd().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'gd'}}
+  gd().Release(); // expected-error{{'Release' cannot be called on the return value of 'gd'}}
+  gd().foo();
+  hd().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'hd'}}
+  hd().Release(); // expected-error{{'Release' cannot be called on the return value of 'hd'}}
+  hd().foo();
 }

dom/ipc/TabParent.cpp

@@ -3062,15 +3062,16 @@ public:
   NS_IMETHOD SetOriginalURI(nsIURI*) NO_IMPL
   NS_IMETHOD GetURI(nsIURI** aUri) override
   {
-    NS_IF_ADDREF(mUri);
-    *aUri = mUri;
+    nsCOMPtr<nsIURI> copy = mUri;
+    copy.forget(aUri);
     return NS_OK;
   }
   NS_IMETHOD GetOwner(nsISupports**) NO_IMPL
   NS_IMETHOD SetOwner(nsISupports*) NO_IMPL
   NS_IMETHOD GetLoadInfo(nsILoadInfo** aLoadInfo) override
   {
-    NS_IF_ADDREF(*aLoadInfo = mLoadInfo);
+    nsCOMPtr<nsILoadInfo> copy = mLoadInfo;
+    copy.forget(aLoadInfo);
     return NS_OK;
   }
   NS_IMETHOD SetLoadInfo(nsILoadInfo* aLoadInfo) override

dom/media/gmp/GMPService.cpp

@@ -283,8 +283,8 @@ GeckoMediaPluginService::GetThread(nsIThread** aThread)
     InitializePlugins();
   }
 
-  NS_ADDREF(mGMPThread);
-  *aThread = mGMPThread;
+  nsCOMPtr<nsIThread> copy = mGMPThread;
+  copy.forget(aThread);
 
   return NS_OK;
 }

gfx/2d/DrawTargetCG.cpp

@@ -251,7 +251,7 @@ GetRetainedImageFromSourceSurface(SourceSurface *aSurface)
       if (!data) {
         MOZ_CRASH("unsupported source surface");
       }
-      data->AddRef();
+      data.get()->AddRef();
       return CreateCGImage(releaseDataSurface, data.get(),
                            data->GetData(), data->GetSize(),
                            data->Stride(), data->GetFormat());

netwerk/protocol/ftp/FTPChannelParent.cpp

@@ -465,8 +465,8 @@ FTPChannelParent::GetInterface(const nsIID& uuid, void** result)
 {
   // Only support nsILoadContext if child channel's callbacks did too
   if (uuid.Equals(NS_GET_IID(nsILoadContext)) && mLoadContext) {
-    NS_ADDREF(mLoadContext);
-    *result = static_cast<nsILoadContext*>(mLoadContext);
+    nsCOMPtr<nsILoadContext> copy = mLoadContext;
+    copy.forget(result);
     return NS_OK;
   }
 

netwerk/protocol/http/HttpChannelParent.cpp

@@ -223,8 +223,8 @@ HttpChannelParent::GetInterface(const nsIID& aIID, void **result)
 
   // Only support nsILoadContext if child channel's callbacks did too
   if (aIID.Equals(NS_GET_IID(nsILoadContext)) && mLoadContext) {
-    NS_ADDREF(mLoadContext);
-    *result = static_cast<nsILoadContext*>(mLoadContext);
+    nsCOMPtr<nsILoadContext> copy = mLoadContext;
+    copy.forget(result);
     return NS_OK;
   }
 

netwerk/protocol/websocket/WebSocketChannelParent.cpp

@@ -306,8 +306,8 @@ WebSocketChannelParent::GetInterface(const nsIID & iid, void **result)
 
   // Only support nsILoadContext if child channel's callbacks did too
   if (iid.Equals(NS_GET_IID(nsILoadContext)) && mLoadContext) {
-    NS_ADDREF(mLoadContext);
-    *result = static_cast<nsILoadContext*>(mLoadContext);
+    nsCOMPtr<nsILoadContext> copy = mLoadContext;
+    copy.forget(result);
     return NS_OK;
   }
 

netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp

@@ -343,8 +343,8 @@ WyciwygChannelParent::GetInterface(const nsIID& uuid, void** result)
 {
   // Only support nsILoadContext if child channel's callbacks did too
   if (uuid.Equals(NS_GET_IID(nsILoadContext)) && mLoadContext) {
-    NS_ADDREF(mLoadContext);
-    *result = static_cast<nsILoadContext*>(mLoadContext);
+    nsCOMPtr<nsILoadContext> copy = mLoadContext;
+    copy.forget(result);
     return NS_OK;
   }