media/gpu/test: Fix potential thread-safety issues in video decoder tests.

This CL fixes some thread-safety issues in our video decoder tests. These might
be responsible for some rare crashes we've been seeing when running the tests on
our test infrastructure, but are not reproducible locally.

Most callbacks will be triggered on the same thread that scheduled these, but
this is not always the case. However WeakPtrs are not thread-safe, so this CL
introduces functors that guarantee that WeakPtrs are only dereferenced on the
thread that created them.

This CL also adds an additional sequence check to make sure events are sent on
the correct thread, and a comment is added about the use of base::Unretained for
the video player event callback.

TEST=./video_decode_accelerator_tests on samus

BUG=981718

Change-Id: If04268953f9be5dabc4ec11759fb3223f45055cd
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1843998
Commit-Queue: David Staessens <dstaessens@chromium.org>
Reviewed-by: Chih-Yu Huang <akahuang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704467}

media/gpu/test/video_player/test_vda_video_decoder.cc

@@ -12,7 +12,6 @@
 #include "base/task/post_task.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
-#include "media/base/bind_to_current_loop.h"
 #include "media/base/video_frame.h"
 #include "media/base/waiting.h"
 #include "media/gpu/gpu_video_decode_accelerator_factory.h"
@@ -47,12 +46,14 @@ TestVDAVideoDecoder::TestVDAVideoDecoder(
       gpu_memory_buffer_factory_(gpu_memory_buffer_factory),
 #endif  // BUILDFLAG(USE_CHROMEOS_MEDIA_ACCELERATION)
       decode_start_timestamps_(kTimestampCacheSize) {
-  DETACH_FROM_SEQUENCE(vda_wrapper_sequence_checker_);
+  DCHECK_CALLED_ON_VALID_SEQUENCE(vda_wrapper_sequence_checker_);
 
   // TODO(crbug.com/933632) Remove support for allocate mode, and always use
   // import mode. Support for allocate mode is temporary maintained for older
   // platforms that don't support import mode.
 
+  vda_wrapper_task_runner_ = base::ThreadTaskRunnerHandle::Get();
+
   weak_this_ = weak_this_factory_.GetWeakPtr();
 }
 
@@ -268,6 +269,8 @@ void TestVDAVideoDecoder::ProvidePictureBuffersWithVisibleRect(
 }
 
 void TestVDAVideoDecoder::DismissPictureBuffer(int32_t picture_buffer_id) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(vda_wrapper_sequence_checker_);
+
   // Drop reference to the video frame associated with the picture buffer, so
   // the video frame and related texture are automatically destroyed once the
   // renderer and video frame processors are done using them.
@@ -321,13 +324,24 @@ void TestVDAVideoDecoder::PictureReady(const Picture& picture) {
   // |video_frames_| to map between picture buffers and frames, but that
   // reference will be released when the decoder calls DismissPictureBuffer()
   // (e.g. on a resolution change).
-  base::OnceClosure reuse_cb = BindToCurrentLoop(
-      base::BindOnce(&TestVDAVideoDecoder::ReusePictureBufferTask, weak_this_,
-                     picture.picture_buffer_id()));
+  base::OnceClosure reuse_cb =
+      base::BindOnce(&TestVDAVideoDecoder::ReusePictureBufferThunk, weak_this_,
+                     vda_wrapper_task_runner_, picture.picture_buffer_id());
   wrapped_video_frame->AddDestructionObserver(std::move(reuse_cb));
   output_cb_.Run(std::move(wrapped_video_frame));
 }
 
+// static
+void TestVDAVideoDecoder::ReusePictureBufferThunk(
+    base::Optional<base::WeakPtr<TestVDAVideoDecoder>> vda_video_decoder,
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    int32_t picture_buffer_id) {
+  DCHECK(vda_video_decoder);
+  task_runner->PostTask(
+      FROM_HERE, base::BindOnce(&TestVDAVideoDecoder::ReusePictureBufferTask,
+                                *vda_video_decoder, picture_buffer_id));
+}
+
 // Called when a picture buffer is ready to be re-used.
 void TestVDAVideoDecoder::ReusePictureBufferTask(int32_t picture_buffer_id) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(vda_wrapper_sequence_checker_);

media/gpu/test/video_player/test_vda_video_decoder.h

@@ -80,6 +80,12 @@ class TestVDAVideoDecoder : public media::VideoDecoder,
   void NotifyResetDone() override;
   void NotifyError(VideoDecodeAccelerator::Error error) override;
 
+  // Helper thunk to avoid dereferencing WeakPtrs on the wrong thread.
+  static void ReusePictureBufferThunk(
+      base::Optional<base::WeakPtr<TestVDAVideoDecoder>> decoder_client,
+      scoped_refptr<base::SequencedTaskRunner> task_runner,
+      int32_t picture_buffer_id);
+
   // Get the next bitstream buffer id to be used.
   int32_t GetNextBitstreamBufferId();
   // Get the next picture buffer id to be used.
@@ -118,6 +124,8 @@ class TestVDAVideoDecoder : public media::VideoDecoder,
 
   std::unique_ptr<VideoDecodeAccelerator> decoder_;
 
+  scoped_refptr<base::SequencedTaskRunner> vda_wrapper_task_runner_;
+
   SEQUENCE_CHECKER(vda_wrapper_sequence_checker_);
 
   base::WeakPtr<TestVDAVideoDecoder> weak_this_;

media/gpu/test/video_player/video_decoder_client.cc

@@ -11,7 +11,6 @@
 #include "base/memory/ptr_util.h"
 #include "base/threading/thread_task_runner_handle.h"
 #include "build/build_config.h"
-#include "media/base/bind_to_current_loop.h"
 #include "media/base/waiting.h"
 #include "media/gpu/buildflags.h"
 #include "media/gpu/macros.h"
@@ -32,6 +31,21 @@
 namespace media {
 namespace test {
 
+namespace {
+// Callbacks can be called from any thread, but WeakPtrs are not thread-safe.
+// This helper thunk wraps a WeakPtr into an 'Optional' value, so the WeakPtr is
+// only dereferenced after rescheduling the task on the specified task runner.
+template <typename F, typename... Args>
+void CallbackThunk(
+    base::Optional<base::WeakPtr<VideoDecoderClient>> decoder_client,
+    scoped_refptr<base::SequencedTaskRunner> task_runner,
+    F f,
+    Args... args) {
+  DCHECK(decoder_client);
+  task_runner->PostTask(FROM_HERE, base::BindOnce(f, *decoder_client, args...));
+}
+}  // namespace
+
 VideoDecoderClient::VideoDecoderClient(
     const VideoPlayer::EventCallback& event_cb,
     gpu::GpuMemoryBufferFactory* gpu_memory_buffer_factory,
@@ -208,10 +222,16 @@ void VideoDecoderClient::InitializeDecoderTask(const Video* video,
       kNoTransformation, video_->Resolution(), gfx::Rect(video_->Resolution()),
       video_->Resolution(), std::vector<uint8_t>(0), EncryptionScheme());
 
-  VideoDecoder::InitCB init_cb = BindToCurrentLoop(
-      base::BindOnce(&VideoDecoderClient::DecoderInitializedTask, weak_this_));
-  VideoDecoder::OutputCB output_cb = BindToCurrentLoop(
-      base::BindRepeating(&VideoDecoderClient::FrameReadyTask, weak_this_));
+  VideoDecoder::InitCB init_cb = base::BindOnce(
+      CallbackThunk<decltype(&VideoDecoderClient::DecoderInitializedTask),
+                    bool>,
+      weak_this_, decoder_client_thread_.task_runner(),
+      &VideoDecoderClient::DecoderInitializedTask);
+  VideoDecoder::OutputCB output_cb = base::BindRepeating(
+      CallbackThunk<decltype(&VideoDecoderClient::FrameReadyTask),
+                    scoped_refptr<VideoFrame>>,
+      weak_this_, decoder_client_thread_.task_runner(),
+      &VideoDecoderClient::FrameReadyTask);
 
   decoder_->Initialize(config, false, nullptr, std::move(init_cb), output_cb,
                        WaitingCB());
@@ -283,8 +303,11 @@ void VideoDecoderClient::DecodeNextFragmentTask() {
       reinterpret_cast<const uint8_t*>(fragment_bytes.data()), fragment_size);
   bitstream_buffer->set_timestamp(base::TimeTicks::Now().since_origin());
 
-  VideoDecoder::DecodeCB decode_cb = BindToCurrentLoop(
-      base::BindOnce(&VideoDecoderClient::DecodeDoneTask, weak_this_));
+  VideoDecoder::DecodeCB decode_cb = base::BindOnce(
+      CallbackThunk<decltype(&VideoDecoderClient::DecodeDoneTask),
+                    media::DecodeStatus>,
+      weak_this_, decoder_client_thread_.task_runner(),
+      &VideoDecoderClient::DecodeDoneTask);
   decoder_->Decode(std::move(bitstream_buffer), std::move(decode_cb));
 
   num_outstanding_decode_requests_++;
@@ -304,8 +327,11 @@ void VideoDecoderClient::FlushTask() {
   // Changing the state to flushing will abort any pending decodes.
   decoder_client_state_ = VideoDecoderClientState::kFlushing;
 
-  VideoDecoder::DecodeCB flush_done_cb = BindToCurrentLoop(
-      base::BindOnce(&VideoDecoderClient::FlushDoneTask, weak_this_));
+  VideoDecoder::DecodeCB flush_done_cb =
+      base::BindOnce(CallbackThunk<decltype(&VideoDecoderClient::FlushDoneTask),
+                                   media::DecodeStatus>,
+                     weak_this_, decoder_client_thread_.task_runner(),
+                     &VideoDecoderClient::FlushDoneTask);
   decoder_->Decode(DecoderBuffer::CreateEOSBuffer(), std::move(flush_done_cb));
 
   FireEvent(VideoPlayerEvent::kFlushing);
@@ -319,8 +345,10 @@ void VideoDecoderClient::ResetTask() {
   decoder_client_state_ = VideoDecoderClientState::kResetting;
   // TODO(dstaessens@) Allow resetting to any point in the stream.
   encoded_data_helper_->Rewind();
-  base::RepeatingClosure reset_done_cb = BindToCurrentLoop(
-      base::BindRepeating(&VideoDecoderClient::ResetDoneTask, weak_this_));
+
+  base::RepeatingClosure reset_done_cb = base::BindRepeating(
+      CallbackThunk<decltype(&VideoDecoderClient::ResetDoneTask)>, weak_this_,
+      decoder_client_thread_.task_runner(), &VideoDecoderClient::ResetDoneTask);
   decoder_->Reset(reset_done_cb);
   FireEvent(VideoPlayerEvent::kResetting);
 }
@@ -393,6 +421,8 @@ void VideoDecoderClient::ResetDoneTask() {
 }
 
 void VideoDecoderClient::FireEvent(VideoPlayerEvent event) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(decoder_client_sequence_checker_);
+
   bool continue_decoding = event_cb_.Run(event);
   if (!continue_decoding) {
     // Changing the state to idle will abort any pending decodes.

media/gpu/test/video_player/video_player.cc

@@ -54,6 +54,9 @@ bool VideoPlayer::CreateDecoderClient(
   CHECK(frame_renderer);
   DVLOGF(4);
 
+  // base::Unretained is safe here as we will never receive callbacks after
+  // destroying the video player, since the video decoder client will be
+  // destroyed first.
   EventCallback event_cb =
       base::BindRepeating(&VideoPlayer::NotifyEvent, base::Unretained(this));