Pipeline: Use WeakPtr for DemuxerHost Calls and Tasks.

Pipeline: Use WeakPtr for DemuxerHost Calls and Tasks.

There is a chance that the Demuxer calls host_->OnDemuxerError() right
before Stop() is called. The Pipeline always posts a ErrorChangedTask() for
OnDemuxerError() with base::Retained(this).

After the Demuxer fires the stop callback, the Pipeline could be destroyed
immediately. So If the media thread hasn't been destroyed we could end up with
running ErrorChangedTask() on null pipeline which causes a crash.

This CL uses a weak pointer for DemuxerHost calls so that no task will run
after the pipeline is destroyed.

BUG=397656, 399417, 365141
TEST=Updated unit tests to cover this case.
R=scherkus@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=287687

[xhwang, 2014-08-04 19:03:31]

This CL is related to the bug but I suspect it's not fixing it. The bug happened
on an ogv file so we should be using FFmpegDemuxer. I'll look at FFD separately.

The fix is a bit odd in the sense that ChunkDemuxer never posts tasks. But in
media code, we rely on the fact that callees post callbacks instead of running
them directly. I could BindToCurrentLoop(done_cb) in Pipeline::DoStop(), but
that sounds hackier.

PTAL.

[scherkus (not reviewing), 2014-08-04 19:55:44]

is there a reason why we can't use WeakPtr instead of Unretained inside the
Demuxer-callback methods in Pipeline?

would that be a better fix?

[xhwang, 2014-08-04 21:37:24]

On 2014/08/04 19:55:44, scherkus wrote:
> is there a reason why we can't use WeakPtr instead of Unretained inside the
> Demuxer-callback methods in Pipeline?
> 
> would that be a better fix?

Done. I completed changed this CL based on this, which does result in a much
better solution. PTAL again.

I could repro the crash with the updated DemuxerErrorDuringStop test, and
verified that the fix works with that test.

[xhwang, 2014-08-05 17:59:28]

scherkus: Just found that this CL should also fix another bug:
http://crbug.com/399417. So the priority of this CL gets a bit higher now. PTAL

[scherkus (not reviewing), 2014-08-05 18:11:30]

https://codereview.chromium.org/423073012/diff/40001/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/423073012/diff/40001/media/base/pipeline.h#ne...
media/base/pipeline.h:434: // weak_factory_.GetWeakPtr() to get weak pointers.
Instead, use a permanent
you sure about this?

I'm fairly certain GetWeakPtr() is OK to call on different threads, it's that
you cannot inspect/dereference the pointer on a different thread

[xhwang, 2014-08-05 20:44:47]

PTAL, I building asan and will test it more.

https://codereview.chromium.org/423073012/diff/40001/media/base/pipeline.h
File media/base/pipeline.h (right):

https://codereview.chromium.org/423073012/diff/40001/media/base/pipeline.h#ne...
media/base/pipeline.h:434: // weak_factory_.GetWeakPtr() to get weak pointers.
Instead, use a permanent
On 2014/08/05 18:11:30, scherkus wrote:
> you sure about this?
> 
> I'm fairly certain GetWeakPtr() is OK to call on different threads, it's that
> you cannot inspect/dereference the pointer on a different thread 

Cool, I didn't know that. Double checked weak_ptr.h and you are right. This
actually makes the CL much simpler.

https://codereview.chromium.org/423073012/diff/60001/media/base/pipeline.cc
File media/base/pipeline.cc (right):

https://codereview.chromium.org/423073012/diff/60001/media/base/pipeline.cc#n...
media/base/pipeline.cc:553: weak_factory_.InvalidateWeakPtrs();
Move this down here because OnStopComplete() can also be triggered by error.

[Ken Russell (switch to Gerrit), 2014-08-05 22:32:09]

Thanks for putting this together and sorry about the win_gpu_triggered_tests
flake. I'm not sure what happened there and have resubmitted the try job to
win_gpu.

Is it possible for this fix to land today? This bot is flaking very often with
this failure:

http://build.chromium.org/p/chromium.gpu/builders/Win7%20Release%20(ATI)

We could almost verify this fix by ensuring this bot goes reliably green --
though of course it would be better to have a more targeted test to avoid
regressions in the future.

[scherkus (not reviewing), 2014-08-05 23:56:58]

lgtm

[scherkus (not reviewing), 2014-08-05 23:57:06]

The CQ bit was checked by scherkus@chromium.org

[commit-bot: I haz the power, 2014-08-05 23:58:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/xhwang@chromium.org/423073012/60001

[commit-bot: I haz the power, 2014-08-06 01:13:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_gpu_triggered_tests on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu_triggered...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)
  win_chromium_x64_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Ken Russell (switch to Gerrit), 2014-08-06 01:24:32]

There are some infrastructure issues affecting several bots right now. Please
keep retrying this CL. Thanks.

[xhwang, 2014-08-06 05:49:04]

Committed patchset #4 manually as 287687 (presubmit successful).