Closed Bug 1156084 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox Build System :: Source Code Analysis, defect)

x86
macOS
defect
Not set
normal

Tracking

(firefox41 fixed)

RESOLVED FIXED
mozilla41
Tracking Status
firefox41 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file)

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!
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>.
Attachment #8594512 - Flags: review?(jmuizelaar)
Depends on: 1156094
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.
Attachment #8594512 - Flags: review?(jmuizelaar) → review+
I forgot about the dependency on bug 1156094...
Flags: needinfo?(ehsan)
Depends on: 1157057
Depends on: 1157059
https://hg.mozilla.org/mozilla-central/rev/b494504e80f7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.