Fix errors in destruction sequence of GpuVideoEncodeAccelerator::OnWillDestroyStub()

media/gpu/ipc/service/gpu_video_encode_accelerator.cc

 // Copyright 2013 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/gpu/ipc/service/gpu_video_encode_accelerator.h"
 
 #include <memory>
 
 #include "base/callback.h"
 #include "base/command_line.h"
@@ skipping 164 matching lines @@
       weak_this_factory_(this) {
   stub_->AddDestructionObserver(this);
   make_context_current_ =
       base::Bind(&MakeDecoderContextCurrent, stub_->AsWeakPtr());
 }
 
 GpuVideoEncodeAccelerator::~GpuVideoEncodeAccelerator() {
   // This class can only be self-deleted from OnWillDestroyStub(), which means
   // the VEA has already been destroyed in there.
   DCHECK(!encoder_);
-  if (encoder_worker_thread_.IsRunning()) {
-    encoder_worker_task_runner_->PostTask(
-        FROM_HERE,
-        base::Bind(&GpuVideoEncodeAccelerator::DestroyOnEncoderWorker,
-                   weak_this_factory_for_encoder_worker_.GetWeakPtr()));
-    encoder_worker_thread_.Stop();
-  }
+  DCHECK(!encoder_worker_thread_.IsRunning());
 }
 
 bool GpuVideoEncodeAccelerator::Initialize(VideoPixelFormat input_format,
                                            const gfx::Size& input_visible_size,
                                            VideoCodecProfile output_profile,
                                            uint32_t initial_bitrate) {
   DCHECK(main_task_runner_->BelongsToCurrentThread());
   DVLOG(1) << __func__
            << " input_format=" << VideoPixelFormatToString(input_format)
            << ", input_visible_size=" << input_visible_size.ToString()
@@ skipping 116 matching lines @@
   // removed however, since we cannot service incoming messages with VEA gone.
   // We cannot simply check for existence of VEA on IO thread though, because
   // we don't want to synchronize the IO thread with the ChildThread.
   // So we have to wait for the RemoveFilter callback here instead and remove
   // the VEA after it arrives and before returning.
   if (filter_) {
     stub_->channel()->RemoveFilter(filter_.get());
     filter_removed_.Wait();
   }
 
+  // We should stop |encoder_worker_thread_| before releasing |encoder_|, see
+  // crbug.com/715759.
+  if (encoder_worker_thread_.IsRunning()) {
+    encoder_worker_task_runner_->PostTask(
+        FROM_HERE,
+        base::Bind(&GpuVideoEncodeAccelerator::DestroyOnEncoderWorker,
+                   weak_this_factory_for_encoder_worker_.GetWeakPtr()));

[sandersd (OOO until July 31), 2017/04/28 00:47:44]

This WeakPtrFactory should probably not be vending outside of the worker thread.
It seems that it should be possible to create a weak pointer at worker thread
creation time and then just use copies of that?

I'm not entirely sure what's going on with |weak_this_factory_|. It is used for
posting to the main task runner but then is invalidated on the IO task runner?

[emircan, 2017/04/28 01:11:09]

Sure. I changed it such that we use copies of the same WeakPtr.
There is actually 2 WeakPtrFactories here:
- |weak_this_factory_for_encoder_worker_| to post on |encoder_worker_thread_|.
It is also invalidated there.
- |weak_this_factory_| to post either main or io thread, based on |filter_|. The
choice is then stored as |encode_task_runner_| where this is posted. It is
invalidated on io if there is |filter_|, or on main(dtor) otherwise.

[sandersd (OOO until July 31), 2017/04/28 18:22:44]

Please document this in the header file, the current comment about VideoFrames
is not as clear.

[emircan, 2017/04/29 00:10:46]

Done.

+    encoder_worker_thread_.Stop();
+  }
+
   stub_->channel()->RemoveRoute(host_route_id_);
   stub_->RemoveDestructionObserver(this);
   encoder_.reset();
   delete this;
 }
 
 // static
 gpu::VideoEncodeAcceleratorSupportedProfiles
 GpuVideoEncodeAccelerator::GetSupportedProfiles(
     const gpu::GpuPreferences& gpu_preferences) {
@@ skipping 172 matching lines @@
   weak_this_factory_for_encoder_worker_.InvalidateWeakPtrs();
 }
 
 void GpuVideoEncodeAccelerator::OnEncodeFrameCreated(
     int32_t frame_id,
     bool force_keyframe,
     const scoped_refptr<media::VideoFrame>& frame) {
   DVLOG(3) << __func__;
   DCHECK(CheckIfCalledOnCorrectThread());
 
+  if (filter_removed_.IsSignaled())
+    return;
+
   if (!frame) {
     DLOG(ERROR) << __func__ << " could not create a frame";
     NotifyError(VideoEncodeAccelerator::kPlatformFailureError);
     return;
   }
 
   frame->AddDestructionObserver(BindToCurrentLoop(
       base::Bind(&GpuVideoEncodeAccelerator::EncodeFrameFinished,
                  weak_this_factory_.GetWeakPtr(), frame_id)));
   encoder_->Encode(frame, force_keyframe);
 }
 
 void GpuVideoEncodeAccelerator::EncodeFrameFinished(int32_t frame_id) {
   DCHECK(CheckIfCalledOnCorrectThread());
   if (!Send(new AcceleratedVideoEncoderHostMsg_NotifyInputDone(host_route_id_,
                                                                frame_id))) {
     DLOG(ERROR) << __func__ << " failed.";
   }
 }
 
 bool GpuVideoEncodeAccelerator::CheckIfCalledOnCorrectThread() {
   return (filter_ && io_task_runner_->BelongsToCurrentThread()) ||
          (!filter_ && main_task_runner_->BelongsToCurrentThread());
 }
 
 }  // namespace media