Ensure FFmpegDemuxer WeakPtrs are created on the right thread.

Ensure FFmpegDemuxer WeakPtrs are created on the right thread.

CancelPendingSeek() was called from the render thread, but the
WeakFactory in FFmpegDemuxer is for use on the media thread.

BUG=643441
TEST=none

Committed: https://crrev.com/a6912725c190eebf472d78a44d8fd247751e9a07
Cr-Commit-Position: refs/heads/master@{#416426}

[DaleCurtis, 2016-09-01 23:52:18]

dalecurtis@chromium.org changed reviewers:
+ watk@chromium.org

[watk, 2016-09-02 00:21:20]

https://codereview.chromium.org/2305923002/diff/1/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/2305923002/diff/1/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:957: if (!blocking_thread_.IsRunning())
Pretty sure this isn't thread safe?

https://codereview.chromium.org/2305923002/diff/1/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:965: FROM_HERE,
base::Bind(&FFmpegDemuxer::AbortPendingReads, weak_this_));
Can you tell me why this pattern is safe? I see it a lot but I don't get it.
Can't we be writing to weak_this_ on another thread (l.911) while calling its
copy constructor here?

[DaleCurtis, 2016-09-02 00:32:47]

https://codereview.chromium.org/2305923002/diff/1/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/2305923002/diff/1/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:957: if (!blocking_thread_.IsRunning())
On 2016/09/02 at 00:21:20, watk wrote:
> Pretty sure this isn't thread safe?

Why do you think not? IsRunning() is thread safe. Removed in any case since it's
unnecessary.

https://codereview.chromium.org/2305923002/diff/1/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:965: FROM_HERE,
base::Bind(&FFmpegDemuxer::AbortPendingReads, weak_this_));
On 2016/09/02 at 00:21:20, watk wrote:
> Can you tell me why this pattern is safe? I see it a lot but I don't get it.
Can't we be writing to weak_this_ on another thread (l.911) while calling its
copy constructor here?

This particular pattern is wrong since we invalidate the WeakPtrs in two places.
I've fixed this by adding a second WeakPtr factory just for cancel pending seek.
It uses the pattern I think you're referring to now. Where we bind the WeakPtr
on the right thread and pass it from the wrong thread.

[watk, 2016-09-02 20:07:10]

lgtm

https://codereview.chromium.org/2305923002/diff/1/media/filters/ffmpeg_demuxe...
File media/filters/ffmpeg_demuxer.cc (right):

https://codereview.chromium.org/2305923002/diff/1/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:957: if (!blocking_thread_.IsRunning())
On 2016/09/02 00:32:47, DaleCurtis wrote:
> On 2016/09/02 at 00:21:20, watk wrote:
> > Pretty sure this isn't thread safe?
> 
> Why do you think not? IsRunning() is thread safe. Removed in any case since
it's
> unnecessary.

Ah, just because the docs in base::Thread say its not threadsafe unless
otherwise noted.

https://codereview.chromium.org/2305923002/diff/1/media/filters/ffmpeg_demuxe...
media/filters/ffmpeg_demuxer.cc:965: FROM_HERE,
base::Bind(&FFmpegDemuxer::AbortPendingReads, weak_this_));
On 2016/09/02 00:32:47, DaleCurtis wrote:
> On 2016/09/02 at 00:21:20, watk wrote:
> > Can you tell me why this pattern is safe? I see it a lot but I don't get it.
> Can't we be writing to weak_this_ on another thread (l.911) while calling its
> copy constructor here?
> 
> This particular pattern is wrong since we invalidate the WeakPtrs in two
places.
> I've fixed this by adding a second WeakPtr factory just for cancel pending
seek.
> It uses the pattern I think you're referring to now. Where we bind the WeakPtr
> on the right thread and pass it from the wrong thread.

Oh ok, I was confused, thanks :)

[watk, 2016-09-02 20:07:39]

The CQ bit was checked by watk@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-02 20:08:20]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-02 23:46:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-02 23:46:08]

Dry run: This issue passed the CQ dry run.

[DaleCurtis, 2016-09-03 02:26:01]

The CQ bit was checked by dalecurtis@chromium.org

[commit-bot: I haz the power, 2016-09-03 02:26:24]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-03 02:30:56]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-03 02:33:52]

Description was changed from

==========
Ensure FFmpegDemuxer WeakPtrs are created on the right thread.

CancelPendingSeek() was called from the render thread, but the
WeakFactory in FFmpegDemuxer is for use on the media thread.

BUG=643441
TEST=none
==========

to

==========
Ensure FFmpegDemuxer WeakPtrs are created on the right thread.

CancelPendingSeek() was called from the render thread, but the
WeakFactory in FFmpegDemuxer is for use on the media thread.

BUG=643441
TEST=none

Committed: https://crrev.com/a6912725c190eebf472d78a44d8fd247751e9a07
Cr-Commit-Position: refs/heads/master@{#416426}
==========

[commit-bot: I haz the power, 2016-09-03 02:33:53]

Patchset 2 (id:??) landed as
https://crrev.com/a6912725c190eebf472d78a44d8fd247751e9a07
Cr-Commit-Position: refs/heads/master@{#416426}