Bug 1156084 - Disallow AddRef() and Release() calls on the return value of methods returning XPCOM objects; r=jrmuizel
authorEhsan Akhgari <ehsan@mozilla.com>
Sun, 19 Apr 2015 13:22:35 -0400
changeset 262165 b494504e80f782ba0cfbb9d0f4255ead5e9791f2
parent 262164 7562387802435952a44204af4b854dc8e9898623
child 262166 84d5d34e1445d39944d6090628e026887c2ddc42
push id4932
push userjlund@mozilla.com
push dateMon, 10 Aug 2015 18:23:06 +0000
treeherdermozilla-esr52@6dd5a4f5f745 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjrmuizel
bugs1156084
milestone41.0a1
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
build/clang-plugin/tests/TestNoAddRefReleaseOnReturn.cpp
dom/ipc/TabParent.cpp
dom/media/gmp/GMPService.cpp
gfx/2d/DrawTargetCG.cpp
netwerk/protocol/ftp/FTPChannelParent.cpp
netwerk/protocol/http/HttpChannelParent.cpp
netwerk/protocol/websocket/WebSocketChannelParent.cpp
netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp
--- a/build/clang-plugin/clang-plugin.cpp
+++ b/build/clang-plugin/clang-plugin.cpp
@@ -685,21 +685,29 @@ DiagnosticsMatcher::DiagnosticsMatcher()
 
   astMatcher.addMatcher(binaryOperator(allOf(binaryEqualityOperator(),
           hasLHS(has(declRefExpr(hasType(qualType((isFloat())))).bind("lhs"))),
           hasRHS(has(declRefExpr(hasType(qualType((isFloat())))).bind("rhs"))),
           unless(anyOf(isInSystemHeader(), isInSkScalarDotH()))
       )).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"))
         ),
     &refCountedInsideLambdaChecker);
 
   // Older clang versions such as the ones used on the infra recognize these
   // conversions as 'operator _Bool', but newer clang versions recognize these
--- a/build/clang-plugin/tests/TestNoAddRefReleaseOnReturn.cpp
+++ b/build/clang-plugin/tests/TestNoAddRefReleaseOnReturn.cpp
@@ -1,65 +1,110 @@
 #define MOZ_NO_ADDREF_RELEASE_ON_RETURN __attribute__((annotate("moz_no_addref_release_on_return")))
 
 struct Test {
   void AddRef();
   void Release();
   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;
   T& g() MOZ_NO_ADDREF_RELEASE_ON_RETURN;
   T  h() MOZ_NO_ADDREF_RELEASE_ON_RETURN;
 };
 
 template<class T>
 struct SP {
   T* operator->() MOZ_NO_ADDREF_RELEASE_ON_RETURN;
 };
 
 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'}}
   s.f()->Release(); // expected-error{{'Release' cannot be called on the return value of 'f'}}
   s.f()->foo();
   s.g().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'g'}}
   s.g().Release(); // expected-error{{'Release' cannot be called on the return value of 'g'}}
   s.g().foo();
   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'}}
   x.f()->foo();
   x.g().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'g'}}
   x.g().Release(); // expected-error{{'Release' cannot be called on the return value of 'g'}}
   x.g().foo();
   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();
   g().AddRef(); // expected-error{{'AddRef' cannot be called on the return value of 'g'}}
   g().Release(); // expected-error{{'Release' cannot be called on the return value of 'g'}}
   g().foo();
   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();
 }
--- a/dom/ipc/TabParent.cpp
+++ b/dom/ipc/TabParent.cpp
@@ -3073,25 +3073,26 @@ public:
   NS_IMETHOD GetLoadGroup(nsILoadGroup**) NO_IMPL
   NS_IMETHOD SetLoadGroup(nsILoadGroup*) NO_IMPL
   NS_IMETHOD SetLoadFlags(nsLoadFlags) NO_IMPL
   NS_IMETHOD GetLoadFlags(nsLoadFlags*) NO_IMPL
   NS_IMETHOD GetOriginalURI(nsIURI**) NO_IMPL
   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
   {
     mLoadInfo = aLoadInfo;
     return NS_OK;
   }
   NS_IMETHOD GetNotificationCallbacks(nsIInterfaceRequestor** aRequestor) override
--- a/dom/media/gmp/GMPService.cpp
+++ b/dom/media/gmp/GMPService.cpp
@@ -270,18 +270,18 @@ GeckoMediaPluginService::GetThread(nsITh
     if (NS_FAILED(rv)) {
       return rv;
     }
 
     // Tell the thread to initialize plugins
     InitializePlugins();
   }
 
-  NS_ADDREF(mGMPThread);
-  *aThread = mGMPThread;
+  nsCOMPtr<nsIThread> copy = mGMPThread;
+  copy.forget(aThread);
 
   return NS_OK;
 }
 
 class GetGMPContentParentForAudioDecoderDone : public GetGMPContentParentCallback
 {
 public:
   explicit GetGMPContentParentForAudioDecoderDone(UniquePtr<GetGMPAudioDecoderCallback>&& aCallback)
--- a/gfx/2d/DrawTargetCG.cpp
+++ b/gfx/2d/DrawTargetCG.cpp
@@ -246,17 +246,17 @@ GetRetainedImageFromSourceSurface(Source
       return CGImageRetain(static_cast<SourceSurfaceCGContext*>(aSurface)->GetImage());
 
     default:
     {
       RefPtr<DataSourceSurface> data = aSurface->GetDataSurface();
       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());
     }
   }
 }
 
 TemporaryRef<SourceSurface>
--- a/netwerk/protocol/ftp/FTPChannelParent.cpp
+++ b/netwerk/protocol/ftp/FTPChannelParent.cpp
@@ -460,18 +460,18 @@ FTPChannelParent::Delete()
 // FTPChannelParent::nsIInterfaceRequestor
 //-----------------------------------------------------------------------------
 
 NS_IMETHODIMP
 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;
   }
 
   return QueryInterface(uuid, result);
 }
 
 //-----------------------------------------------------------------------------
 // FTPChannelParent::ADivertableParentChannel
--- a/netwerk/protocol/http/HttpChannelParent.cpp
+++ b/netwerk/protocol/http/HttpChannelParent.cpp
@@ -218,18 +218,18 @@ HttpChannelParent::GetInterface(const ns
   if (XRE_GetProcessType() == GeckoProcessType_Default &&
       aIID.Equals(NS_GET_IID(nsIAuthPromptProvider))) {
     *result = nullptr;
     return NS_OK;
   }
 
   // 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;
   }
 
   return QueryInterface(aIID, result);
 }
 
 //-----------------------------------------------------------------------------
 // HttpChannelParent::PHttpChannelParent
--- a/netwerk/protocol/websocket/WebSocketChannelParent.cpp
+++ b/netwerk/protocol/websocket/WebSocketChannelParent.cpp
@@ -299,18 +299,18 @@ WebSocketChannelParent::GetInterface(con
 {
   LOG(("WebSocketChannelParent::GetInterface() %p\n", this));
   if (mAuthProvider && iid.Equals(NS_GET_IID(nsIAuthPromptProvider)))
     return mAuthProvider->GetAuthPrompt(nsIAuthPromptProvider::PROMPT_NORMAL,
                                         iid, 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;
   }
 
   return QueryInterface(iid, result);
 }
 
 void
 WebSocketChannelParent::OfflineDisconnect()
--- a/netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp
+++ b/netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp
@@ -336,18 +336,18 @@ WyciwygChannelParent::OnDataAvailable(ns
 // WyciwygChannelParent::nsIInterfaceRequestor
 //-----------------------------------------------------------------------------
 
 NS_IMETHODIMP
 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;
   }
 
   return QueryInterface(uuid, result);
 }
 
 
 }} // mozilla::net