1281935 - Change our static analyses to allow lambdas with raw pointers to refcounted types in more situations

Status: RESOLVED FIXED

(Developer Infrastructure :: Source Code Analysis, defect, P3)

Description

[Seth Fowler [:seth] [:s2h]]

We should only forbid lambdas with raw pointers to refcounted types in the follow two situations:

(1) When the lambda is returned from a function.

(2) When the lambda's type is used as a template parameter to instantiate a template, and as a result that template has a member variable of the lambda type. (This covers things like mozilla::Function, std::Function, nsRunnableFunction, etc.)

Outside of these two situations, the lambda lives on the stack only, and there's no danger in using raw pointers to refcounted types. There is a significant additional burden in callsite complexity when the current static analysis gets a false positive, so this new approach will make it much easier and more readable to use lambdas in many common situations.

Here's a fuller discussion, copy pasted from IRC:

<seth>	mystor: so the current static analyses that check for use of refcounted types with lambdas are excessively restrictive, because using the raw pointers is only a problem if you store the lambda
<seth>	mystor: right now the static analyses freak out no matter what, which can bloat callsites that just use the lambda on the stack and immediately return, for no good reason
<seth>	mystor: can we inspect the member variables of the desugared lambda struct when it's stored into a mozilla::Function or std::Function?
<seth>	mystor: the only real dangerous situation is when the lambda is stored. this would pretty much eliminate false positives
<seth>	mystor: i suppose we also need to check for the lambda type being used for a member variable. that's the pattern that nsRunnableFunction uses
<seth>	mystor: actually, that's all we need, of course. <smacks forehead> after all mozilla::Function works the same way. (see https://dxr.mozilla.org/mozilla-central/source/mfbt/Function.h#66)
<seth>	mystor: should just be able to examine template instantiations, see if a template type is instantiated with a lambda type parameter and if the lambda type is used as for a member variable, and if so see if the lambda type has any raw pointers to refcounted types
<mystor>	seth: That could potentially be done
<mystor>	seth: (sorry was afk)
<seth>	mystor: i'd be very happy if this replaced the existing static analysis
<seth>	mystor: i can file a bug; what component does this belong in?
<mystor>	Rewriting and Analysis
<seth>	mystor: thanks!
<mystor>	seth: So what you want to do is complain when a struct has a member which is a lambda which closes over a raw pointer to a reference counted type?
<seth>	mystor: exactly
<seth>	mystor: that's the only unsafe situation
<seth>	it's fine to close over raw pointers to refcounted types if the lambda stays on the stack
<mystor>	seth: Can you return a lambda from an auto function?
mystor	isn't sure
<seth>	mystor: hmmm. so you definitely can, actually
<mystor>	and yeah, it makes sense to allow lambdas in some more cases I would think. ehsan wrote the original analysis, and it was done when we didn't have very many lambdas in tree
<mystor>	seth: I can also complain if you have a lambda in the return value of a function
<seth>	mystor: yeah, i think that should be sufficient

Comment 1

[Botond Ballo [:botond]]

Nice! If implemented, I think this would supersede bug 1201275.

Comment 2

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

Attachment: Part 1: Relax raw pointer inside lambda analysis

Comment 3

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

Attachment: Part 2: Remove the FakeRef hack added in Bug 1283620

Comment 4

[Bobby Holley (:bholley)]

Comment on attachment 8772183 [details] [diff] [review]
Part 2: Remove the FakeRef hack added in Bug 1283620

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

Thanks.

Comment 5

[(no longer active)]

Comment on attachment 8772182 [details] [diff] [review]
Part 1: Relax raw pointer inside lambda analysis

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

Nice, thanks!

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Seth Fowler [:seth] [:s2h] from comment #0)
> We should only forbid lambdas with raw pointers to refcounted types in the
> follow two situations:
> 
> (1) When the lambda is returned from a function.
> 
> (2) When the lambda's type is used as a template parameter to instantiate a
> template, and as a result that template has a member variable of the lambda
> type. (This covers things like mozilla::Function, std::Function,
> nsRunnableFunction, etc.)

...or any member objects of the template have a member variable of the lambda type, and so on recursively.  Admittedly, this would be kind of an unusual case, but I don't think it's completely weird.

I think Michael's patch covers this case?  I'm not sure.

Comment 7

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

(In reply to Nathan Froyd [:froydnj] from comment #6)
> (In reply to Seth Fowler [:seth] [:s2h] from comment #0)
> > We should only forbid lambdas with raw pointers to refcounted types in the
> > follow two situations:
> > 
> > (1) When the lambda is returned from a function.
> > 
> > (2) When the lambda's type is used as a template parameter to instantiate a
> > template, and as a result that template has a member variable of the lambda
> > type. (This covers things like mozilla::Function, std::Function,
> > nsRunnableFunction, etc.)
> 
> ...or any member objects of the template have a member variable of the
> lambda type, and so on recursively.  Admittedly, this would be kind of an
> unusual case, but I don't think it's completely weird.
> 
> I think Michael's patch covers this case?  I'm not sure.

I don't think it would be possible to get the type of a lambda to put it in a struct without either 

A: returning the lambda from a function and using decltype() to get the type, or
B: Passing it as a template parameter.

Because of that I doubt that you can get past the analysis in its current state.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bfd95fdd1996
Part 1: Relax raw pointer inside lambda analysis, r=ehsan
https://hg.mozilla.org/integration/mozilla-inbound/rev/d392476190ec
Part 2: Remove the FakeRef hack added in Bug 1283620, r=bholley

Comment 9

[Seth Fowler [:seth] [:s2h]]

Thanks for fixing this Michael! I'm excited to clean up some code now that this change has been made.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bfd95fdd1996
https://hg.mozilla.org/mozilla-central/rev/d392476190ec