On 2015-04-13 1:36 PM, Jan-Ivar Bruaroey wrote:
On 4/10/15 2:09 PM, Seth Fowler wrote:
On Apr 10, 2015, at 8:46 AM, Ehsan Akhgari <ehsan.akhg...@gmail.com>
wrote:
I would like to propose that we should ban the usage of refcounted
objects
inside lambdas in Gecko. Here is the reason:
Consider the following code:
nsINode* myNode;
TakeLambda([&]() {
myNode->Foo();
});
There is nothing that guarantees that the lambda passed to TakeLambda is
executed before myNode is destroyed somehow.
The above is a raw pointer bug, not a lambda bug.
Yes, I'm aware. Please see bug 1114683.
IMHO the use of lambdas helps spot the problem, by
1. Being more precise (less boilerplate junk for bugs to hide in), and
2. lambda capture use safer copy construction by default (hence the
standout [&] above for reviewers).
There is nothing safe about copying raw pointers to refcounted objects.
So the assertion that lambda capture is safe by default is clearly
false. Point #1 is objective, and I don't think has any bearing on what
is safe and what unsafe operations need to be banned.
> Lambdas will be much less useful if they can’t capture refcounted
objects, so I’m strongly against banning that.
+1.
Case in point, we use raw pointers with most runnables, a practice
established in NS_DispatchToMainThread [2]. Look in mxr/dxr for the 100+
uses of NS_DispatchToMainThread(new SomeRunnable()).
That is a terrible pattern that needs to be eliminated and not
replicated in new code. You can't argue against doing more terrible
things because we do bad things elsewhere in the code.
The new ban would prevent us from passing runnables to lambdas, like [3]
MyRunnableBackToMain* runnable = new MyRunnableBackToMain();
nsRefPtr<media::ChildPledge<nsCString>> p = SomeAsyncFunc();
p->Then([runnable](nsCString result) mutable {
runnable->mResult = result;
NS_DispatchToMainThread(runnable);
// future Gecko hacker comes around and does:
runnable->FooBar(); // oops, is runnable still valid? maybe not!
});
So I think this ban is misguided. These are old sins not new to lambdas.
Thanks for the great example of this unsafe code. I modified it to
inject a possible use after free in it.
I think you're missing the intent of my proposal. My goal is to prevent
a new class of sec-critical bugs that will creep into our code base
through this pattern. You seem to think that this is somehow a ban of
lambda usage; please re-read the original post. The only thing that you
need to do in the code above to be able to still use lambdas is to make
runnable an nsRefPtr, and that makes the code safe against the issue
under discussion.
Cheers,
Ehsan
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
