Re: nsThread now leaks runnables if dispatch fails
Xidorn Quan writes: > On Thu, Jan 7, 2016 at 6:47 AM, Karl Tomlinson wrote: >> Xidorn Quan writes: >> >>> You can keep a raw pointer yourself, and release it manually after you >>> find the dispatch fails, like what is done in >>> NS_DispatchToCurrentThread [1]. It is ugly, but it works and safe, >>> because Dispatch guarantees to leak when it returns failure code for >>> any async dispatch. >>> >>> [1] >>> https://dxr.mozilla.org/mozilla-central/rev/29258f59e5456a1a518ccce6b473b50c1173477e/xpcom/glue/nsThreadUtils.cpp#162-171 >> >> I think it would be much better to keep the sane ref-counting of >> the original implementation and add a different method for use when the >> thread is known to be in a state to accept new runnables but this >> method will assert and leak if the assumption was invalid. > > No, that would make the race condition inevitable. The race condition is not inevitable, but it may exist where code is not managing thread/object lifetimes appropriately. The "different" method can be used where there is risk of that happening. The race condition only matters when the runnable holds references to objects that must be released on a certain thread. These situations need to be managed by the calling code regardless of whether the Dispatch() method would leak or not on failure to manage the object appropriately. Passing references to main thread objects on other threads is difficult and just leaking the objects is not the solution. We can however have a special method for these situations that catches coding failures in a safe manner, provided that behavior is not enforced on callers that want the runnable released. Having the Dispatch() methods not leak means we get the behavior one would expect, which is the behavior that it had until recently. No quirky Release() calls are required by the callers that can safely release their objects on any thread. > It is probably doable to return an already_AddRefed somehow, e.g. via > return value, or a pointer parameter. But it isn't really worth to do > so. Yes, not necessary because the caller has the option to hold a reference if any thread can release. > Wrapping the hack in a desent interface is the best way to go. I don't know what you have in mind here, but the point of this thread is that we don't have that now. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: nsThread now leaks runnables if dispatch fails
On Thu, Jan 7, 2016 at 6:47 AM, Karl Tomlinson wrote: > Xidorn Quan writes: > >> On Wed, Jan 6, 2016 at 3:53 AM, Kyle Huey wrote: >>> On Mon, Jan 4, 2016 at 4:10 PM, Randell Jesup wrote: Yup. In cases where we anticipate a possible Dispatch failure (which is supposed to become impossible, but isn't currently) you can use the (still-existing) raw ptr interface and handle Dispatch failure explicitly to release the (leaked) reference, if it's safe. Not all cases are safe to release in that case (which is what drove the initial bug filing, where it tried to release JS objects on Dispatch failure off mainthread). Leaking is better than crashing/sec-bugs. >>> >>> No, you can't. If you can the raw ptr interface today Dispatch will >>> create its own reference and pass that to the already_AddRefed version >>> which then leaks it. There's no way for the caller to handle this >>> safely. Again, as karlt points out, Dispatch leaks today even if the >>> caller does everything correctly. >> >> You can keep a raw pointer yourself, and release it manually after you >> find the dispatch fails, like what is done in >> NS_DispatchToCurrentThread [1]. It is ugly, but it works and safe, >> because Dispatch guarantees to leak when it returns failure code for >> any async dispatch. >> >> [1] >> https://dxr.mozilla.org/mozilla-central/rev/29258f59e5456a1a518ccce6b473b50c1173477e/xpcom/glue/nsThreadUtils.cpp#162-171 > > I think it would be much better to keep the sane ref-counting of > the original implementation and add a different method for use when the > thread is known to be in a state to accept new runnables but this > method will assert and leak if the assumption was invalid. No, that would make the race condition inevitable. It is probably doable to return an already_AddRefed somehow, e.g. via return value, or a pointer parameter. But it isn't really worth to do so. Wrapping the hack in a desent interface is the best way to go. - Xidorn ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: nsThread now leaks runnables if dispatch fails
Xidorn Quan writes: > On Wed, Jan 6, 2016 at 3:53 AM, Kyle Huey wrote: >> On Mon, Jan 4, 2016 at 4:10 PM, Randell Jesup wrote: >>> >>> Yup. In cases where we anticipate a possible Dispatch failure (which is >>> supposed to become impossible, but isn't currently) you can use the >>> (still-existing) raw ptr interface and handle Dispatch failure >>> explicitly to release the (leaked) reference, if it's safe. Not all >>> cases are safe to release in that case (which is what drove the initial >>> bug filing, where it tried to release JS objects on Dispatch failure off >>> mainthread). Leaking is better than crashing/sec-bugs. >> >> No, you can't. If you can the raw ptr interface today Dispatch will >> create its own reference and pass that to the already_AddRefed version >> which then leaks it. There's no way for the caller to handle this >> safely. Again, as karlt points out, Dispatch leaks today even if the >> caller does everything correctly. > > You can keep a raw pointer yourself, and release it manually after you > find the dispatch fails, like what is done in > NS_DispatchToCurrentThread [1]. It is ugly, but it works and safe, > because Dispatch guarantees to leak when it returns failure code for > any async dispatch. > > [1] > https://dxr.mozilla.org/mozilla-central/rev/29258f59e5456a1a518ccce6b473b50c1173477e/xpcom/glue/nsThreadUtils.cpp#162-171 I think it would be much better to keep the sane ref-counting of the original implementation and add a different method for use when the thread is known to be in a state to accept new runnables but this method will assert and leak if the assumption was invalid. ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: nsThread now leaks runnables if dispatch fails
On Wed, Jan 6, 2016 at 3:53 AM, Kyle Huey wrote: > On Mon, Jan 4, 2016 at 4:10 PM, Randell Jesup wrote: >> >> Yup. In cases where we anticipate a possible Dispatch failure (which is >> supposed to become impossible, but isn't currently) you can use the >> (still-existing) raw ptr interface and handle Dispatch failure >> explicitly to release the (leaked) reference, if it's safe. Not all >> cases are safe to release in that case (which is what drove the initial >> bug filing, where it tried to release JS objects on Dispatch failure off >> mainthread). Leaking is better than crashing/sec-bugs. > > No, you can't. If you can the raw ptr interface today Dispatch will > create its own reference and pass that to the already_AddRefed version > which then leaks it. There's no way for the caller to handle this > safely. Again, as karlt points out, Dispatch leaks today even if the > caller does everything correctly. You can keep a raw pointer yourself, and release it manually after you find the dispatch fails, like what is done in NS_DispatchToCurrentThread [1]. It is ugly, but it works and safe, because Dispatch guarantees to leak when it returns failure code for any async dispatch. [1] https://dxr.mozilla.org/mozilla-central/rev/29258f59e5456a1a518ccce6b473b50c1173477e/xpcom/glue/nsThreadUtils.cpp#162-171 > Leaking is better than crashing/sec-bugs, but that doesn't mean that > we get to introduce leaks to fix crash/sec-bugs, especially when it's > possible to fix the bug through other means. I don't think he suggested we should use leaks to fix crash/sec-bugs. Leaks are bugs which should be fixed as well. But avoiding crash/sec-bug as much as possible at the expense of reasonable amount of leaking can improve our overall stability and security, which might be good. >> Leaks are annoying, but only because of automation meant to catch >> persistent/accumulating leaks - shutdown-time-only leaks are far less >> concerning, and (waving hands) 99% of Dispatch()-failures that create >> leaks are in shutdown. > > We have no data upon which to wave those hands. > > Furthermore leaking a single DOM object is usually enough to leak an > entire page and start clogging up the cycle collector, producing > severe performance degradation over time. Hmmm... but releasing a page in a wrong thread could be a disaster, no? >> I'd be good with flagging/asserting on Dispatch failures *before* shutdown >> begins as a step to making it infallible, since those have much more >> potential to be logic flaws and/or create accumulating leaks. > > So who is going to do that, and who is on the hook for driving > infallible dispatch to completion? I can probably take it as part of bug 1184729. - Xidorn ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: nsThread now leaks runnables if dispatch fails
On Mon, Jan 4, 2016 at 4:10 PM, Randell Jesup wrote: >>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. > > Solving the "who knows what thread this will be released on" was a > primary goal. See comment 0, and the discussion here it references. > > Independently, bsmedberg wanted to make Dispatch infallible (at least > normally), thus making the pattern of "avoid leak in the case of > Dispatch failing" irrelevant (once done, which it hasn't been). This seems like the best path forward. > I started the conversion of Dispatch(rawptr) in the tree to > Dispatch(already_AddRefed<>); xidorn took over finishing it but hasn't > yet. We should re-energize that. >From the perspective of bug 1218297, that change doesn't actually affect anything. > There's considerable discussion in the bug of when leaks occur and also > the assumed behavior of DispatchToMainThread (which is especially > failure-prone because of how XPCOM dispatch works - even when MainThread > still exists, that can fail in shutdown). Ok, cool. I didn't want to read the entire thing :) >>> 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. > > Yup. In cases where we anticipate a possible Dispatch failure (which is > supposed to become impossible, but isn't currently) you can use the > (still-existing) raw ptr interface and handle Dispatch failure > explicitly to release the (leaked) reference, if it's safe. Not all > cases are safe to release in that case (which is what drove the initial > bug filing, where it tried to release JS objects on Dispatch failure off > mainthread). Leaking is better than crashing/sec-bugs. No, you can't. If you can the raw ptr interface today Dispatch will create its own reference and pass that to the already_AddRefed version which then leaks it. There's no way for the caller to handle this safely. Again, as karlt points out, Dispatch leaks today even if the caller does everything correctly. Leaking is better than crashing/sec-bugs, but that doesn't mean that we get to introduce leaks to fix crash/sec-bugs, especially when it's possible to fix the bug through other means. > If the problem is that when this happens, a leak is reported by infra, > then we could ping the leak-checker that there was a dispatch failure > and leaks are expected. Even better but maybe not possible would be > annotate the root of the leak and suppress anything tied to it. The problem is not that a leak is reported, the problem is that we leak. Automatically hiding our bugs is not good engineering practice. >>> 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. > > There's another (likely impractical) option: wrap all thread-sensitive > pointers in a ReleaseRefOnThread<>
Re: nsThread now leaks runnables if dispatch fails
>> 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 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.
>>
>
>I am a bit confused with this, if the executing thread has already shut
>down, why would releasing the reference dispatching thread holds
>reintroduce the race condition? Who is racing with dispatching thread?
It doesn't re-introduce a race. What it may do is cause the same effect
as a lost race, whereby an object meant to be released on the target
thread gets released on the sending thread. If the object is a
non-threadsafe object (say, for example, a JS success or failure
callback object), then releasing it on the sending thread is Bad. This
is the same thing that can happen in a race where you have:
What used to happen:
{
RefPtr bar = new FooRunnable(nonthreadsafe_object);
target_thread->Dispatch(bar, ...);
// Note: Dispatch in this (old-style) case takes a new reference to
// bar. And the runnable may run and release that reference before
// Dispatch() returns! And if Dispatch() fails, you always 'lose'
// the race.
}
// bar drops the reference. If the runnable ran already, this is
// the last reference and it gets destroyed here. If it hasn't run,
// it gets destroyed on the target thread.
New way (already_AddRefed<>):
{
RefPtr bar = new FooRunnable(nonthreadsafe_object);
target_thread->Dispatch(bar.forget(), ...);
// bar is always destroyed on the target thread - unless
// Dispatch() fails, in which case it leaks (for safety).
}
or:
other_thread->Dispatch(do_AddRef(new FooRunnable(nonthreadsafe_object)),
...);
If you use the old-style (raw ptr Dispatch) as a lot of the tree still
does, it will now leak on Dispatch() failure instead of possibly
wrong-thread releasing:
nsThread.h:
nsresult Dispatch(nsIRunnable* aEvent, uint32_t aFlags) {
return Dispatch(nsCOMPtr(aEvent).forget(), aFlags);
}
Since Dispatch(already_AddRefed<>,...) leaks on failure, this means that
Dispatch(raw_ptr,...) now leaks on failure (which is what engendered
this discussion). You can override this behavior with a check of the
result of Dispatch() and a manual Release if it fails (and you know
the object is entirely threadsafe).
We could add DispatchFallible() (DispatchNeverLeaks()?) if that's a
wide-enough pattern; part of my question in that case is "did the caller
really expect Dispatch to possibly fail, and what implications does such
a failure have to the rest of the system?"
--
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
Re: nsThread now leaks runnables if dispatch fails
The "race condition" here is the question of which thread the runnable destructor runs on. If the executing thread is already shut down the destructor still runs on the "wrong" dispatching thread (or not at all). Since the point of fixing the original race is to guarantee that the destructor runs on the executing thread it's useful to consider this an extension of the original race. - Kyle On Mon, Jan 4, 2016 at 10:39 PM, Ting-Yu Chou wrote: > >> 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 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. > > > I am a bit confused with this, if the executing thread has already shut > down, > why would releasing the reference dispatching thread holds reintroduce the > race > condition? Who is racing with dispatching thread? > > Ting ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: nsThread now leaks runnables if dispatch fails
> 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 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. > I am a bit confused with this, if the executing thread has already shut down, why would releasing the reference dispatching thread holds reintroduce the race condition? Who is racing with dispatching thread? Ting ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
Re: nsThread now leaks runnables if dispatch fails
>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. Solving the "who knows what thread this will be released on" was a primary goal. See comment 0, and the discussion here it references. Independently, bsmedberg wanted to make Dispatch infallible (at least normally), thus making the pattern of "avoid leak in the case of Dispatch failing" irrelevant (once done, which it hasn't been). I started the conversion of Dispatch(rawptr) in the tree to Dispatch(already_AddRefed<>); xidorn took over finishing it but hasn't yet. We should re-energize that. There's considerable discussion in the bug of when leaks occur and also the assumed behavior of DispatchToMainThread (which is especially failure-prone because of how XPCOM dispatch works - even when MainThread still exists, that can fail in shutdown). >> 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. Yup. In cases where we anticipate a possible Dispatch failure (which is supposed to become impossible, but isn't currently) you can use the (still-existing) raw ptr interface and handle Dispatch failure explicitly to release the (leaked) reference, if it's safe. Not all cases are safe to release in that case (which is what drove the initial bug filing, where it tried to release JS objects on Dispatch failure off mainthread). Leaking is better than crashing/sec-bugs. If the problem is that when this happens, a leak is reported by infra, then we could ping the leak-checker that there was a dispatch failure and leaks are expected. Even better but maybe not possible would be annotate the root of the leak and suppress anything tied to it. >> 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. There's another (likely impractical) option: wrap all thread-sensitive pointers in a ReleaseRefOnThread<> holder which refuses to release objects on the wrong thread and RELEASE_ASSERTS, and integrate that with Dispatch's failure path to not assert if you try to release it due to a Dispatch failure and instead leak and mark it somehow for automation. Leaks are annoying, but only because of automation meant to catch persistent/accumulating leaks - shutdown-time-only leaks are far less concerning, and (waving hands) 99% of Dispatch()-failures that create leaks are in shutdown. Also - blocking event processing and DispatchToMainthread before the final cycle-collect adds to the problem since anything released in the final CC pass can't DispatchToMainthread; this has forced a number of tricks or bits of code to handle Dispatch failure. Once upon a time we didn't do very much with other threads, and also didn't pass JS references around, etc. We use threads a lot more and in much more complex ways now. As Nathan said in the bug: > Well, the runnables themselves ar
Re: nsThread now leaks runnables if dispatch fails
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
Re: nsThread now leaks runnables if dispatch fails
It seems like we should make the default behavior infallible + assert, and introduce a separate dispatch method for the fallible cases. On Mon, Jan 4, 2016 at 10:50 AM, Kyle Huey wrote: > (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. So far, so > good. > > 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 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. > > - Kyle > ___ > dev-platform mailing list > dev-platform@lists.mozilla.org > https://lists.mozilla.org/listinfo/dev-platform > ___ dev-platform mailing list dev-platform@lists.mozilla.org https://lists.mozilla.org/listinfo/dev-platform
