BindToCurrentLoop should delete callback on original thread

BindToCurrentLoop 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

[johnme, 2015-04-24 18:35:58]

johnme@chromium.org changed reviewers:
+ danakj@chromium.org, xhwang@chromium.org

[johnme, 2015-04-24 18:35:58]

danakj: please review patch (hopefully it addresses your comments in
https://codereview.chromium.org/1083883003)

xhwang: please give owners review

Thanks!

[danakj, 2015-04-24 19:00:03]

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.

[xhwang, 2015-04-27 17:56:42]

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!

[johnme, 2015-04-27 18:32:33]

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?

[danakj, 2015-04-29 20:42:54]

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.

[johnme, 2015-04-29 21:18:46]

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.

[danakj, 2015-04-29 21:22:44]

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.

[danakj, 2015-04-29 23:35:02]

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()?

[danakj, 2015-04-29 23:37:34]

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

[johnme, 2015-04-30 16:15:53]

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).

[danakj, 2015-04-30 16:42:25]

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.

[johnme, 2015-04-30 17:09:12]

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.

[danakj, 2015-04-30 17:13:10]

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.

[danakj, 2015-04-30 17:44:36]

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.

[johnme, 2015-05-01 18:18:09]

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.

[danakj, 2015-05-04 18:46:13]

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.

[johnme, 2015-05-05 16:01:54]

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...