On 4/14/15 10:06 AM, Ehsan Akhgari wrote:
On 2015-04-13 1:36 PM, Jan-Ivar Bruaroey wrote:
The above is a raw pointer bug, not a lambda bug.
Yes, I'm aware. Please see bug 1114683.
Thanks, I was not aware of this larger effort. This somewhat addresses
my concern that we seem overly focused on lambdas and calling them
unsafe when they're just another heap instantiation pattern.
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.
There's nothing safe about copying raw pointers to heap objects, period.
Not buying that refcounted objects or lambdas are worse.
So the assertion that lambda capture is safe by default is clearly
false.
I said "safer", as in lambdas' defaults are safer for many copyable
types like nsRefPtr than manual instantiation is. They are not less safe
than other things because of raw pointers.
> 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.
This is a terrible pattern that we should not grandfather any code in
under, so why is https://bugzil.la/833797 WONTFIX?
NS_DispatchToMainThread encourages use of raw pointers, so lets fix that
for everyone. Until then, this seems to be the pattern.
You can't argue [I think you mean 'for'] doing more terrible things
> because we do bad things elsewhere in the code.
I write non-exception c++ code every day for that very reason.
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.
Thanks for making my point! The ban would not catch the general problem:
MyRunnableBackToMain* runnable = new MyRunnableBackToMain();
NS_DispatchToMainThread(runnable);
// future Gecko hacker comes around and does:
runnable->FooBar(); // oops, is runnable still valid? maybe not!
I hope this shows that lambdas are a red herring here.
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.
It's not a new class, and the new kids on the bus are not why the bus is
broken.
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.
Can't hold nsRefPtr to main-destined runnables off main-thread.
I've filed https://bugzil.la/1154337 to discuss a solution to that.
Cheers,
Ehsan
.: Jan-Ivar :.
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
