(was: Re: Proposal to ban the usage of refcounted objects inside C++
lambdas in Gecko)
tl;dr: We should make DispatchToMainThread take already_AddRefed and
move away from raw ptrs for Dispatches in general.
So:
What I want to avoid is this pattern for runnables that hold
thread-restricted pointers (mostly MainThread-restricted, such as JS
callbacks):
FooRunnable::FooRunnable(nsCOMPtr<nsIFoo>& aFoo)
{
mFoo.swap(aFoo); // Never AddRef/Release off MainThread
}
...
{
nsRefPtr<FooRunnable> foo = new FooRunnable(myFoo);
// RefCnt == 1, myFoo.get() == nullptr now
NS_DispatchToMainThread(foo); // infallible (soon), takes a ref to foo
(refcnt 2)
// XXX what is foo's refcnt here? You don't know!
}
The reason is that foo may go out of scope here *after* it has Run() and
had the event ref dropped on mainthread. I.e.: at the XXX comment, it
may "usually" be 2, but sometimes may be 1, and when foo goes out of
scope we delete it here, and violate thread-safety for mFoo.
Blindly taking an NS_DispatchToMainThread(new FooRunnable) and
converting it to the pattern above (with nsRefPtr) will introduce a
timing-based thread safety violation IF it holds thread-locked items
(which isn't super-common). But also, it's extra thread-safe
addref/releasing when we don't need to.
A safe pattern is this:
{
nsRefPtr<FooRunnable> foo = new FooRunnable(myFoo);
// RefCnt == 1, myFoo.get() == nullptr now
// This requires adding a version of
// NS_DispatchToMainThread(already_AddRefed<nsIRunnable>) or some such
NS_DispatchToMainThread(foo.forget()); // infallible (soon)
// foo is nullptr
}
And to be honest, that's generally a better pattern for runnables
anyways - we *want* to give them away on Dispatch()/DispatchToMainThread().
Note that you can also do ehsan's trick for safety instead:
~FooRunnable()
{
if (!NS_IsMainThread()) {
// get mainthread
NS_ProxyRelease(mainthread, mFoo);
}
}
I don't love this though. It adds almost-never-executed code, we
addref/release extra times, it could bitrot or forget to grow new
mainthread-only refs. And it wastes code and adds semantic boilerplate
to ignore when reading code.
You could add a macro to build these and hide the boilerplate some, but
that's not wonderful either.
So, if I have my druthers: (And really only #1 is needed, probably)
1) Add already_AddRefed variants of Dispatch/DispatchToMainThread, and
convert things as needed to .forget() (Preferably most
Dispatch/DispatchToMainThreads would become this.) If the Dispatch()
can fail and it's not a logic error somewhere (due to Dispatch to a
doomed thread perhaps in some race conditions), then live with a leak
- we can't send it there to die. DispatchToMainThread() isn't
affected by this and will be infallible soon.
2) If you're building a runnable with a complex lifetime and a
MainThread or thread-locked object, also add the ProxyRelease
destructor. Otherwise consider simple boilerplate (for
MainThread-only runnables) to do:
~FooRunnable() { if (!NS_IsMainThread()) { MOZ_CRASH(); } }
(Perhaps DESTROYED_ON_MAINTHREAD_ONLY(FooRunnable)). Might not be a
bad thing to have in our pockets for other reasons.
3) Consider replacing nsCOMPtr<nsIFoo> mFoo in the runnable with
nsReleaseOnMainThread<nsCOMPtr<nsIFoo>> mFoo and have
~nsReleaseOnMainThread() do NS_ProxyRelease, and also have
nsReleaseOnMainThread block attempts to do assignments that AddRef
(only set via swap or assign from already_AddRefed). Unlike the
MainThreadPtrHolder this could be created on other threads since it
never AddRefs; it only inherits references and releases them on MainThread
I wonder if we could move to requiring already_AddRefed for
DispatchToMainThread (and Dispatch?), and thus block all attempts to do
DispatchToMainThread(new FooRunnable), etc. :-)
--
Randell Jesup, Mozilla Corp
remove "news" for personal email
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
