1156084 - The analysis added in bug 1114999 doesn't work for methods returning XPCOM objects

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[(no longer active)]

The AST matcher expects the MemberExpr to be the direct parent of CallExpr, but if the method returns a derived class from a base refcounted class (such as nsISupports), the matcher won't match it.  This for example makes us not catch:

  nsCOMPtr<nsIFoo> foo;
  foo->AddRef(); // oops!

Comment 1

[(no longer active)]

Attachment: Disallow AddRef() and Release() calls on the return value of methods returning XPCOM objects

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>.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

Comment on attachment 8594512 [details] [diff] [review]
Disallow AddRef() and Release() calls on the return value of methods returning XPCOM objects

Review of attachment 8594512 [details] [diff] [review]:
-----------------------------------------------------------------

::: build/clang-plugin/clang-plugin.cpp
@@ +713,5 @@
>        )).bind("node"),
>      &noAddRefReleaseOnReturnChecker);
> +  astMatcher.addMatcher(callExpr(callee(functionDecl(hasNoAddRefReleaseOnReturnAttr()).bind("func")),
> +                                 hasParent(castExpr(hasParent(memberExpr(isAddRefOrRelease(),
> +                                                                         hasParent(callExpr())).bind("member"))))

maybe add a comment about what's different in this expression from the one above.

::: dom/ipc/TabParent.cpp
@@ +3030,5 @@
>    NS_IMETHOD SetOriginalURI(nsIURI*) NO_IMPL
>    NS_IMETHOD GetURI(nsIURI** aUri) override
>    {
> +    auto copy = mUri;
> +    *aUri = copy.forget().take();;

double semi-colon. copy.forget(aUri);

::: dom/media/gmp/GMPService.cpp
@@ +283,5 @@
>      InitializePlugins();
>    }
>  
> +  auto copy = mGMPThread;
> +  *aThread = copy.forget().take();

Do it better.

::: netwerk/protocol/ftp/FTPChannelParent.cpp
@@ +465,5 @@
>  {
>    // Only support nsILoadContext if child channel's callbacks did too
>    if (uuid.Equals(NS_GET_IID(nsILoadContext)) && mLoadContext) {
> +    auto copy = mLoadContext;
> +    *result = static_cast<nsILoadContext*>(copy.forget().take());

Not using auto would make this easier to read.

Comment 3

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b99d282cd87

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/df384f61363a for static bustage:

https://treeherder.mozilla.org/logviewer.html#?job_id=9146285&repo=mozilla-inbound

Comment 5

[(no longer active)]

I forgot about the dependency on bug 1156094...

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b494504e80f7

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b494504e80f7