gpu: Check errors when restoring context in ScopedMakeCurrent.

ScopedMakeCurrent and ScopedReleaseCurrent don't check for errors when
restoring the previous context. In most cases the MakeCurrent that
restores the previous context should not fail, and if it fails we don't
return errors up the stack so we CHECK that MakeCurrent succeeds.

However, for DirectComposition, we use ScopedReleaseCurrent when
reallocating the drawing surface. Restoring the context can fail in this
case. This CL adds a Restore method to both classes whose return value
must be handled. This prevents the CHECK in the destructor.

The lack of error checking is causing a crash in SetSwapInterval, due to
null-dereference of GLContext::GetCurrent. This was found by adding a
crash key to get the stack trace for SetCurrent(NULL).

BUG=724999
R=piman

Cq-Include-Trybots: master.tryserver.chromium.android:android_optional_gpu_tests_rel;master.tryserver.chromium.linux:linux_optional_gpu_tests_rel;master.tryserver.chromium.mac:mac_optional_gpu_tests_rel;master.tryserver.chromium.win:win_optional_gpu_tests_rel
Change-Id: I370ff470c78afa76bd000de60a6a63295746ee1b
Reviewed-on: https://chromium-review.googlesource.com/818533Reviewed-by: Antoine Labour <piman@chromium.org>
Commit-Queue: Sunny Sachanandani <sunnyps@chromium.org>
Cr-Commit-Position: refs/heads/master@{#523562}

gpu/ipc/service/direct_composition_child_surface_win.cc

@@ -8,6 +8,7 @@
 #include <dcomptypes.h>
 
 #include "base/debug/alias.h"
+#include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/synchronization/waitable_event.h"
@@ -55,6 +56,7 @@ DirectCompositionChildSurfaceWin::~DirectCompositionChildSurfaceWin() {
 }
 
 bool DirectCompositionChildSurfaceWin::Initialize(gl::GLSurfaceFormat format) {
+  ui::ScopedReleaseCurrent release_current;
   d3d11_device_ = gl::QueryD3D11DeviceObjectFromANGLE();
   dcomp_device_ = gl::QueryDirectCompositionDevice(d3d11_device_);
   if (!dcomp_device_)
@@ -73,7 +75,7 @@ bool DirectCompositionChildSurfaceWin::Initialize(gl::GLSurfaceFormat format) {
       eglCreatePbufferSurface(display, GetConfig(), &pbuffer_attribs[0]);
   CHECK(!!default_surface_);
 
-  return true;
+  return release_current.Restore();
 }
 
 void DirectCompositionChildSurfaceWin::ReleaseCurrentSurface() {
@@ -129,6 +131,7 @@ bool DirectCompositionChildSurfaceWin::InitializeSurface() {
 }
 
 void DirectCompositionChildSurfaceWin::ReleaseDrawTexture(bool will_discard) {
+  DCHECK(!gl::GLContext::GetCurrent());
   if (real_surface_) {
     eglDestroySurface(GetDisplay(), real_surface_);
     real_surface_ = nullptr;
@@ -242,24 +245,33 @@ bool DirectCompositionChildSurfaceWin::SupportsDCLayers() const {
 
 bool DirectCompositionChildSurfaceWin::SetDrawRectangle(
     const gfx::Rect& rectangle) {
-  if (draw_texture_)
+  if (!gfx::Rect(size_).Contains(rectangle)) {
+    DLOG(ERROR) << "Draw rectangle must be contained within size of surface";
     return false;
+  }
+
+  if (draw_texture_) {
+    DLOG(ERROR) << "SetDrawRectangle must be called only once per swap buffers";
+    return false;
+  }
+
   DCHECK(!real_surface_);
-  ui::ScopedReleaseCurrent release_current(this);
+
+  ui::ScopedReleaseCurrent release_current;
 
   if ((enable_dc_layers_ && !dcomp_surface_) ||
       (!enable_dc_layers_ && !swap_chain_)) {
     ReleaseCurrentSurface();
     if (!InitializeSurface()) {
-      LOG(ERROR) << "InitializeSurface failed";
+      DLOG(ERROR) << "InitializeSurface failed";
+      // It is likely that restoring the context will fail, so call Restore here
+      // to avoid the assert during ScopedReleaseCurrent destruction.
+      ignore_result(release_current.Restore());
       return false;
     }
   }
 
-  if (!gfx::Rect(size_).Contains(rectangle)) {
-    DLOG(ERROR) << "Draw rectangle must be contained within size of surface";
-    return false;
-  }
+  // Check this after reinitializing the surface because we reset state there.
   if (gfx::Rect(size_) != rectangle && !has_been_rendered_to_) {
     DLOG(ERROR) << "First draw to surface must draw to everything";
     return false;
@@ -300,6 +312,11 @@ bool DirectCompositionChildSurfaceWin::SetDrawRectangle(
       GetDisplay(), EGL_D3D_TEXTURE_ANGLE, buffer, GetConfig(),
       &pbuffer_attribs[0]);
 
+  if (!release_current.Restore()) {
+    DLOG(ERROR) << "Failed to restore context";
+    return false;
+  }
+
   return true;
 }
 

gpu/ipc/service/direct_composition_surface_win.cc

@@ -1231,20 +1231,17 @@ bool DirectCompositionSurfaceWin::Resize(const gfx::Size& size,
   size_ = size;
   is_hdr_ = is_hdr;
   has_alpha_ = has_alpha;
-  ui::ScopedReleaseCurrent release_current(this);
   return RecreateRootSurface();
 }
 
 gfx::SwapResult DirectCompositionSurfaceWin::SwapBuffers(
     const PresentationCallback& callback) {
-  {
-    ui::ScopedReleaseCurrent release_current(this);
-    root_surface_->SwapBuffers(callback);
-
-    layer_tree_->CommitAndClearPendingOverlays();
-  }
+  ui::ScopedReleaseCurrent release_current;
+  root_surface_->SwapBuffers(callback);
+  layer_tree_->CommitAndClearPendingOverlays();
   child_window_.ClearInvalidContents();
-  return gfx::SwapResult::SWAP_ACK;
+  return release_current.Restore() ? gfx::SwapResult::SWAP_ACK
+                                   : gfx::SwapResult::SWAP_FAILED;
 }
 
 gfx::SwapResult DirectCompositionSurfaceWin::PostSubBuffer(
@@ -1270,12 +1267,10 @@ bool DirectCompositionSurfaceWin::ScheduleDCLayer(
 bool DirectCompositionSurfaceWin::SetEnableDCLayers(bool enable) {
   if (enable_dc_layers_ == enable)
     return true;
-  ui::ScopedReleaseCurrent release_current(this);
   enable_dc_layers_ = enable;
   return RecreateRootSurface();
 }
 
-
 bool DirectCompositionSurfaceWin::FlipsVertically() const {
   return true;
 }

ui/gl/scoped_make_current.cc

@@ -15,36 +15,55 @@ ScopedMakeCurrent::ScopedMakeCurrent(gl::GLContext* context,
     : previous_context_(gl::GLContext::GetCurrent()),
       previous_surface_(gl::GLSurface::GetCurrent()),
       context_(context),
-      surface_(surface),
-      succeeded_(false) {
+      surface_(surface) {
   DCHECK(context);
   DCHECK(surface);
   succeeded_ = context->MakeCurrent(surface);
 }
 
 ScopedMakeCurrent::~ScopedMakeCurrent() {
-  if (previous_context_.get()) {
-    DCHECK(previous_surface_.get());
-    previous_context_->MakeCurrent(previous_surface_.get());
-  } else {
-    context_->ReleaseCurrent(surface_.get());
-  }
+  if (!restored_)
+    CHECK(Restore()) << "ScopedMakeCurrent: Restore failed";
 }
 
 bool ScopedMakeCurrent::Succeeded() const {
   return succeeded_;
 }
 
-ScopedReleaseCurrent::ScopedReleaseCurrent(gl::GLSurface* this_surface) {
-  gl::GLContext* current_context = gl::GLContext::GetCurrent();
-  bool was_current =
-      current_context && current_context->IsCurrent(this_surface);
-  if (was_current) {
-    make_current_.emplace(current_context, this_surface);
-    current_context->ReleaseCurrent(this_surface);
-  }
+bool ScopedMakeCurrent::Restore() {
+  DCHECK(!restored_);
+  restored_ = true;
+
+  if (!succeeded_)
+    return true;
+
+  if (previous_context_)
+    return previous_context_->MakeCurrent(previous_surface_.get());
+
+  context_->ReleaseCurrent(surface_.get());
+  return true;
+}
+
+ScopedReleaseCurrent::ScopedReleaseCurrent()
+    : previous_context_(gl::GLContext::GetCurrent()),
+      previous_surface_(gl::GLSurface::GetCurrent()) {
+  if (previous_context_)
+    previous_context_->ReleaseCurrent(previous_surface_.get());
 }
 
-ScopedReleaseCurrent::~ScopedReleaseCurrent() {}
+ScopedReleaseCurrent::~ScopedReleaseCurrent() {
+  if (!restored_)
+    CHECK(Restore()) << "ScopedReleaseCurrent: Restore failed";
+}
+
+bool ScopedReleaseCurrent::Restore() {
+  DCHECK(!restored_);
+  restored_ = true;
+
+  if (previous_context_)
+    return previous_context_->MakeCurrent(previous_surface_.get());
+
+  return true;
+}
 
 }  // namespace ui

ui/gl/scoped_make_current.h

@@ -5,6 +5,7 @@
 #ifndef UI_GL_SCOPED_MAKE_CURRENT_H_
 #define UI_GL_SCOPED_MAKE_CURRENT_H_
 
+#include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
 #include "base/optional.h"
@@ -17,6 +18,11 @@ class GLSurface;
 
 namespace ui {
 
+// ScopedMakeCurrent makes given context current with the given surface. If this
+// fails, Succeeded() returns false. It also restores the previous context on
+// destruction and asserts that this succeeds. Users can optionally use
+// Restore() to check if restoring the previous context succeeds. This prevents
+// the assert during destruction.
 class GL_EXPORT ScopedMakeCurrent {
  public:
   ScopedMakeCurrent(gl::GLContext* context, gl::GLSurface* surface);
@@ -24,26 +30,32 @@ class GL_EXPORT ScopedMakeCurrent {
 
   bool Succeeded() const;
 
+  bool Restore() WARN_UNUSED_RESULT;
+
  private:
   scoped_refptr<gl::GLContext> previous_context_;
   scoped_refptr<gl::GLSurface> previous_surface_;
   scoped_refptr<gl::GLContext> context_;
   scoped_refptr<gl::GLSurface> surface_;
-  bool succeeded_;
+  bool succeeded_ = false;
+  bool restored_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(ScopedMakeCurrent);
 };
 
-// This class is used to make sure a specified surface isn't current, and upon
-// destruction it will make the surface current again if it had been before.
+// This behaves similarly to ScopedMakeCurrent, except that it releases the
+// current context on creation and restores it on destruction.
 class GL_EXPORT ScopedReleaseCurrent {
  public:
-  explicit ScopedReleaseCurrent(gl::GLSurface* this_surface);
-
+  ScopedReleaseCurrent();
   ~ScopedReleaseCurrent();
 
+  bool Restore() WARN_UNUSED_RESULT;
+
  private:
-  base::Optional<ScopedMakeCurrent> make_current_;
+  scoped_refptr<gl::GLContext> previous_context_;
+  scoped_refptr<gl::GLSurface> previous_surface_;
+  bool restored_ = false;
 
   DISALLOW_COPY_AND_ASSIGN(ScopedReleaseCurrent);
 };