Kyle Huey writes: > (This is a continuation of the discussion in bug 1218297) > > In bug 1155059 we made nsIEventTarget::Dispatch take an > already_AddRefed instead of a raw pointer. This was done to allow the > dispatcher to transfer its reference to the runnable to the thread the > runnable will run on. That solves a race condition we've had > historically where the destructor of a runnable can run on either the > dispatching thread or the executing thread, depending on whether the > executing thread can run the event to completion before the > dispatching thread destroys the nsCOMPtr on the stack.
IIUC solving the race condition wasn't the goal of the change in API, but something that was done to retain existing workarounds for leaks. > In bug 1218297 we saw a case where dispatch to a thread (the socket > transport service thread in this case) fails because the thread has > already shut down. In our brave new world, nsThread simply leaks the > runnable. It did previously leak in some quirky situations where objects were intentionally created with no reference so as to leak. e.g. auto runnable = new FooRunnable(...); target->Dispatch(runnable, flags); Since https://hg.mozilla.org/mozilla-central/rev/2265e031ab97#l25.46 however, Dispatch() leaks even when the caller is doing ref-counting properly. > It can't release the reference it holds, because that would > reintroduce the race condition we wanted to avoid, and it can't > release the reference on the correct thread so it's already gone away. > But now we have a new source of intermittent leaks. > > Was this anticipated when designing bug 1155059? I don't think > leaking is acceptable here, so we may need to back that out and return > to living with that race condition. I agree this approach is not going to work out. Short term, I think the situation could be improved and most of those changes kept by adding a DispatchOrLeak() variant that can be used by the callers that want to leak. Then we still have the leaks we had prior to 2265e031ab97 but the new ones are resolved. For the remaining (old) leaks I can think of two options: 1) Never call Dispatch() when it might fail and assert that it does not. 2) Keep and additional reference to the runnable in the caller if it doesn't want Dispatch() to release the last reference. With the former, the caller needs to ensure the thread lives long enough for the Dispatch() to succeed, or that clean up happens before the thread is closed. With the latter, the caller needs to find another way to release when Dispatch() fails. Making Dispatch() infallible would permit only option 1. Keeping Dispatch() fallible allows the caller to use either option. _______________________________________________ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
