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

RESOLVED FIXED in Firefox 41

Status

Firefox Build System
Source Code Analysis
RESOLVED FIXED
3 years ago
2 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

unspecified
mozilla41
x86
Mac OS X
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
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!
(Assignee)

Comment 1

3 years ago
Created attachment 8594512 [details] [diff] [review]
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>.
Attachment #8594512 - Flags: review?(jmuizelaar)
(Assignee)

Updated

3 years ago
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+
(Assignee)

Comment 5

3 years ago
I forgot about the dependency on bug 1156094...
Flags: needinfo?(ehsan)
(Assignee)

Updated

3 years ago
Depends on: 1157057
(Assignee)

Updated

3 years ago
Depends on: 1157059
https://hg.mozilla.org/mozilla-central/rev/b494504e80f7
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla41

Updated

2 months ago
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.