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

media/gpu/ipc/service/gpu_video_encode_accelerator.cc

Index: media/gpu/ipc/service/gpu_video_encode_accelerator.cc
diff --git a/media/gpu/ipc/service/gpu_video_encode_accelerator.cc b/media/gpu/ipc/service/gpu_video_encode_accelerator.cc
index 533e2cd95de4ed3640239454d8f7c89de8e65849..f51855b46e2f3e965935297a0098d4b41bdccc57 100644
--- a/media/gpu/ipc/service/gpu_video_encode_accelerator.cc
+++ b/media/gpu/ipc/service/gpu_video_encode_accelerator.cc
@@ -182,13 +182,7 @@ 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,
@@ -325,6 +319,16 @@ void GpuVideoEncodeAccelerator::OnWillDestroyStub() {
     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();
@@ -517,6 +521,9 @@ void GpuVideoEncodeAccelerator::OnEncodeFrameCreated(
   DVLOG(3) << __func__;
   DCHECK(CheckIfCalledOnCorrectThread());
 
+  if (filter_removed_.IsSignaled())
+    return;
+
   if (!frame) {
     DLOG(ERROR) << __func__ << " could not create a frame";
     NotifyError(VideoEncodeAccelerator::kPlatformFailureError);