Do not delete the media ContextProvider inside the lost context callback

Currently the GpuVideoAcceleratorFactoriesImpl will check for context
loss every time it tries to use the GL context, and destroy it
immediately if it sees a loss. This is problematic if the
ContextProvider is still on the callstack, which it is if we're inside
the context loss callback. Instead, leave the ContextProvider alive
until the GpuVideoAcceleratorFactoriesImpl stops being used.

R=liberato@chromium.org

Bug: 849131
Change-Id: I8ec66671b4bbf5d426cb5fb184653a5863de9fd9
Reviewed-on: https://chromium-review.googlesource.com/1104777
Commit-Queue: danakj <danakj@chromium.org>
Reviewed-by: Frank Liberato <liberato@chromium.org>
Reviewed-by: Antoine Labour <piman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#568256}

content/renderer/media/gpu/gpu_video_accelerator_factories_impl.cc

@@ -82,7 +82,6 @@ GpuVideoAcceleratorFactoriesImpl::GpuVideoAcceleratorFactoriesImpl(
       task_runner_(task_runner),
       gpu_channel_host_(std::move(gpu_channel_host)),
       context_provider_(context_provider),
-      context_provider_lost_(false),
       enable_video_gpu_memory_buffers_(enable_video_gpu_memory_buffers),
       enable_media_stream_gpu_memory_buffers_(
           enable_media_stream_gpu_memory_buffers),
@@ -112,19 +111,31 @@ void GpuVideoAcceleratorFactoriesImpl::BindContextToTaskRunner() {
   DCHECK(context_provider_);
   if (context_provider_->BindToCurrentThread() !=
       gpu::ContextResult::kSuccess) {
-    ReleaseContextProvider();
+    SetContextProviderLost();
   }
 }
 
 bool GpuVideoAcceleratorFactoriesImpl::CheckContextLost() {
   DCHECK(task_runner_->BelongsToCurrentThread());
-  if (context_provider_) {
-    if (context_provider_->ContextGL()->GetGraphicsResetStatusKHR() !=
-        GL_NO_ERROR) {
-      ReleaseContextProvider();
-    }
+  if (context_provider_lost_on_media_thread_)
+    return true;
+  if (context_provider_->ContextGL()->GetGraphicsResetStatusKHR() !=
+      GL_NO_ERROR) {
+    SetContextProviderLost();
+    return true;
   }
-  return !context_provider_;
+  return false;
+}
+
+void GpuVideoAcceleratorFactoriesImpl::DestroyContext() {
+  DCHECK(task_runner_->BelongsToCurrentThread());
+  DCHECK(context_provider_lost_on_media_thread_);
+
+  if (!context_provider_)
+    return;
+  context_provider_ = nullptr;
+  RecordContextProviderPhaseUmaEnum(
+      ContextProviderPhase::CONTEXT_PROVIDER_RELEASED);
 }
 
 bool GpuVideoAcceleratorFactoriesImpl::IsGpuVideoAcceleratorEnabled() {
@@ -367,7 +378,7 @@ void GpuVideoAcceleratorFactoriesImpl::SetRenderingColorSpace(
   rendering_color_space_ = color_space;
 }
 
-bool GpuVideoAcceleratorFactoriesImpl::CheckContextProviderLost() {
+bool GpuVideoAcceleratorFactoriesImpl::CheckContextProviderLostOnMainThread() {
   DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
   return context_provider_lost_;
 }
@@ -381,20 +392,23 @@ void GpuVideoAcceleratorFactoriesImpl::
   vea_provider_.Bind(std::move(unbound_vea_provider), task_runner_);
 }
 
-void GpuVideoAcceleratorFactoriesImpl::ReleaseContextProvider() {
+void GpuVideoAcceleratorFactoriesImpl::SetContextProviderLost() {
   DCHECK(task_runner_->BelongsToCurrentThread());
 
+  // Don't delete the |context_provider_| here, we could be in the middle of
+  // it notifying about the loss, and we'd be destroying it while it's on
+  // the stack.
+  context_provider_lost_on_media_thread_ = true;
+  // Inform the main thread of the loss as well, so that this class can be
+  // replaced.
   main_thread_task_runner_->PostTask(
       FROM_HERE,
-      base::BindOnce(&GpuVideoAcceleratorFactoriesImpl::SetContextProviderLost,
-                     base::Unretained(this)));
-
-  context_provider_ = nullptr;
-  RecordContextProviderPhaseUmaEnum(
-      ContextProviderPhase::CONTEXT_PROVIDER_RELEASED);
+      base::BindOnce(
+          &GpuVideoAcceleratorFactoriesImpl::SetContextProviderLostOnMainThread,
+          base::Unretained(this)));
 }
 
-void GpuVideoAcceleratorFactoriesImpl::SetContextProviderLost() {
+void GpuVideoAcceleratorFactoriesImpl::SetContextProviderLostOnMainThread() {
   DCHECK(main_thread_task_runner_->BelongsToCurrentThread());
   context_provider_lost_ = true;
 }

content/renderer/media/gpu/gpu_video_accelerator_factories_impl.h

@@ -90,9 +90,19 @@ class CONTENT_EXPORT GpuVideoAcceleratorFactoriesImpl
   unsigned ImageTextureTarget(gfx::BufferFormat format) override;
   OutputFormat VideoFrameOutputFormat(size_t bit_depth) override;
 
+  // Called on the media thread. Returns the GLES2Interface unless the
+  // ContextProvider has been lost, in which case it returns null.
   gpu::gles2::GLES2Interface* ContextGL() override;
   void BindContextToTaskRunner();
+  // Called on the media thread. Verifies if the ContextProvider is lost and
+  // notifies the main thread of loss if it has occured, which can be seen later
+  // from CheckContextProviderLost().
   bool CheckContextLost();
+  // Called on the media thread. Destroys the ContextProvider held in this
+  // class. Should only be called if the ContextProvider was previously lost,
+  // and this class will no longer be used, as it assumes a ContextProvider is
+  // present otherwise.
+  void DestroyContext();
   std::unique_ptr<base::SharedMemory> CreateSharedMemory(size_t size) override;
   scoped_refptr<base::SingleThreadTaskRunner> GetTaskRunner() override;
 
@@ -106,7 +116,10 @@ class CONTENT_EXPORT GpuVideoAcceleratorFactoriesImpl
 
   void SetRenderingColorSpace(const gfx::ColorSpace& color_space) override;
 
-  bool CheckContextProviderLost();
+  // Called on the main thread. Returns whether the media thread has seen the
+  // ContextProvider become lost, in which case this class should be replaced
+  // with a new ContextProvider.
+  bool CheckContextProviderLostOnMainThread();
 
   ~GpuVideoAcceleratorFactoriesImpl() override;
 
@@ -125,8 +138,8 @@ class CONTENT_EXPORT GpuVideoAcceleratorFactoriesImpl
   void BindVideoEncodeAcceleratorProviderOnTaskRunner(
       media::mojom::VideoEncodeAcceleratorProviderPtrInfo unbound_vea_provider);
 
-  void ReleaseContextProvider();
   void SetContextProviderLost();
+  void SetContextProviderLostOnMainThread();
 
   const scoped_refptr<base::SingleThreadTaskRunner> main_thread_task_runner_;
   const scoped_refptr<base::SingleThreadTaskRunner> task_runner_;
@@ -136,8 +149,11 @@ class CONTENT_EXPORT GpuVideoAcceleratorFactoriesImpl
   // 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_;
+  // Signals if |context_provider_| is alive on the media thread. For use on the
+  // main thread.
+  bool context_provider_lost_ = false;
+  // A shadow of |context_provider_lost_| for the media thread.
+  bool context_provider_lost_on_media_thread_ = false;
 
   base::UnguessableToken channel_token_;
 

content/renderer/render_thread_impl.cc

@@ -1412,13 +1412,12 @@ media::GpuVideoAcceleratorFactories* RenderThreadImpl::GetGpuFactories() {
   DCHECK(IsMainThread());
 
   if (!gpu_factories_.empty()) {
-    if (!gpu_factories_.back()->CheckContextProviderLost())
+    if (!gpu_factories_.back()->CheckContextProviderLostOnMainThread())
       return gpu_factories_.back().get();
 
     GetMediaThreadTaskRunner()->PostTask(
         FROM_HERE,
-        base::BindOnce(base::IgnoreResult(
-                           &GpuVideoAcceleratorFactoriesImpl::CheckContextLost),
+        base::BindOnce(&GpuVideoAcceleratorFactoriesImpl::DestroyContext,
                        base::Unretained(gpu_factories_.back().get())));
   }