|
|
DescriptionBindToCurrentLoop should delete callback on original thread
It was pointed out on https://codereview.chromium.org/1083883003 that
the callback wrapped by media::base::BindToCurrentLoop can get deleted
on an arbitrary thread. This patch makes the callback always be deleted
on the thread from which BindToCurrentLoop was invoked.
BUG=432381
Patch Set 1 #
Total comments: 6
Patch Set 2 : Alternative impl, with scoped_ptr<Callback, DeleteOnCurrentLoop>* #
Total comments: 20
Patch Set 3 : Original impl, with nits addressed #Patch Set 4 : Use ThreadTaskRunnerHandle::Get() instead of MessageLoop::current()->task_runner() #Patch Set 5 : Address danakj's review comments #Patch Set 6 : Rebase #Patch Set 7 : Rebase #
Messages
Total messages: 18 (1 generated)
johnme@chromium.org changed reviewers: + danakj@chromium.org, xhwang@chromium.org
danakj: please review patch (hopefully it addresses your comments in https://codereview.chromium.org/1083883003) xhwang: please give owners review Thanks!
https://codereview.chromium.org/1082113004/diff/1/media/base/bind_to_current_... File media/base/bind_to_current_loop.h (right): https://codereview.chromium.org/1082113004/diff/1/media/base/bind_to_current_... media/base/bind_to_current_loop.h:67: } else if (!origin_loop_->DeleteSoon(from_here_, cb_ptr_)) { I think you want FROM_HERE here? Or why do you use from_here_ here? https://codereview.chromium.org/1082113004/diff/1/media/base/bind_to_current_... media/base/bind_to_current_loop.h:75: LOG(ERROR) << "DeleteSoon failed"; FATAL? https://codereview.chromium.org/1082113004/diff/1/media/base/bind_to_current_... media/base/bind_to_current_loop.h:83: from_here_, base::Bind(*cb_ptr_, internal::TrampolineForward(args)...)); Would it be simpler to do this as follows? template<Args...> void RelayOnOtherThread(STTR* origin_task_runner, scoped_ptr<Callback<...>> origin_copy_cb, Args... args) { // closure holds a ref on anything in origin_copy_cb, so we can delete origin_copy_cb on the wrong thread here. scoped_ptr<base::Closure> origin_closure(new Bind(*origin_copy_cb, args)); origin_copy_cb.reset(); origin_task_runner->PostTask(&RelayOnOriginThread<...>, base::Passed(&closure)); } void RelayOnOriginThread(STTR* origin_task_runner, scoped_ptr<Closure> origin_closure) { origin_closure->Run(args); // origin_closure is destroyed on the origin thread here. } BindToCurrentLoop(const Callback<...>& origin_cb) { scoped_ptr<Callback<...>> origin_copy_cb = new Callback<...>(origin_cb); return Bind(&RelayOnOtherThread, TTRH::Get(), base::Passed(&origin_copy_cb)); } It uses 1 templated function instead of a templated function+struct, and it deletes the callback when it runs it instead of posting a separate task.
I like this idea of this CL. I'll wait for you to address danakj@'s comment to take a look though. Thanks for taking this!
https://codereview.chromium.org/1082113004/diff/1/media/base/bind_to_current_... File media/base/bind_to_current_loop.h (right): https://codereview.chromium.org/1082113004/diff/1/media/base/bind_to_current_... media/base/bind_to_current_loop.h:67: } else if (!origin_loop_->DeleteSoon(from_here_, cb_ptr_)) { On 2015/04/24 19:00:03, danakj wrote: > I think you want FROM_HERE here? Or why do you use from_here_ here? Ok, I've changed both the |from_here_|s to FROM_HERE. I kept the comment though, as I think it would be better if BindToCurrentLoop took a tracked_objects::Location parameter, but that's a separate refactoring. https://codereview.chromium.org/1082113004/diff/1/media/base/bind_to_current_... media/base/bind_to_current_loop.h:75: LOG(ERROR) << "DeleteSoon failed"; On 2015/04/24 19:00:03, danakj wrote: > FATAL? I copied this from https://codereview.chromium.org/6691044, where jam suggested not to use FATAL, because we know about and are ok with leaks at shutdown, and was worried that FATAL would introduce confusing flaky crashes during development. https://codereview.chromium.org/1082113004/diff/1/media/base/bind_to_current_... media/base/bind_to_current_loop.h:83: from_here_, base::Bind(*cb_ptr_, internal::TrampolineForward(args)...)); On 2015/04/24 19:00:03, danakj wrote: > Would it be simpler to do this as follows? > > template<Args...> > void RelayOnOtherThread(STTR* origin_task_runner, > scoped_ptr<Callback<...>> origin_copy_cb, > Args... args) { > // closure holds a ref on anything in origin_copy_cb, so we can delete > origin_copy_cb on the wrong thread here. > scoped_ptr<base::Closure> origin_closure(new Bind(*origin_copy_cb, args)); > origin_copy_cb.reset(); > origin_task_runner->PostTask(&RelayOnOriginThread<...>, > base::Passed(&closure)); > } > > void RelayOnOriginThread(STTR* origin_task_runner, > scoped_ptr<Closure> origin_closure) { > origin_closure->Run(args); > // origin_closure is destroyed on the origin thread here. > } > > BindToCurrentLoop(const Callback<...>& origin_cb) { > scoped_ptr<Callback<...>> origin_copy_cb = new Callback<...>(origin_cb); > return Bind(&RelayOnOtherThread, TTRH::Get(), base::Passed(&origin_copy_cb)); > } If you delete the callback returned from BindToCurrentLoop on a different thread, then AIUI the bound parameters will be deleted on that thread, and hence origin_cb_copy will be deleted on the wrong thread. I thought avoiding that was the whole point of this patch? Also, using base::Passed to pass a scoped_ptr and deleting it when the callback is run means the callback can only be run once, which is an odd restriction to place on a general-purpose method like this. Finally new Bind(*origin_copy_cb, args) tries to copy the arguments; the quirky TrampolineForward does unfortunately seem to be necessary. I tried to see if I could get something more along these lines, and I've uploaded the closest I got as Patch Set 2 (https://codereview.chromium.org/1082113004/#ps20001). It introduces a non-template struct called DeleteOnCurrentLoop (which might be of general utility). But overall it seems less legible (due to the heap-allocated scoped_ptr hack), so I reuploaded the original implementation (with the nits from your other comments fixed) as Patch Set 3 (https://codereview.chromium.org/1082113004/#ps40001). If you prefer it, I'm happy to go back to Patch Set 2 though. > It uses 1 templated function instead of a templated function+struct, and it > deletes the callback when it runs it instead of posting a separate task. Doesn't your BindToCurrentLoop(const Callback<...>& origin_cb) needs to be a template function too?
On Mon, Apr 27, 2015 at 11:32 AM, <johnme@chromium.org> wrote: > > > https://codereview.chromium.org/1082113004/diff/1/media/base/bind_to_current_... > File media/base/bind_to_current_loop.h (right): > > > https://codereview.chromium.org/1082113004/diff/1/media/base/bind_to_current_... > media/base/bind_to_current_loop.h:67: } else if > (!origin_loop_->DeleteSoon(from_here_, cb_ptr_)) { > On 2015/04/24 19:00:03, danakj wrote: > >> I think you want FROM_HERE here? Or why do you use from_here_ here? >> > > Ok, I've changed both the |from_here_|s to FROM_HERE. I kept the comment > though, as I think it would be better if BindToCurrentLoop took a > tracked_objects::Location parameter, but that's a separate refactoring. > > > https://codereview.chromium.org/1082113004/diff/1/media/base/bind_to_current_... > media/base/bind_to_current_loop.h:75: LOG(ERROR) << "DeleteSoon failed"; > On 2015/04/24 19:00:03, danakj wrote: > >> FATAL? >> > > I copied this from https://codereview.chromium.org/6691044, where jam > suggested not to use FATAL, because we know about and are ok with leaks > at shutdown, and was worried that FATAL would introduce confusing flaky > crashes during development. > > > https://codereview.chromium.org/1082113004/diff/1/media/base/bind_to_current_... > media/base/bind_to_current_loop.h:83: from_here_, base::Bind(*cb_ptr_, > internal::TrampolineForward(args)...)); > On 2015/04/24 19:00:03, danakj wrote: > >> Would it be simpler to do this as follows? >> > > template<Args...> >> void RelayOnOtherThread(STTR* origin_task_runner, >> scoped_ptr<Callback<...>> origin_copy_cb, >> Args... args) { >> // closure holds a ref on anything in origin_copy_cb, so we can >> > delete > >> origin_copy_cb on the wrong thread here. >> scoped_ptr<base::Closure> origin_closure(new Bind(*origin_copy_cb, >> > args)); > >> origin_copy_cb.reset(); >> origin_task_runner->PostTask(&RelayOnOriginThread<...>, >> base::Passed(&closure)); >> } >> > > void RelayOnOriginThread(STTR* origin_task_runner, >> scoped_ptr<Closure> origin_closure) { >> origin_closure->Run(args); >> // origin_closure is destroyed on the origin thread here. >> } >> > > BindToCurrentLoop(const Callback<...>& origin_cb) { >> scoped_ptr<Callback<...>> origin_copy_cb = new >> > Callback<...>(origin_cb); > >> return Bind(&RelayOnOtherThread, TTRH::Get(), >> > base::Passed(&origin_copy_cb)); > >> } >> > > If you delete the callback returned from BindToCurrentLoop on a > different thread, then AIUI the bound parameters will be deleted on that > thread, and hence origin_cb_copy will be deleted on the wrong thread. I > thought avoiding that was the whole point of this patch? > My thought was that you copy into a new cb right before this, so anything refcounted would already be reffed and not deleted here. > > Also, using base::Passed to pass a scoped_ptr and deleting it when the > callback is run means the callback can only be run once, which is an odd > restriction to place on a general-purpose method like this. > This is true. Do we really need something so general though? This is for async replies. There should always be a single reply to such things, no? > > Finally new Bind(*origin_copy_cb, args) tries to copy the arguments; the > quirky TrampolineForward does unfortunately seem to be necessary. > > I tried to see if I could get something more along these lines, and I've > uploaded the closest I got as Patch Set 2 > (https://codereview.chromium.org/1082113004/#ps20001). It introduces a > non-template struct called DeleteOnCurrentLoop (which might be of > general utility). But overall it seems less legible (due to the > heap-allocated scoped_ptr hack), so I reuploaded the original > implementation (with the nits from your other comments fixed) as Patch > Set 3 (https://codereview.chromium.org/1082113004/#ps40001). If you > prefer it, I'm happy to go back to Patch Set 2 though. > > It uses 1 templated function instead of a templated function+struct, >> > and it > >> deletes the callback when it runs it instead of posting a separate >> > task. > > Doesn't your BindToCurrentLoop(const Callback<...>& origin_cb) needs to > be a template function too? > > https://codereview.chromium.org/1082113004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/04/29 20:42:54, danakj wrote: > On Mon, Apr 27, 2015 at 11:32 AM, johnme wrote: > > On 2015/04/24 19:00:03, danakj wrote: > > > > > > If you delete the callback returned from BindToCurrentLoop on a > > different thread, then AIUI the bound parameters will be deleted on that > > thread, and hence origin_cb_copy will be deleted on the wrong thread. I > > thought avoiding that was the whole point of this patch? > > > > My thought was that you copy into a new cb right before this, so anything > refcounted would already be reffed and not deleted here. I'm not talking about the `origin_copy_cb.reset()` line - that's fine. But if the callback returned from BindToCurrentLoop (i.e. `Bind(&RelayOnOtherThread, ...)`) gets deleted on a different thread - as seems quite likely - then AIUI so will the arguments. That's why I have to use a struct with a custom destructor that deletes the original callback (and hence its arguments) on the origin_loop_ (thread that invoked BindToCurrentLoop) (or why in patch set 2 I went to all the trouble with the scoped_ptr that deletes its contents on the origin_loop_). > > Also, using base::Passed to pass a scoped_ptr and deleting it when the > > callback is run means the callback can only be run once, which is an odd > > restriction to place on a general-purpose method like this. > > This is true. Do we really need something so general though? This is for > async replies. There should always be a single reply to such things, no? The default assumption with Callbacks/Closures is that they could be run multiples times, even if they usually won't be. Since it's no harder to support that (just use Owned instead of Passed), it seems that's the path of least astonishment.
On Wed, Apr 29, 2015 at 2:18 PM, <johnme@chromium.org> wrote: > On 2015/04/29 20:42:54, danakj wrote: > >> On Mon, Apr 27, 2015 at 11:32 AM, johnme wrote: >> > On 2015/04/24 19:00:03, danakj wrote: >> > >> > > > >> > If you delete the callback returned from BindToCurrentLoop on a >> > different thread, then AIUI the bound parameters will be deleted on that >> > thread, and hence origin_cb_copy will be deleted on the wrong thread. I >> > thought avoiding that was the whole point of this patch? >> > >> > > My thought was that you copy into a new cb right before this, so anything >> refcounted would already be reffed and not deleted here. >> > > I'm not talking about the `origin_copy_cb.reset()` line - that's fine. But > if > the callback returned from BindToCurrentLoop (i.e. > `Bind(&RelayOnOtherThread, > ...)`) gets deleted on a different thread - as seems quite likely - then > AIUI so > will the arguments. That's why I have to use a struct with a custom > destructor > Ya, but it's a scoped_ptr it can't be deleted twice. The copy of that Bind left on the wrong thread would have an empty pointer (Passed to the origin thread) so it wouldn't delete anything. The callback on the origin thread gets a scoped_ptr<> of the Callback so it has to have ownership of the pointer, so while you end up with a Callback on each thread during the Post, only one has the Passed pointer. > that deletes the original callback (and hence its arguments) on the > origin_loop_ > (thread that invoked BindToCurrentLoop) (or why in patch set 2 I went to > all the > trouble with the scoped_ptr that deletes its contents on the origin_loop_). > > > Also, using base::Passed to pass a scoped_ptr and deleting it when the >> > callback is run means the callback can only be run once, which is an odd >> > restriction to place on a general-purpose method like this. >> > > This is true. Do we really need something so general though? This is for >> async replies. There should always be a single reply to such things, no? >> > > The default assumption with Callbacks/Closures is that they could be run > multiples times, even if they usually won't be. Since it's no harder to > support > that (just use Owned instead of Passed), it seems that's the path of least > astonishment. > > https://codereview.chromium.org/1082113004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... File media/base/bind_to_current_loop.h (right): https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:23: // media::base::BindToCurrentLoop(base::Bind(&MyClass::MyMethod, this))); no base::? https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:71: // scoped_ptr<Foo, media::base::DeleteOnCurrentLoop> ptr; no base::? https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:72: struct MEDIA_EXPORT DeleteOnCurrentLoop { should this be inside anon/internal? https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:73: DeleteOnCurrentLoop(); i prefer this take a STTR* as an argument instead of finding the current one. Do that finding in BindToCurrentLoop https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:86: LOG(ERROR) << "DeleteSoon failed"; I'd still prefer LOG(FATAL) and maybe a way to turn this off for a specific test if it really shows itself as an unsolvable problem? So far I think there's only speculation. Maybe a "SetLeakOnDestroy" or something if we needed it, and it could just not try delete then or something even. https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:96: namespace { why is this anon but the above is internal? https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:104: origin_task_runner->PostTask( I think that I prefer posting to our own method on the origin thread here, and passing ownership of the callback there to run it, rather than posting the original callback here, and letting the destructor handle the posttask. Then we don't need the custom deletor? https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:117: auto cb_copy_ptr_ptr = new scoped_ptr<base::Callback<void(A...)>, can we do this with a scoped_ptr Passed instead of a scoped_ptr* which is making this harder to understand. it'll make the callback only callable once, but that's maybe okay? https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:120: base::MessageLoop::current()->task_runner(), ThreadTaskRunnerHandler::Get()?
https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... File media/base/bind_to_current_loop.h (right): https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:79: if (origin_loop_->BelongsToCurrentThread()) { oh and no conditional posting please
Addressed/replied to review comments - thanks :) https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... File media/base/bind_to_current_loop.h (right): https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:23: // media::base::BindToCurrentLoop(base::Bind(&MyClass::MyMethod, this))); On 2015/04/29 23:35:01, danakj wrote: > no base::? Done. https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:71: // scoped_ptr<Foo, media::base::DeleteOnCurrentLoop> ptr; On 2015/04/29 23:35:01, danakj wrote: > no base::? Done (ditto above). https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:72: struct MEDIA_EXPORT DeleteOnCurrentLoop { On 2015/04/29 23:35:01, danakj wrote: > should this be inside anon/internal? I've moved DeleteOnLoop into internal (a subclass I created for your comment below about explicitly passing in the loop). However I think DeleteOnCurrentLoop could be useful to expose. Almost every RefCountedThreadSafe has thread-racey destruction, except for those that use content::BrowserThread::DeleteOn*Thread as traits. But you can only use those traits from parts of the codebase that have access to content/public; this could be an essential companion to RefCountedThreadSafe everywhere else. Maybe I should save DeleteOnCurrentLoop for a follow-up patch though, so it can be discussed on its own (and only include internal::DeleteOnLoop in this patch)? https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:73: DeleteOnCurrentLoop(); On 2015/04/29 23:35:01, danakj wrote: > i prefer this take a STTR* as an argument instead of finding the current one. Do > that finding in BindToCurrentLoop Done. https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:79: if (origin_loop_->BelongsToCurrentThread()) { On 2015/04/29 23:37:34, danakj wrote: > oh and no conditional posting please I modeled this on BrowserThread::DeleteOnThread, which does conditional DeleteSoon: https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... The original code was written by jam/darin: https://codereview.chromium.org/338065 I'm guessing their reasoning is that we may as well delete it if we have the chance, whereas if we delay it may no longer be possible to delete it. I tried removing the conditional and always using DeleteSoon, but that causes the following tests to fail: NullVideoSinkTest.BasicFunctionality VideoRendererImplTest.StartPlayingFrom_LowDelay VideoRendererImplTest.Underflow These unit tests each have a MessageLoop owned by the test class. They wait for some event on the message loop, then return, which causes the test class to be destroyed. Hence the MessageLoop is destroyed, and hence the DeleteOnLoop scoped_ptr to the callback ends up being destructed. With the conditional this is fine -- it's already on the right thread, so it goes ahead and synchronously deletes the callback. But without the conditional, it tries to send a DeleteSoon to the being-deleted message loop, which fails and our LOG(FATAL) causes the test to crash. https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:86: LOG(ERROR) << "DeleteSoon failed"; On 2015/04/29 23:35:01, danakj wrote: > I'd still prefer LOG(FATAL) and maybe a way to turn this off for a specific test > if it really shows itself as an unsolvable problem? So far I think there's only > speculation. Maybe a "SetLeakOnDestroy" or something if we needed it, and it > could just not try delete then or something even. Done (hmm ok, I re-read https://codereview.chromium.org/6691044/ and it seems the key point is the UNIT_TEST guards, but FATAL should be ok since we've got those). https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:96: namespace { Done (oops, shouldn't use anonymous namespaces in a header; and template function definitions have to go in the header. So I've moved PostBackToOriginLoop to internal:: as well) https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:104: origin_task_runner->PostTask( On 2015/04/29 23:35:01, danakj wrote: > I think that I prefer posting to our own method on the origin thread here, and > passing ownership of the callback there to run it, rather than posting the > original callback here, and letting the destructor handle the posttask. Then we > don't need the custom deletor? We'd still need the custom deleter in case it doesn't get run at all. So I don't think this gains us much? https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:117: auto cb_copy_ptr_ptr = new scoped_ptr<base::Callback<void(A...)>, On 2015/04/29 23:35:01, danakj wrote: > can we do this with a scoped_ptr Passed instead of a scoped_ptr* which is making > this harder to understand. it'll make the callback only callable once, but > that's maybe okay? Only allowing the callback to be called once breaks 57 tests in media_unittests. https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... media/base/bind_to_current_loop.h:120: base::MessageLoop::current()->task_runner(), On 2015/04/29 23:35:01, danakj wrote: > ThreadTaskRunnerHandler::Get()? Done (and in .cc).
On Thu, Apr 30, 2015 at 9:15 AM, <johnme@chromium.org> wrote: > Addressed/replied to review comments - thanks :) > > > > https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... > File media/base/bind_to_current_loop.h (right): > > > https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... > media/base/bind_to_current_loop.h:23: // > media::base::BindToCurrentLoop(base::Bind(&MyClass::MyMethod, this))); > On 2015/04/29 23:35:01, danakj wrote: > >> no base::? >> > > Done. > > > https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... > media/base/bind_to_current_loop.h:71: // scoped_ptr<Foo, > media::base::DeleteOnCurrentLoop> ptr; > On 2015/04/29 23:35:01, danakj wrote: > >> no base::? >> > > Done (ditto above). > > > https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... > media/base/bind_to_current_loop.h:72: struct MEDIA_EXPORT > DeleteOnCurrentLoop { > On 2015/04/29 23:35:01, danakj wrote: > >> should this be inside anon/internal? >> > > I've moved DeleteOnLoop into internal (a subclass I created for your > comment below about explicitly passing in the loop). > > However I think DeleteOnCurrentLoop could be useful to expose. Almost > every RefCountedThreadSafe has thread-racey destruction, except for > those that use content::BrowserThread::DeleteOn*Thread as traits. But > you can only use those traits from parts of the codebase that have > access to content/public; this could be an essential companion to > RefCountedThreadSafe everywhere else. > This is where I may appear to contradict myself perhaps. I don't think that making RefCountedThreadSafe things that can only be deleted on a single thread is a valid pattern. If it can't be deleted anywhere it's not RefCountedThreadSafe, I think these are incompatible contracts. OTOH what I'm asking here is to support that in BindToCurrentLoop because I know people do this sort of thing anyhow. But I don't want to encourage it with more helpers. > > Maybe I should save DeleteOnCurrentLoop for a follow-up patch though, so > it can be discussed on its own (and only include internal::DeleteOnLoop > in this patch)? > > > https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... > media/base/bind_to_current_loop.h:73: DeleteOnCurrentLoop(); > On 2015/04/29 23:35:01, danakj wrote: > >> i prefer this take a STTR* as an argument instead of finding the >> > current one. Do > >> that finding in BindToCurrentLoop >> > > Done. > > > https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... > media/base/bind_to_current_loop.h:79: if > (origin_loop_->BelongsToCurrentThread()) { > On 2015/04/29 23:37:34, danakj wrote: > >> oh and no conditional posting please >> > > I modeled this on BrowserThread::DeleteOnThread, which does conditional > DeleteSoon: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... > > The original code was written by jam/darin: > > https://codereview.chromium.org/338065 > > I'm guessing their reasoning is that we may as well delete it if we have > the chance, whereas if we delay it may no longer be possible to delete > it. > > I tried removing the conditional and always using DeleteSoon, but that > causes the following tests to fail: > NullVideoSinkTest.BasicFunctionality > VideoRendererImplTest.StartPlayingFrom_LowDelay > VideoRendererImplTest.Underflow > > These unit tests each have a MessageLoop owned by the test class. They > wait for some event on the message loop, then return, which causes the > test class to be destroyed. Hence the MessageLoop is destroyed, and > hence the DeleteOnLoop scoped_ptr to the callback ends up being > destructed. With the conditional this is fine -- it's already on the > right thread, so it goes ahead and synchronously deletes the callback. > But without the conditional, it tries to send a DeleteSoon to the > being-deleted message loop, which fails and our LOG(FATAL) causes the > test to crash. Can we RunUntilIdle before exiting the test? > > > > https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... > media/base/bind_to_current_loop.h:86: LOG(ERROR) << "DeleteSoon failed"; > On 2015/04/29 23:35:01, danakj wrote: > >> I'd still prefer LOG(FATAL) and maybe a way to turn this off for a >> > specific test > >> if it really shows itself as an unsolvable problem? So far I think >> > there's only > >> speculation. Maybe a "SetLeakOnDestroy" or something if we needed it, >> > and it > >> could just not try delete then or something even. >> > > Done (hmm ok, I re-read https://codereview.chromium.org/6691044/ and it > seems the key point is the UNIT_TEST guards, but FATAL should be ok > since we've got those). > > > https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... > media/base/bind_to_current_loop.h:96: namespace { > Done (oops, shouldn't use anonymous namespaces in a header; and template > function definitions have to go in the header. So I've moved > PostBackToOriginLoop to internal:: as well) > > > https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... > media/base/bind_to_current_loop.h:104: origin_task_runner->PostTask( > On 2015/04/29 23:35:01, danakj wrote: > >> I think that I prefer posting to our own method on the origin thread >> > here, and > >> passing ownership of the callback there to run it, rather than posting >> > the > >> original callback here, and letting the destructor handle the >> > posttask. Then we > >> don't need the custom deletor? >> > > We'd still need the custom deleter in case it doesn't get run at all. So > I don't think this gains us much? > > > https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... > media/base/bind_to_current_loop.h:117: auto cb_copy_ptr_ptr = new > scoped_ptr<base::Callback<void(A...)>, > On 2015/04/29 23:35:01, danakj wrote: > >> can we do this with a scoped_ptr Passed instead of a scoped_ptr* which >> > is making > >> this harder to understand. it'll make the callback only callable once, >> > but > >> that's maybe okay? >> > > Only allowing the callback to be called once breaks 57 tests in > media_unittests. > > > https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... > media/base/bind_to_current_loop.h:120: > base::MessageLoop::current()->task_runner(), > On 2015/04/29 23:35:01, danakj wrote: > >> ThreadTaskRunnerHandler::Get()? >> > > Done (and in .cc). > > https://codereview.chromium.org/1082113004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/04/30 16:42:25, danakj wrote: > https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... > > media/base/bind_to_current_loop.h:72: struct MEDIA_EXPORT > > DeleteOnCurrentLoop { > On Thu, Apr 30, 2015 at 9:15 AM, <mailto:johnme@chromium.org> wrote: > > On 2015/04/29 23:35:01, danakj wrote: > > > should this be inside anon/internal? > > > > I've moved DeleteOnLoop into internal (a subclass I created for your > > comment below about explicitly passing in the loop). > > > > However I think DeleteOnCurrentLoop could be useful to expose. Almost > > every RefCountedThreadSafe has thread-racey destruction, except for > > those that use content::BrowserThread::DeleteOn*Thread as traits. But > > you can only use those traits from parts of the codebase that have > > access to content/public; this could be an essential companion to > > RefCountedThreadSafe everywhere else. > > > > This is where I may appear to contradict myself perhaps. I don't think that > making RefCountedThreadSafe things that can only be deleted on a single > thread is a valid pattern. If it can't be deleted anywhere it's not > RefCountedThreadSafe, I think these are incompatible contracts. OTOH what > I'm asking here is to support that in BindToCurrentLoop because I know > people do this sort of thing anyhow. But I don't want to encourage it with > more helpers. I agree that RefCountedThreadSafe classes ought to be built so it's safe to release references to them on any thread (even though that's not always true today). That's precisely why the content::BrowserThread::DeleteOnUIThread etc traits that you can pass as a template parameter to RefCountedThreadSafe are so useful - because they allow a RefCountedThreadSafe to run its destructor on a safe thread, without having to worry about what thread released the last reference. > > Maybe I should save DeleteOnCurrentLoop for a follow-up patch though, so > > it can be discussed on its own (and only include internal::DeleteOnLoop > > in this patch)? > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... > > > > The original code was written by jam/darin: > > > > https://codereview.chromium.org/338065 > > > > I'm guessing their reasoning is that we may as well delete it if we have > > the chance, whereas if we delay it may no longer be possible to delete > > it. > > > > I tried removing the conditional and always using DeleteSoon, but that > > causes the following tests to fail: > > NullVideoSinkTest.BasicFunctionality > > VideoRendererImplTest.StartPlayingFrom_LowDelay > > VideoRendererImplTest.Underflow > > > > These unit tests each have a MessageLoop owned by the test class. They > > wait for some event on the message loop, then return, which causes the > > test class to be destroyed. Hence the MessageLoop is destroyed, and > > hence the DeleteOnLoop scoped_ptr to the callback ends up being > > destructed. With the conditional this is fine -- it's already on the > > right thread, so it goes ahead and synchronously deletes the callback. > > But without the conditional, it tries to send a DeleteSoon to the > > being-deleted message loop, which fails and our LOG(FATAL) causes the > > test to crash. > > > Can we RunUntilIdle before exiting the test? No, I tried that :-| In fact, VideoRendererImplTest::Destroy() does that already at the end of each test before deleting the MessageLoop.
On Thu, Apr 30, 2015 at 10:09 AM, <johnme@chromium.org> wrote: > On 2015/04/30 16:42:25, danakj wrote: > > > https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... > >> > media/base/bind_to_current_loop.h:72: struct MEDIA_EXPORT >> > DeleteOnCurrentLoop { >> On Thu, Apr 30, 2015 at 9:15 AM, <mailto:johnme@chromium.org> wrote: >> > On 2015/04/29 23:35:01, danakj wrote: >> > > should this be inside anon/internal? >> > >> > I've moved DeleteOnLoop into internal (a subclass I created for your >> > comment below about explicitly passing in the loop). >> > >> > However I think DeleteOnCurrentLoop could be useful to expose. Almost >> > every RefCountedThreadSafe has thread-racey destruction, except for >> > those that use content::BrowserThread::DeleteOn*Thread as traits. But >> > you can only use those traits from parts of the codebase that have >> > access to content/public; this could be an essential companion to >> > RefCountedThreadSafe everywhere else. >> > >> > > This is where I may appear to contradict myself perhaps. I don't think >> that >> making RefCountedThreadSafe things that can only be deleted on a single >> thread is a valid pattern. If it can't be deleted anywhere it's not >> RefCountedThreadSafe, I think these are incompatible contracts. OTOH what >> I'm asking here is to support that in BindToCurrentLoop because I know >> people do this sort of thing anyhow. But I don't want to encourage it with >> more helpers. >> > > I agree that RefCountedThreadSafe classes ought to be built so it's safe to > release references to them on any thread (even though that's not always > true > today). That's precisely why the content::BrowserThread::DeleteOnUIThread > etc > traits that you can pass as a template parameter to RefCountedThreadSafe > are so > useful - because they allow a RefCountedThreadSafe to run its destructor > on a > safe thread, without having to worry about what thread released the last > reference. > > > Maybe I should save DeleteOnCurrentLoop for a follow-up patch though, so >> > it can be discussed on its own (and only include internal::DeleteOnLoop >> > in this patch)? >> > >> > >> > >> > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... > >> > >> > The original code was written by jam/darin: >> > >> > https://codereview.chromium.org/338065 >> > >> > I'm guessing their reasoning is that we may as well delete it if we have >> > the chance, whereas if we delay it may no longer be possible to delete >> > it. >> > >> > I tried removing the conditional and always using DeleteSoon, but that >> > causes the following tests to fail: >> > NullVideoSinkTest.BasicFunctionality >> > VideoRendererImplTest.StartPlayingFrom_LowDelay >> > VideoRendererImplTest.Underflow >> > >> > These unit tests each have a MessageLoop owned by the test class. They >> > wait for some event on the message loop, then return, which causes the >> > test class to be destroyed. Hence the MessageLoop is destroyed, and >> > hence the DeleteOnLoop scoped_ptr to the callback ends up being >> > destructed. With the conditional this is fine -- it's already on the >> > right thread, so it goes ahead and synchronously deletes the callback. >> > But without the conditional, it tries to send a DeleteSoon to the >> > being-deleted message loop, which fails and our LOG(FATAL) causes the >> > test to crash. >> > > > Can we RunUntilIdle before exiting the test? >> > > No, I tried that :-| In fact, VideoRendererImplTest::Destroy() does that > already > at the end of each test before deleting the MessageLoop. > That's weird, why would it not run the DeleteSoon? To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Apr 30, 2015 at 9:15 AM, <johnme@chromium.org> wrote: > > https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... > media/base/bind_to_current_loop.h:117: auto cb_copy_ptr_ptr = new > scoped_ptr<base::Callback<void(A...)>, > On 2015/04/29 23:35:01, danakj wrote: > >> can we do this with a scoped_ptr Passed instead of a scoped_ptr* which >> > is making > >> this harder to understand. it'll make the callback only callable once, >> > but > >> that's maybe okay? >> > > Only allowing the callback to be called once breaks 57 tests in > media_unittests. Do these represent real uses of the callback in prod code, or test-only usage that we could change and maintain the same coverage? To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/04/30 17:13:10, danakj wrote: > On Thu, Apr 30, 2015 at 10:09 AM, <mailto:johnme@chromium.org> wrote: > > > On 2015/04/30 16:42:25, danakj wrote: > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... > > > >> > > >> > The original code was written by jam/darin: > >> > > >> > https://codereview.chromium.org/338065 > >> > > >> > I'm guessing their reasoning is that we may as well delete it if we have > >> > the chance, whereas if we delay it may no longer be possible to delete > >> > it. > >> > > >> > I tried removing the conditional and always using DeleteSoon, but that > >> > causes the following tests to fail: > >> > NullVideoSinkTest.BasicFunctionality > >> > VideoRendererImplTest.StartPlayingFrom_LowDelay > >> > VideoRendererImplTest.Underflow > >> > > >> > These unit tests each have a MessageLoop owned by the test class. They > >> > wait for some event on the message loop, then return, which causes the > >> > test class to be destroyed. Hence the MessageLoop is destroyed, and > >> > hence the DeleteOnLoop scoped_ptr to the callback ends up being > >> > destructed. With the conditional this is fine -- it's already on the > >> > right thread, so it goes ahead and synchronously deletes the callback. > >> > But without the conditional, it tries to send a DeleteSoon to the > >> > being-deleted message loop, which fails and our LOG(FATAL) causes the > >> > test to crash. > >> > > > > > > Can we RunUntilIdle before exiting the test? > >> > > > > No, I tried that :-| In fact, VideoRendererImplTest::Destroy() does that > > already > > at the end of each test before deleting the MessageLoop. > > > > That's weird, why would it not run the DeleteSoon? I'm not sure :-| On 2015/04/30 17:44:36, danakj wrote: > On Thu, Apr 30, 2015 at 9:15 AM, <mailto:johnme@chromium.org> wrote: > > > > > > https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... > > media/base/bind_to_current_loop.h:117: auto cb_copy_ptr_ptr = new > > scoped_ptr<base::Callback<void(A...)>, > > On 2015/04/29 23:35:01, danakj wrote: > > > >> can we do this with a scoped_ptr Passed instead of a scoped_ptr* which > >> > > is making > > > >> this harder to understand. it'll make the callback only callable once, > >> > > but > > > >> that's maybe okay? > >> > > > > Only allowing the callback to be called once breaks 57 tests in > > media_unittests. > > > Do these represent real uses of the callback in prod code, or test-only > usage that we could change and maintain the same coverage? Seems to be real uses of the callback in prod code. The failures occur in the following sets of tests: DecryptingAudioDecoderTest FFmpegAudioDecoderTest/AudioDecoderTest FFmpegVideoDecoderTest OpusAudioDecoderTest/AudioDecoderTest PipelineIntegrationTest None of those files, or the test/mock files they transitively include, make any reference to BindToCurrentLoop.
On Fri, May 1, 2015 at 11:18 AM, <johnme@chromium.org> wrote: > On 2015/04/30 17:13:10, danakj wrote: > >> On Thu, Apr 30, 2015 at 10:09 AM, <mailto:johnme@chromium.org> wrote: >> > > > On 2015/04/30 16:42:25, danakj wrote: >> > >> > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... > >> > >> >> > >> >> > The original code was written by jam/darin: >> >> > >> >> > https://codereview.chromium.org/338065 >> >> > >> >> > I'm guessing their reasoning is that we may as well delete it if we >> have >> >> > the chance, whereas if we delay it may no longer be possible to >> delete >> >> > it. >> >> > >> >> > I tried removing the conditional and always using DeleteSoon, but >> that >> >> > causes the following tests to fail: >> >> > NullVideoSinkTest.BasicFunctionality >> >> > VideoRendererImplTest.StartPlayingFrom_LowDelay >> >> > VideoRendererImplTest.Underflow >> >> > >> >> > These unit tests each have a MessageLoop owned by the test class. >> They >> >> > wait for some event on the message loop, then return, which causes >> the >> >> > test class to be destroyed. Hence the MessageLoop is destroyed, and >> >> > hence the DeleteOnLoop scoped_ptr to the callback ends up being >> >> > destructed. With the conditional this is fine -- it's already on the >> >> > right thread, so it goes ahead and synchronously deletes the >> callback. >> >> > But without the conditional, it tries to send a DeleteSoon to the >> >> > being-deleted message loop, which fails and our LOG(FATAL) causes the >> >> > test to crash. >> >> >> > >> > >> > Can we RunUntilIdle before exiting the test? >> >> >> > >> > No, I tried that :-| In fact, VideoRendererImplTest::Destroy() does that >> > already >> > at the end of each test before deleting the MessageLoop. >> > >> > > That's weird, why would it not run the DeleteSoon? >> > > I'm not sure :-| > Is it something you can figure out? I don't like blindly making changes. > > On 2015/04/30 17:44:36, danakj wrote: > > On Thu, Apr 30, 2015 at 9:15 AM, <mailto:johnme@chromium.org> wrote: >> > > > >> > >> > > > https://codereview.chromium.org/1082113004/diff/20001/media/base/bind_to_curr... > >> > media/base/bind_to_current_loop.h:117: auto cb_copy_ptr_ptr = new >> > scoped_ptr<base::Callback<void(A...)>, >> > On 2015/04/29 23:35:01, danakj wrote: >> > >> >> can we do this with a scoped_ptr Passed instead of a scoped_ptr* which >> >> >> > is making >> > >> >> this harder to understand. it'll make the callback only callable once, >> >> >> > but >> > >> >> that's maybe okay? >> >> >> > >> > Only allowing the callback to be called once breaks 57 tests in >> > media_unittests. >> > > > Do these represent real uses of the callback in prod code, or test-only >> usage that we could change and maintain the same coverage? >> > > Seems to be real uses of the callback in prod code. The failures occur in > the > following sets of tests: > > DecryptingAudioDecoderTest > FFmpegAudioDecoderTest/AudioDecoderTest > FFmpegVideoDecoderTest > OpusAudioDecoderTest/AudioDecoderTest > PipelineIntegrationTest > > None of those files, or the test/mock files they transitively include, > make any > reference to BindToCurrentLoop. > > https://codereview.chromium.org/1082113004/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/05/04 18:46:13, danakj wrote: > On Fri, May 1, 2015 at 11:18 AM, <mailto:johnme@chromium.org> wrote: > > > On 2015/04/30 17:13:10, danakj wrote: > > > >> On Thu, Apr 30, 2015 at 10:09 AM, <mailto:johnme@chromium.org> wrote: > >> > > > > > On 2015/04/30 16:42:25, danakj wrote: > >> > > >> > > > > > > > https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro... > > > >> > > >> >> > > >> >> > The original code was written by jam/darin: > >> >> > > >> >> > https://codereview.chromium.org/338065 > >> >> > > >> >> > I'm guessing their reasoning is that we may as well delete it if we > >> have > >> >> > the chance, whereas if we delay it may no longer be possible to > >> delete > >> >> > it. > >> >> > > >> >> > I tried removing the conditional and always using DeleteSoon, but > >> that > >> >> > causes the following tests to fail: > >> >> > NullVideoSinkTest.BasicFunctionality > >> >> > VideoRendererImplTest.StartPlayingFrom_LowDelay > >> >> > VideoRendererImplTest.Underflow > >> >> > > >> >> > These unit tests each have a MessageLoop owned by the test class. > >> They > >> >> > wait for some event on the message loop, then return, which causes > >> the > >> >> > test class to be destroyed. Hence the MessageLoop is destroyed, and > >> >> > hence the DeleteOnLoop scoped_ptr to the callback ends up being > >> >> > destructed. With the conditional this is fine -- it's already on the > >> >> > right thread, so it goes ahead and synchronously deletes the > >> callback. > >> >> > But without the conditional, it tries to send a DeleteSoon to the > >> >> > being-deleted message loop, which fails and our LOG(FATAL) causes the > >> >> > test to crash. > >> >> > >> > > >> > > >> > Can we RunUntilIdle before exiting the test? > >> >> > >> > > >> > No, I tried that :-| In fact, VideoRendererImplTest::Destroy() does that > >> > already > >> > at the end of each test before deleting the MessageLoop. > >> > > >> > > > > That's weird, why would it not run the DeleteSoon? > >> > > > > I'm not sure :-| > > > > Is it something you can figure out? I don't like blindly making changes. Ok, it turns out that all three tests define mock methods, e.g.: class NullVideoSinkTest : public testing::Test, public VideoRendererSink::RenderCallback { public: ... // VideoRendererSink::RenderCallback implementation. MOCK_METHOD2(Render, scoped_refptr<VideoFrame>(base::TimeTicks, base::TimeTicks)); MOCK_METHOD0(OnFrameDropped, void()); MOCK_METHOD1(FrameReceived, void(const scoped_refptr<VideoFrame>&)); protected: base::MessageLoop message_loop_; ... }; Then the tests have code like: TEST_F (NullVideoSinkTest, BasicFunctionality) { ... WaitableMessageLoopEvent event; EXPECT_CALL(*this, FrameReceived(test_frame)) .WillOnce(RunClosure(event.GetClosure())); event.RunAndWait(); ... } This is simply intended to spin a message loop until the mocked method is called, then continue. But WaitableMessageLoopEvent::GetClosure is implemented as: base::Closure WaitableMessageLoopEvent::GetClosure() { ... return BindToCurrentLoop(base::Bind( &WaitableMessageLoopEvent::OnCallback, base::Unretained(this), PIPELINE_OK)); } And, from debugging the lifetimes, it turns out that Google Mock's MOCK_METHODs hold on to the RunClosure callback until the MOCK_METHOD is destroyed (i.e. when NullVideoSinkTest or VideoRendererImplTest are destroyed). The MessageLoop member of both test classes happens to be destroyed before the MOCK_METHODs, hence when the MOCK_METHODs are destroyed, the callback returned by BindToCurrentLoop finally gets destroyed, and DeleteOnLoop::Destruct fails to call DeleteSoon because the MessageLoop no longer exists. A reduced test case for all this is: TEST_F(Foo, DestroyAfterMessageLoop) { base::Closure cb; { base::MessageLoop message_loop; cb = BindToCurrentLoop(base::Bind(&base::DoNothing)); } } which hits the same LOG(FATAL) due to DeleteSoon failing. It would be possible to fix these tests by ensuring the callback returned by BindToCurrentLoop is destroyed before the MessageLoop is; however (as well as the refactoring possibly being invasive), I'm not convinced there's actually anything wrong with tests like my reduced DestroyAfterMessageLoop above. It seems pretty reasonable to allow the thread that the callback is meant to be destroyed on to keep a reference whose lifetime exceeds that of its message loop, since the message loop won't be needed to destroy it as it's already on the right thread. Not allowing this will likely just waste developer time hunting down lifetime subtleties that only affect unit tests... |