viz/mac: Add an explicit GLSurface clean-up call

Add an explicit method on GLSurface that is to be called while the
GLContext is still current, but before the GLSurface is destroyed.

A step in this direction was taken in https://crrev.com/538387, where
we made the GLContext current again after its decoder had been
destroyed, but that path has proved unstable. In particular, the
GLSurface internals on macOS are created while the decoder is active,
and so destroying them after the decoder is destroyed is precarious.

Bug: 772576, 817830
Cq-Include-Trybots: luci.chromium.try:android_optional_gpu_tests_rel;luci.chromium.try:linux_optional_gpu_tests_rel;luci.chromium.try:mac_optional_gpu_tests_rel;luci.chromium.try:win_optional_gpu_tests_rel
Change-Id: I3ff1c395c4e9d7ac0b0f1ab54400cde28bdbe298
Reviewed-on: https://chromium-review.googlesource.com/1046046Reviewed-by: Zhenyao Mo <zmo@chromium.org>
Commit-Queue: ccameron <ccameron@chromium.org>
Cr-Commit-Position: refs/heads/master@{#557250}

gpu/command_buffer/service/gles2_cmd_decoder.cc

@@ -5042,6 +5042,11 @@ void GLES2DecoderImpl::Destroy(bool have_context) {
 
   DCHECK(!have_context || context_->IsCurrent(nullptr));
 
+  // Prepare to destroy the surface while the context is still current, because
+  // some surface destructors make GL calls.
+  if (surface_)
+    surface_->PrepareToDestroy(have_context);
+
   ReleaseAllBackTextures(have_context);
   if (have_context) {
     if (apply_framebuffer_attachment_cmaa_intel_.get()) {
@@ -5232,14 +5237,11 @@ void GLES2DecoderImpl::Destroy(bool have_context) {
     group_ = nullptr;
   }
 
-  // Destroy the surface before the context, some surface destructors make GL
-  // calls.
-  surface_ = nullptr;
-
   if (context_.get()) {
     context_->ReleaseCurrent(nullptr);
     context_ = nullptr;
   }
+  surface_ = nullptr;
 }
 
 void GLES2DecoderImpl::SetSurface(const scoped_refptr<gl::GLSurface>& surface) {

gpu/ipc/in_process_command_buffer.cc

@@ -560,16 +560,17 @@ bool InProcessCommandBuffer::DestroyOnGpuThread() {
   gpu_thread_weak_ptr_factory_.InvalidateWeakPtrs();
   // Clean up GL resources if possible.
   bool have_context = context_.get() && context_->MakeCurrent(surface_.get());
+
+  // Prepare to destroy the surface while the context is still current, because
+  // some surface destructors make GL calls.
+  if (surface_)
+    surface_->PrepareToDestroy(have_context);
+
   if (decoder_) {
     decoder_->Destroy(have_context);
     decoder_.reset();
   }
   command_buffer_.reset();
-
-  // Destroy the surface with the context current, some surface destructors make
-  // GL calls.
-  if (context_)
-    context_->MakeCurrent(surface_.get());
   surface_ = nullptr;
 
   context_ = nullptr;

gpu/ipc/service/image_transport_surface_overlay_mac.h

@@ -37,6 +37,7 @@ class ImageTransportSurfaceOverlayMac : public gl::GLSurface,
   // GLSurface implementation
   bool Initialize(gl::GLSurfaceFormat format) override;
   void Destroy() override;
+  void PrepareToDestroy(bool have_context) override;
   bool Resize(const gfx::Size& size,
               float scale_factor,
               ColorSpace color_space,

gpu/ipc/service/image_transport_surface_overlay_mac.mm

@@ -82,15 +82,26 @@ bool ImageTransportSurfaceOverlayMac::Initialize(gl::GLSurfaceFormat format) {
   return true;
 }
 
+void ImageTransportSurfaceOverlayMac::PrepareToDestroy(bool have_context) {
+  if (!previous_frame_fence_)
+    return;
+  if (!have_context) {
+    // If we have no context, leak the GL objects, since we have no way to
+    // delete them.
+    DLOG(ERROR) << "Leaking GL fences.";
+    previous_frame_fence_.release();
+    return;
+  }
+  // Ensure we are using the context with which the fence was created.
+  DCHECK_EQ(fence_context_obj_, CGLGetCurrentContext());
+  CheckGLErrors("Before destroy fence");
+  previous_frame_fence_.reset();
+  CheckGLErrors("After destroy fence");
+}
+
 void ImageTransportSurfaceOverlayMac::Destroy() {
   ca_layer_tree_coordinator_.reset();
-  if (previous_frame_fence_) {
-    // Ensure we are using the context with which the fence was created.
-    gl::ScopedCGLSetCurrentContext scoped_set_current(fence_context_obj_);
-    CheckGLErrors("Before destroy fence");
-    previous_frame_fence_.reset();
-    CheckGLErrors("After destroy fence");
-  }
+  DCHECK(!previous_frame_fence_);
 }
 
 bool ImageTransportSurfaceOverlayMac::IsOffscreen() {

ui/gl/gl_surface.cc

@@ -36,6 +36,8 @@ bool GLSurface::Initialize(GLSurfaceFormat format) {
   return true;
 }
 
+void GLSurface::PrepareToDestroy(bool have_context) {}
+
 bool GLSurface::Resize(const gfx::Size& size,
                        float scale_factor,
                        ColorSpace color_space,

ui/gl/gl_surface.h

@@ -58,6 +58,12 @@ class GL_EXPORT GLSurface : public base::RefCounted<GLSurface> {
   // Destroys the surface.
   virtual void Destroy() = 0;
 
+  // Some implementations (macOS), in Destroy, will need to delete GL objects
+  // that exist in the current GL context. This method is called before the
+  // context's decoder (and potentially context itself) are destroyed, giving an
+  // opportunity for this cleanup.
+  virtual void PrepareToDestroy(bool have_context);
+
   // Color spaces that can be dynamically specified to the surface when resized.
   enum class ColorSpace {
     UNSPECIFIED,