1170388 - Static analyzer false positives about lambdas using local (non-captured) raw pointers to refcounted objects

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect)

Description

[Botond Ballo [:botond]]

The patches in bug 1158424 trip [1] the static analysis added in bug 1153304 even though the lambda they introduce does not *capture* a raw pointer to a refcounted object - it merely takes one as argument.

I believe this is perfectly safe - it's just like any other function taking a raw pointer to the refcounted object. The point of bug 1153304 was to guard against *capturing* such a pointer, since that gets stored inside the lambda which may in turn outlive its caller.

[1] http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-96128147095d/try-linux64-st-an-debug/try-linux64-st-an-debug-bm79-try1-build46.txt.gz

Comment 1

[Botond Ballo [:botond]]

Attachment: MozReview Request: Bug 1170388 - Removed an outdated workaround for old clang versions in the static analysis plugin. r=mystor

Bug 1170388 - Restrict the static analysis error given about raw pointers to refcounted objects inside a lambda, to the case where the raw pointer is captured. r=ehsan

Comment 2

[(no longer active)]

Comment on attachment 8614972 [details]
MozReview Request: Bug 1170388 - Removed an outdated workaround for old clang versions in the static analysis plugin. r=mystor

https://reviewboard.mozilla.org/r/10131/#review8949

Ship It!

Comment 3

[Pulsebot]

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

Comment 4

[Botond Ballo [:botond]]

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

Looks like automation's using clang 3.3 for the static analysis, but my patch uses the matcher 'equalsBoundNode' which was only added in clang 3.4.

Comment 5

[(no longer active)]

(In reply to Botond Ballo [:botond] from comment #4)
> Backed out in
> https://hg.mozilla.org/integration/mozilla-inbound/rev/b0b9843b8654 for
> bustage.
> 
> Looks like automation's using clang 3.3 for the static analysis, but my
> patch uses the matcher 'equalsBoundNode' which was only added in clang 3.4.

Ugh.

You can probably copy its implementation...  IIRC I had to do the same for example for IsInSystemHeader.

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 8614972 [details]
MozReview Request: Bug 1170388 - Removed an outdated workaround for old clang versions in the static analysis plugin. r=mystor

Bug 1170388 - Restrict the static analysis error given about raw pointers to refcounted objects inside a lambda, to the case where the raw pointer is captured. r=ehsan

Comment 7

[Botond Ballo [:botond]]

Updated patch to polyfill equalsBoundNode on clang < 3.4. It wasn't a straight copy of the 3.4 implementation because some of the infrastructure changed, but it seems to be working.

Try push showing that it's working:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfd42baee21f

Posting for re-review. Note that the only change from the last patch is the added hunk that provides equalsBoundNode.

Comment 8

[(no longer active)]

Comment on attachment 8614972 [details]
MozReview Request: Bug 1170388 - Removed an outdated workaround for old clang versions in the static analysis plugin. r=mystor

https://reviewboard.mozilla.org/r/10131/#review9189

Ship It!

::: build/clang-plugin/clang-plugin.cpp:611
(Diff revision 2)
> +    const NodeType& Node;

Nit: we try to use the LLVM coding style for this file, so * and & should be attached to the variable names here and elsewhere.

::: build/clang-plugin/clang-plugin.cpp:622
(Diff revision 2)
> +  Visitor* visitor = new Visitor(Node, ID, haveMatchingResult);

Hmm, why are you allocating this on the heap?  It seems like it could live on the stack.

Comment 9

[Botond Ballo [:botond]]

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #8)
> Comment on attachment 8614972 [details]
> MozReview Request: Bug 1170388 - Restrict the static analysis error given
> about raw pointers to refcounted objects inside a lambda, to the case where
> the raw pointer is captured. r=ehsan
> 
> https://reviewboard.mozilla.org/r/10131/#review9189
> 
> Ship It!
> 
> ::: build/clang-plugin/clang-plugin.cpp:611
> (Diff revision 2)
> > +    const NodeType& Node;
> 
> Nit: we try to use the LLVM coding style for this file, so * and & should be
> attached to the variable names here and elsewhere.

Fixed.

> ::: build/clang-plugin/clang-plugin.cpp:622
> (Diff revision 2)
> > +  Visitor* visitor = new Visitor(Node, ID, haveMatchingResult);
> 
> Hmm, why are you allocating this on the heap?  It seems like it could live
> on the stack.

My reason was that visitMatches() takes the visitor by pointer, but that's a bad reason :) Fixed.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/03ff333d49c0

Comment 11

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

https://hg.mozilla.org/mozilla-central/rev/03ff333d49c0

Comment 12

[Botond Ballo [:botond]]

The fact that the static analysis is still triggering [1] on my patches in bug 1158424 indicates that the fix for clang 3.3 isn't correct. Reopening.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ff606ee785a

Comment 13

[Botond Ballo [:botond]]

I spent some time trying to debug this the issue on the cland 3.3 codepath, without luck. 

Ehsan and I discussed this briefly and agreed that a better way forward is to upgrade automation to use clang >= 3.4.

Comment 14

[Botond Ballo [:botond]]

Going to unassign for now as I have no immediately plans to continue working on this. If we end up upgrading automation's clang to >= 3.4, that will solve the issue without further work, otherwise I can continue trying to find a 3.3 workaround after Whistler.

Comment 15

[Botond Ballo [:botond]]

Bug 1123386 has now landed, which upgrades the clang version used for the static analysis builds in automation to >= 3.4. I believe this upgrade should resolve this issue.

Once we verify that this is the case (by someone landing a patch that uses lambdas that use non-captured raw pointers to refcounted objects and that patch passing static analysis), I'll land a patch in this bug to remove my attempt at a pre-3.4 solution (which never worked) that landed in comment 11.

Comment 16

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #15)
> Bug 1123386 has now landed, which upgrades the clang version used for the
> static analysis builds in automation to >= 3.4. I believe this upgrade
> should resolve this issue.
> 
> Once we verify that this is the case (by someone landing a patch that uses
> lambdas that use non-captured raw pointers to refcounted objects and that
> patch passing static analysis), I'll land a patch in this bug to remove my
> attempt at a pre-3.4 solution (which never worked) that landed in comment 11.

Bug 1200413 has made this point moot by rewriting the static analysis to not use the 'equalsBoundNode' matcher that I was trying to polyfill. I'll post a patch to remove the (broken) polyfill attempt.

Comment 17

[Botond Ballo [:botond]]

Comment on attachment 8614972 [details]
MozReview Request: Bug 1170388 - Removed an outdated workaround for old clang versions in the static analysis plugin. r=mystor

Bug 1170388 - Removed an outdated workaround for old clang versions in the static analysis plugin. r=mystor

Comment 18

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8614972 [details]
MozReview Request: Bug 1170388 - Removed an outdated workaround for old clang versions in the static analysis plugin. r=mystor

https://reviewboard.mozilla.org/r/10131/#review16409

LGTM

Comment 19

[Pulsebot]

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

Comment 20

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

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