Release ContextProviderCommandBuffer reference on media thread

GpuVideoAcceleratorFactoriesImpl calls BindToCurrentThread() on media task
runner, so releasing the reference should also happen there to avoid race
conditions. crrev.com/c/959432 introduced the regression, since we took
care of initialization, but we didn't take care of destruction.

Bug: 826539, 580386
Change-Id: I2fff505b445bca7ff38343f6f1e5d3035a06714e
Reviewed-on: https://chromium-review.googlesource.com/984438Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Daniele Castagna <dcastagna@chromium.org>
Commit-Queue: Emircan Uysaler <emircan@chromium.org>
Cr-Commit-Position: refs/heads/master@{#546725}

content/renderer/media/gpu/gpu_video_accelerator_factories_impl.cc

@@ -78,8 +78,8 @@ GpuVideoAcceleratorFactoriesImpl::GpuVideoAcceleratorFactoriesImpl(
     : main_thread_task_runner_(main_thread_task_runner),
       task_runner_(task_runner),
       gpu_channel_host_(std::move(gpu_channel_host)),
-      context_provider_refptr_(context_provider),
-      context_provider_(context_provider.get()),
+      context_provider_(context_provider),
+      context_provider_lost_(false),
       enable_gpu_memory_buffer_video_frames_(
           enable_gpu_memory_buffer_video_frames),
       video_accelerator_enabled_(enable_video_accelerator),
@@ -108,32 +108,16 @@ void GpuVideoAcceleratorFactoriesImpl::BindContextToTaskRunner() {
   DCHECK(context_provider_);
   if (context_provider_->BindToCurrentThread() !=
       gpu::ContextResult::kSuccess) {
-    context_provider_ = nullptr;
-    main_thread_task_runner_->PostTask(
-        FROM_HERE,
-        base::BindOnce(
-            &GpuVideoAcceleratorFactoriesImpl::ReleaseContextProvider,
-            base::Unretained(this)));
+    ReleaseContextProvider();
   }
 }
 
 bool GpuVideoAcceleratorFactoriesImpl::CheckContextLost() {
   DCHECK(task_runner_->BelongsToCurrentThread());
   if (context_provider_) {
-    bool release_context_provider = false;
     if (context_provider_->ContextGL()->GetGraphicsResetStatusKHR() !=
         GL_NO_ERROR) {
-      context_provider_ = nullptr;
-      release_context_provider = true;
-    }
-
-    if (release_context_provider) {
-      // Drop the reference on the main thread.
-      main_thread_task_runner_->PostTask(
-          FROM_HERE,
-          base::BindOnce(
-              &GpuVideoAcceleratorFactoriesImpl::ReleaseContextProvider,
-              base::Unretained(this)));
+      ReleaseContextProvider();
     }
   }
   return !context_provider_;
@@ -372,7 +356,7 @@ GpuVideoAcceleratorFactoriesImpl::GetVideoEncodeAcceleratorSupportedProfiles() {
 
 viz::ContextProvider*
 GpuVideoAcceleratorFactoriesImpl::GetMediaContextProvider() {
-  return CheckContextLost() ? nullptr : context_provider_;
+  return CheckContextLost() ? nullptr : context_provider_.get();
 }
 
 void GpuVideoAcceleratorFactoriesImpl::SetRenderingColorSpace(
@@ -380,17 +364,9 @@ void GpuVideoAcceleratorFactoriesImpl::SetRenderingColorSpace(
   rendering_color_space_ = color_space;
 }
 
-void GpuVideoAcceleratorFactoriesImpl::ReleaseContextProvider() {
+bool GpuVideoAcceleratorFactoriesImpl::CheckContextProviderLost() {
   DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
-  RecordContextProviderPhaseUmaEnum(
-      ContextProviderPhase::CONTEXT_PROVIDER_RELEASED);
-  context_provider_refptr_ = nullptr;
-}
-
-scoped_refptr<ui::ContextProviderCommandBuffer>
-GpuVideoAcceleratorFactoriesImpl::ContextProviderMainThread() {
-  DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
-  return context_provider_refptr_;
+  return context_provider_lost_;
 }
 
 void GpuVideoAcceleratorFactoriesImpl::
@@ -402,4 +378,22 @@ void GpuVideoAcceleratorFactoriesImpl::
   vea_provider_.Bind(std::move(unbound_vea_provider));
 }
 
+void GpuVideoAcceleratorFactoriesImpl::ReleaseContextProvider() {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+
+  main_thread_task_runner_->PostTask(
+      FROM_HERE,
+      base::BindOnce(&GpuVideoAcceleratorFactoriesImpl::SetContextProviderLost,
+                     base::Unretained(this)));
+
+  context_provider_ = nullptr;
+  RecordContextProviderPhaseUmaEnum(
+      ContextProviderPhase::CONTEXT_PROVIDER_RELEASED);
+}
+
+void GpuVideoAcceleratorFactoriesImpl::SetContextProviderLost() {
+  DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
+  context_provider_lost_ = true;
+}
+
 }  // namespace content

content/renderer/media/gpu/gpu_video_accelerator_factories_impl.h

@@ -107,8 +107,7 @@ class CONTENT_EXPORT GpuVideoAcceleratorFactoriesImpl
 
   void SetRenderingColorSpace(const gfx::ColorSpace& color_space) override;
 
-  void ReleaseContextProvider();
-  scoped_refptr<ui::ContextProviderCommandBuffer> ContextProviderMainThread();
+  bool CheckContextProviderLost();
 
   ~GpuVideoAcceleratorFactoriesImpl() override;
 
@@ -126,16 +125,19 @@ class CONTENT_EXPORT GpuVideoAcceleratorFactoriesImpl
   void BindVideoEncodeAcceleratorProviderOnTaskRunner(
       media::mojom::VideoEncodeAcceleratorProviderPtrInfo unbound_vea_provider);
 
+  void ReleaseContextProvider();
+  void SetContextProviderLost();
+
   const scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner_;
   const scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
   const scoped_refptr<gpu::GpuChannelHost> gpu_channel_host_;
 
-  // Shared pointer to a shared context provider that should be accessed
-  // and set only on the main thread.
-  scoped_refptr<ui::ContextProviderCommandBuffer> context_provider_refptr_;
-
-  // Raw pointer to a context provider accessed from the media thread.
-  ui::ContextProviderCommandBuffer* context_provider_;
+  // Shared pointer to a shared context provider. It is initially set on main
+  // thread, but all subsequent access and destruction should happen only on the
+  // media thread.
+  scoped_refptr<ui::ContextProviderCommandBuffer> context_provider_;
+  // Signals if |context_provider_| is alive on the media thread.
+  bool context_provider_lost_;
 
   base::UnguessableToken channel_token_;
 

content/renderer/render_thread_impl.cc

@@ -1536,7 +1536,7 @@ media::GpuVideoAcceleratorFactories* RenderThreadImpl::GetGpuFactories() {
   DCHECK(IsMainThread());
 
   if (!gpu_factories_.empty()) {
-    if (gpu_factories_.back()->ContextProviderMainThread())
+    if (!gpu_factories_.back()->CheckContextProviderLost())
       return gpu_factories_.back().get();
 
     GetMediaThreadTaskRunner()->PostTask(