Ensure FFmpegDemuxer WeakPtrs are created on the right thread.

media/filters/ffmpeg_demuxer.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "media/filters/ffmpeg_demuxer.h"
 
 #include <algorithm>
 #include <memory>
 #include <utility>
 
@@ skipping 842 matching lines @@
 std::string FFmpegDemuxer::GetDisplayName() const {
   return "FFmpegDemuxer";
 }
 
 void FFmpegDemuxer::Initialize(DemuxerHost* host,
                                const PipelineStatusCB& status_cb,
                                bool enable_text_tracks) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   host_ = host;
   text_enabled_ = enable_text_tracks;
+  weak_this_ = weak_factory_.GetWeakPtr();
 
   url_protocol_.reset(new BlockingUrlProtocol(
       data_source_,
       BindToCurrentLoop(base::Bind(&FFmpegDemuxer::OnDataSourceError,
                                    base::Unretained(this)))));
   glue_.reset(new FFmpegGlue(url_protocol_.get()));
   AVFormatContext* format_context = glue_->format_context();
 
   // Disable ID3v1 tag reading to avoid costly seeks to end of file for data we
   // don't use.  FFmpeg will only read ID3v1 tags if no other metadata is
@@ skipping 27 matching lines @@
 
   // Abort all outstanding reads.
   for (auto* stream : streams_) {
     if (stream)
       stream->Abort();
   }
 
   // It's important to invalidate read/seek completion callbacks to avoid any
   // errors that occur because of the data source abort.
   weak_factory_.InvalidateWeakPtrs();
+  weak_this_ = weak_factory_.GetWeakPtr();
   data_source_->Abort();
 
   // Aborting the read may cause EOF to be marked, undo this.
   blocking_thread_.task_runner()->PostTask(
       FROM_HERE, base::Bind(&UnmarkEndOfStream, glue_->format_context()));
   pending_read_ = false;
 
   // TODO(dalecurtis): We probably should report PIPELINE_ERROR_ABORT here
   // instead to avoid any preroll work that may be started upon return, but
   // currently the PipelineImpl does not know how to handle this.
@@ skipping 25 matching lines @@
   data_source_ = NULL;
 
   // Invalidate WeakPtrs on |task_runner_|, destruction may happen on another
   // thread.
   weak_factory_.InvalidateWeakPtrs();
 }
 
 void FFmpegDemuxer::StartWaitingForSeek(base::TimeDelta seek_time) {}
 
 void FFmpegDemuxer::CancelPendingSeek(base::TimeDelta seek_time) {
+  if (!blocking_thread_.IsRunning())

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

Pretty sure this isn't thread safe?

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

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

[watk, 2016/09/02 20:07:09]

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

+    return;
+
   if (task_runner_->BelongsToCurrentThread()) {
     AbortPendingReads();
   } else {
-    task_runner_->PostTask(FROM_HERE,
-                           base::Bind(&FFmpegDemuxer::AbortPendingReads,
-                                      weak_factory_.GetWeakPtr()));
+    // Don't use GetWeakPtr() here since we are on the wrong thread.
+    task_runner_->PostTask(
+        FROM_HERE, base::Bind(&FFmpegDemuxer::AbortPendingReads, weak_this_));

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

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]

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]

Oh ok, I was confused, thanks :)

   }
 }
 
 void FFmpegDemuxer::Seek(base::TimeDelta time, const PipelineStatusCB& cb) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   CHECK(pending_seek_cb_.is_null());
 
   // FFmpeg requires seeks to be adjusted according to the lowest starting time.
   // Since EnqueuePacket() rebased negative timestamps by the start time, we
   // must correct the shift here.
@@ skipping 781 matching lines @@
 
 void FFmpegDemuxer::SetLiveness(DemuxerStream::Liveness liveness) {
   DCHECK(task_runner_->BelongsToCurrentThread());
   for (auto* stream : streams_) {
     if (stream)
       stream->SetLiveness(liveness);
   }
 }
 
 }  // namespace media