Check the device reset status more often

Sometimes a device driver signals an internal error and we just need to
reset. This CL checks for reset more aggressively on all
{SharedContextState,SharedImageStub}::MakeCurrent calls. This
will hopefully turn some crashes into restarts.

Bug: 1079161,1081806
Change-Id: I260f90b4ed2ba63be3cf7ca6759c5aa086f0c46f
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2214919
Commit-Queue: Jonathan Backer <backer@chromium.org>
Reviewed-by: Peng Huang <penghuang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#773838}

components/viz/service/display_embedder/skia_output_surface_impl_on_gpu.cc

@@ -1640,9 +1640,10 @@ bool SkiaOutputSurfaceImplOnGpu::MakeCurrent(bool need_fbo0) {
   if (!context_state_->MakeCurrent(gl_surface, need_gl)) {
     LOG(ERROR) << "Failed to make current.";
     dependency_->DidLoseContext(
-        gpu::error::kMakeCurrentFailed,
+        *context_state_->context_lost_reason(),
         GURL("chrome://gpu/SkiaOutputSurfaceImplOnGpu::MakeCurrent"));
-    MarkContextLost(CONTEXT_LOST_MAKECURRENT_FAILED);
+    MarkContextLost(GetContextLostReason(
+        gpu::error::kLostContext, *context_state_->context_lost_reason()));
     return false;
   }
   context_state_->set_need_context_state_reset(true);

gpu/command_buffer/service/raster_decoder.cc

@@ -208,7 +208,8 @@ bool AllowedBetweenBeginEndRaster(CommandId command) {
 // avoid it as much as possible.
 class RasterDecoderImpl final : public RasterDecoder,
                                 public gles2::ErrorStateClient,
-                                public ServiceFontManager::Client {
+                                public ServiceFontManager::Client,
+                                public SharedContextState::ContextLostObserver {
  public:
   RasterDecoderImpl(DecoderClient* client,
                     CommandBufferServiceBase* command_buffer_service,
@@ -365,6 +366,9 @@ class RasterDecoderImpl final : public RasterDecoder,
   scoped_refptr<Buffer> GetShmBuffer(uint32_t shm_id) override;
   void ReportProgress() override;
 
+  // SharedContextState::ContextLostObserver implementation.
+  void OnContextLost() override;
+
  private:
   gles2::ContextState* state() const {
     if (use_passthrough_) {
@@ -583,8 +587,6 @@ class RasterDecoderImpl final : public RasterDecoder,
   bool use_passthrough_ = false;
   bool use_ddl_ = false;
 
-  bool reset_by_robustness_extension_ = false;
-
   // The current decoder error communicates the decoder error through command
   // processing functions that do not return the error value. Should be set
   // only if not returning an error.
@@ -756,9 +758,12 @@ RasterDecoderImpl::RasterDecoderImpl(
       font_manager_(base::MakeRefCounted<ServiceFontManager>(this)),
       is_privileged_(is_privileged) {
   DCHECK(shared_context_state_);
+  shared_context_state_->AddContextLostObserver(this);
 }
 
-RasterDecoderImpl::~RasterDecoderImpl() = default;
+RasterDecoderImpl::~RasterDecoderImpl() {
+  shared_context_state_->RemoveContextLostObserver(this);
+}
 
 base::WeakPtr<DecoderContext> RasterDecoderImpl::AsWeakPtr() {
   return weak_ptr_factory_.GetWeakPtr();
@@ -893,18 +898,11 @@ bool RasterDecoderImpl::MakeCurrent() {
   if (shared_context_state_->context_lost() ||
       !shared_context_state_->MakeCurrent(nullptr)) {
     LOG(ERROR) << "  RasterDecoderImpl: Context lost during MakeCurrent.";
-    MarkContextLost(error::kMakeCurrentFailed);
     return false;
   }
 
   DCHECK_EQ(api(), gl::g_current_gl_context);
 
-  if (CheckResetStatus()) {
-    LOG(ERROR)
-        << "  RasterDecoderImpl: Context reset detected after MakeCurrent.";
-    return false;
-  }
-
   // Rebind textures if the service ids may have changed.
   RestoreAllExternalTextureBindingsIfNeeded();
 
@@ -1115,55 +1113,27 @@ void RasterDecoderImpl::SetLevelInfo(uint32_t client_id,
 }
 
 bool RasterDecoderImpl::WasContextLost() const {
-  return context_lost_;
+  return shared_context_state_->context_lost();
 }
 
 bool RasterDecoderImpl::WasContextLostByRobustnessExtension() const {
-  return WasContextLost() && reset_by_robustness_extension_;
+  return shared_context_state_->device_needs_reset();
 }
 
 void RasterDecoderImpl::MarkContextLost(error::ContextLostReason reason) {
-  // Only lose the context once.
-  if (WasContextLost())
-    return;
+  shared_context_state_->MarkContextLost(reason);
+}
 
-  // Don't make GL calls in here, the context might not be current.
-  context_lost_ = true;
-  command_buffer_service()->SetContextLostReason(reason);
+void RasterDecoderImpl::OnContextLost() {
+  DCHECK(shared_context_state_->context_lost());
+  command_buffer_service()->SetContextLostReason(
+      *shared_context_state_->context_lost_reason());
   current_decoder_error_ = error::kLostContext;
 }
 
 bool RasterDecoderImpl::CheckResetStatus() {
   DCHECK(!WasContextLost());
-  DCHECK(shared_context_state_->context()->IsCurrent(nullptr));
-
-  // If the reason for the call was a GL error, we can try to determine the
-  // reset status more accurately.
-  GLenum driver_status =
-      shared_context_state_->context()->CheckStickyGraphicsResetStatus();
-  if (driver_status == GL_NO_ERROR)
-    return false;
-
-  LOG(ERROR) << "RasterDecoder context lost via ARB/EXT_robustness. Reset "
-                "status = "
-             << gles2::GLES2Util::GetStringEnum(driver_status);
-
-  switch (driver_status) {
-    case GL_GUILTY_CONTEXT_RESET_ARB:
-      MarkContextLost(error::kGuilty);
-      break;
-    case GL_INNOCENT_CONTEXT_RESET_ARB:
-      MarkContextLost(error::kInnocent);
-      break;
-    case GL_UNKNOWN_CONTEXT_RESET_ARB:
-      MarkContextLost(error::kUnknown);
-      break;
-    default:
-      NOTREACHED();
-      return false;
-  }
-  reset_by_robustness_extension_ = true;
-  return true;
+  return shared_context_state_->CheckResetStatus(/*needs_gl=*/false);
 }
 
 gles2::Logger* RasterDecoderImpl::GetLogger() {
@@ -1502,14 +1472,13 @@ void RasterDecoderImpl::DisableFlushWorkaroundForTest() {
 void RasterDecoderImpl::OnContextLostError() {
   if (!WasContextLost()) {
     // Need to lose current context before broadcasting!
-    CheckResetStatus();
-    reset_by_robustness_extension_ = true;
+    shared_context_state_->CheckResetStatus(/*needs_gl=*/false);
   }
 }
 
 void RasterDecoderImpl::OnOutOfMemoryError() {
   if (lose_context_when_out_of_memory_ && !WasContextLost()) {
-    if (!CheckResetStatus()) {
+    if (!shared_context_state_->CheckResetStatus(/*needs_gl=*/false)) {
       MarkContextLost(error::kOutOfMemory);
     }
   }

gpu/command_buffer/service/shared_context_state.cc

@@ -69,7 +69,7 @@ size_t MaxNumSkSurface() {
 namespace gpu {
 
 void SharedContextState::compileError(const char* shader, const char* errors) {
-  if (!context_lost_) {
+  if (!context_lost()) {
     LOG(ERROR) << "Skia shader compilation error\n"
                << "------------------------\n"
                << shader << "\nErrors:\n"
@@ -163,7 +163,7 @@ SharedContextState::~SharedContextState() {
   // The context should be current so that texture deletes that result from
   // destroying the cache happen in the right context (unless the context is
   // lost in which case we don't delete the textures).
-  DCHECK(IsCurrent(nullptr) || context_lost_);
+  DCHECK(IsCurrent(nullptr) || context_lost());
   transfer_cache_.reset();
 
   // We should have the last ref on this GrContext to ensure we're not holding
@@ -425,28 +425,23 @@ bool SharedContextState::InitializeGL(
 }
 
 bool SharedContextState::MakeCurrent(gl::GLSurface* surface, bool needs_gl) {
-  if (context_lost_)
+  if (context_lost())
     return false;
 
-  if (gr_context_ && gr_context_->abandoned()) {
-    MarkContextLost();
-    return false;
-  }
-
-  if (!GrContextIsGL() && !needs_gl)
-    return true;
-
+  const bool using_gl = GrContextIsGL() || needs_gl;
+  if (using_gl) {
     gl::GLSurface* dont_care_surface =
         last_current_surface_ ? last_current_surface_ : surface_.get();
     surface = surface ? surface : dont_care_surface;
 
     if (!context_->MakeCurrent(surface)) {
-    MarkContextLost();
+      MarkContextLost(error::kMakeCurrentFailed);
       return false;
     }
     last_current_surface_ = surface;
+  }
 
-  return true;
+  return !CheckResetStatus(needs_gl);
 }
 
 void SharedContextState::ReleaseCurrent(gl::GLSurface* surface) {
@@ -457,14 +452,14 @@ void SharedContextState::ReleaseCurrent(gl::GLSurface* surface) {
     return;
 
   last_current_surface_ = nullptr;
-  if (!context_lost_)
+  if (!context_lost())
     context_->ReleaseCurrent(surface);
 }
 
-void SharedContextState::MarkContextLost() {
-  if (!context_lost_) {
+void SharedContextState::MarkContextLost(error::ContextLostReason reason) {
+  if (!context_lost()) {
     scoped_refptr<SharedContextState> prevent_last_ref_drop = this;
-    context_lost_ = true;
+    context_lost_reason_ = reason;
     // context_state_ could be nullptr for some unittests.
     if (context_state_)
       context_state_->MarkContextLost();
@@ -487,7 +482,7 @@ void SharedContextState::MarkContextLost() {
 bool SharedContextState::IsCurrent(gl::GLSurface* surface) {
   if (!GrContextIsGL())
     return true;
-  if (context_lost_)
+  if (context_lost())
     return false;
   return context_->IsCurrent(surface);
 }
@@ -670,4 +665,46 @@ QueryManager* SharedContextState::GetQueryManager() {
   return nullptr;
 }
 
+bool SharedContextState::CheckResetStatus(bool needs_gl) {
+  DCHECK(!context_lost());
+
+  if (device_needs_reset_)
+    return true;
+
+  // Maybe Skia detected VK_ERROR_DEVICE_LOST.
+  if (gr_context_ && gr_context_->abandoned()) {
+    LOG(ERROR) << "SharedContextState context lost via Skia.";
+    device_needs_reset_ = true;
+    MarkContextLost(error::kUnknown);
+    return true;
+  }
+
+  if (!GrContextIsGL() && !needs_gl)
+    return false;
+
+  GLenum driver_status = context()->CheckStickyGraphicsResetStatus();
+  if (driver_status == GL_NO_ERROR)
+    return false;
+  LOG(ERROR) << "SharedContextState context lost via ARB/EXT_robustness. Reset "
+                "status = "
+             << gles2::GLES2Util::GetStringEnum(driver_status);
+
+  switch (driver_status) {
+    case GL_GUILTY_CONTEXT_RESET_ARB:
+      MarkContextLost(error::kGuilty);
+      break;
+    case GL_INNOCENT_CONTEXT_RESET_ARB:
+      MarkContextLost(error::kInnocent);
+      break;
+    case GL_UNKNOWN_CONTEXT_RESET_ARB:
+      MarkContextLost(error::kUnknown);
+      break;
+    default:
+      NOTREACHED();
+      break;
+  }
+  device_needs_reset_ = true;
+  return false;
+}
+
 }  // namespace gpu

gpu/command_buffer/service/shared_context_state.h

@@ -13,8 +13,11 @@
 #include "base/memory/ref_counted.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
+#include "base/optional.h"
 #include "base/trace_event/memory_dump_provider.h"
 #include "build/build_config.h"
+#include "gpu/command_buffer/common/constants.h"
+#include "gpu/command_buffer/common/gl2_types.h"
 #include "gpu/command_buffer/common/skia_utils.h"
 #include "gpu/command_buffer/service/gl_context_virtual_delegate.h"
 #include "gpu/command_buffer/service/memory_tracking.h"
@@ -92,7 +95,7 @@ class GPU_GLES2_EXPORT SharedContextState
 
   bool MakeCurrent(gl::GLSurface* surface, bool needs_gl = false);
   void ReleaseCurrent(gl::GLSurface* surface);
-  void MarkContextLost();
+  void MarkContextLost(error::ContextLostReason reason = error::kUnknown);
   bool IsCurrent(gl::GLSurface* surface);
 
   void PurgeMemory(
@@ -122,7 +125,10 @@ class GPU_GLES2_EXPORT SharedContextState
   void compileError(const char* shader, const char* errors) override;
   gles2::FeatureInfo* feature_info() { return feature_info_.get(); }
   gles2::ContextState* context_state() const { return context_state_.get(); }
-  bool context_lost() const { return context_lost_; }
+  bool context_lost() const { return !!context_lost_reason_; }
+  base::Optional<error::ContextLostReason> context_lost_reason() {
+    return context_lost_reason_;
+  }
   bool need_context_state_reset() const { return need_context_state_reset_; }
   void set_need_context_state_reset(bool reset) {
     need_context_state_reset_ = reset;
@@ -179,6 +185,11 @@ class GPU_GLES2_EXPORT SharedContextState
     return found->second->unique();
   }
 
+  // Updates |context_lost_reason| and returns true if lost
+  // (e.g. VK_ERROR_DEVICE_LOST or GL_UNKNOWN_CONTEXT_RESET_ARB).
+  bool CheckResetStatus(bool needs_gl);
+  bool device_needs_reset() { return device_needs_reset_; }
+
  private:
   friend class base::RefCounted<SharedContextState>;
 
@@ -265,11 +276,13 @@ class GPU_GLES2_EXPORT SharedContextState
   // driver's GL state.
   bool need_context_state_reset_ = false;
 
-  bool context_lost_ = false;
+  base::Optional<error::ContextLostReason> context_lost_reason_;
   base::ObserverList<ContextLostObserver>::Unchecked context_lost_observers_;
 
   base::MRUCache<void*, sk_sp<SkSurface>> sk_surface_cache_;
 
+  bool device_needs_reset_ = false;
+
   base::WeakPtrFactory<SharedContextState> weak_ptr_factory_{this};
 
   DISALLOW_COPY_AND_ASSIGN(SharedContextState);

gpu/ipc/in_process_gpu_thread_holder.cc

@@ -83,17 +83,19 @@ void InProcessGpuThreadHolder::InitializeOnGpuThread(
   GpuDriverBugWorkarounds gpu_driver_bug_workarounds(
       gpu_feature_info_.enabled_gpu_driver_bug_workarounds);
 
-  bool use_virtualized_gl_context_ = false;
+  bool use_virtualized_gl_context = false;
 #if defined(OS_MACOSX)
   // Virtualize GpuPreference:::kLowPower contexts by default on OS X to prevent
   // performance regressions when enabling FCM. https://crbug.com/180463
-  use_virtualized_gl_context_ = true;
+  use_virtualized_gl_context = true;
 #endif
-  use_virtualized_gl_context_ |=
+  use_virtualized_gl_context |=
       gpu_driver_bug_workarounds.use_virtualized_gl_contexts;
+  if (use_virtualized_gl_context)
+    share_group_->SetSharedContext(context_.get());
 
   context_state_ = base::MakeRefCounted<SharedContextState>(
-      share_group_, surface_, context_, use_virtualized_gl_context_,
+      share_group_, surface_, context_, use_virtualized_gl_context,
       base::DoNothing(), gpu_preferences_.gr_context_type);
   auto feature_info = base::MakeRefCounted<gles2::FeatureInfo>(
       gpu_driver_bug_workarounds, gpu_feature_info_);

gpu/ipc/service/shared_image_stub.cc

@@ -400,13 +400,9 @@ bool SharedImageStub::MakeContextCurrent() {
   // |factory_| never writes to the surface, so pass nullptr to
   // improve performance. https://crbug.com/457431
   auto* context = context_state_->real_context();
-  if (context->IsCurrent(nullptr) ||
-      context->MakeCurrent(context_state_->surface())) {
-    return true;
-  }
-  context_state_->MarkContextLost();
-  LOG(ERROR) << "SharedImageStub: MakeCurrent failed";
-  return false;
+  if (context->IsCurrent(nullptr))
+    return !context_state_->CheckResetStatus(/*needs_gl=*/true);
+  return context_state_->MakeCurrent(/*surface=*/nullptr, /*needs_gl=*/true);
 }
 
 ContextResult SharedImageStub::MakeContextCurrentAndCreateFactory() {

gpu/ipc/shared_image_interface_in_process.cc

@@ -104,12 +104,9 @@ bool SharedImageInterfaceInProcess::MakeContextCurrent() {
   // |shared_image_factory_| never writes to the surface, so skip unnecessary
   // MakeCurrent to improve performance. https://crbug.com/457431
   auto* context = context_state_->real_context();
-  if (context->IsCurrent(nullptr) ||
-      context->MakeCurrent(context_state_->surface()))
-    return true;
-
-  context_state_->MarkContextLost();
-  return false;
+  if (context->IsCurrent(nullptr))
+    return !context_state_->CheckResetStatus(/*needs_gl=*/true);
+  return context_state_->MakeCurrent(/*surface=*/nullptr, /*needs_gl=*/true);
 }
 
 void SharedImageInterfaceInProcess::LazyCreateSharedImageFactory() {