Workaround to prevent crashes when destroying CGL contexts

content/browser/compositor/io_surface_layer_mac.mm

 // Copyright 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "content/browser/compositor/io_surface_layer_mac.h"
 
 #include <sstream>
 
 #include <CoreFoundation/CoreFoundation.h>
 #include <OpenGL/CGLIOSurface.h>
 #include <OpenGL/CGLRenderers.h>
 #include <OpenGL/OpenGL.h>
 
 #include "base/mac/mac_util.h"
 #include "base/mac/sdk_forward_declarations.h"
 #include "content/browser/gpu/gpu_data_manager_impl.h"
 #include "content/browser/renderer_host/render_widget_host_impl.h"
 #include "content/browser/renderer_host/render_widget_host_view_mac.h"
 #include "ui/base/cocoa/animation_utils.h"
 #include "ui/gfx/size_conversions.h"
 #include "ui/gl/scoped_cgl.h"
 #include "ui/gl/gpu_switching_manager.h"
 
 // Convenience macro for checking errors in the below code.
 #define CHECK_GL_ERROR() do {                                           \
     GLenum gl_error = glGetError();                                     \
     LOG_IF(ERROR, gl_error != GL_NO_ERROR) << "GL Error: " << gl_error; \
   } while (0)
 
+namespace {
+
 // Helper function for logging error codes.
-namespace {
 template<typename T>
 std::string to_string(T value) {
   std::ostringstream stream;
   stream << value;
   return stream.str();
 }
+
+// Helper function posted to destroy textures on a clean stack.
+void DestroyTexture(
+    base::ScopedTypeRef<CGLContextObj> context,
+    base::ScopedCFTypeRef<IOSurfaceRef> io_surface,
+    GLuint texture) {
+  // Destroy the texture before tearing down the context, and while the
+  // IOSurface is still being kept around.
+  CGLSetCurrentContext(context);
+  glDeleteTextures(1, &texture);
+  CGLSetCurrentContext(NULL);
+
+  // Release (and likely destroy) the context.
+  context.reset();
+
+  // Finally release down the IOSurface.

[Ken Russell (switch to Gerrit), 2014/09/10 01:04:08]

"release the IOSurface"

[ccameron, 2014/09/10 01:30:03]

Done.

+  io_surface.reset();
+}
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // IOSurfaceLayer(Private)
 
 @interface IOSurfaceLayer(Private)
+// Reset the texture and post a task to clean it up.
+- (void)resetTextureAndPostDestroy;
+
 // Force a draw immediately, but only if one was requested.
 - (void)displayIfNeededAndAck;
 
 // Called  when it has been a fixed interval of time and a frame has yet to be
 // drawn.
 - (void)timerFired;
 @end
 
 ////////////////////////////////////////////////////////////////////////////////
 // IOSurfaceLayerHelper
@@ skipping 52 matching lines @@
 - (id)initWithClient:(content::IOSurfaceLayerClient*)client
      withScaleFactor:(float)scale_factor {
   if (self = [super init]) {
     client_ = client;
     helper_.reset(new content::IOSurfaceLayerHelper(self));
     needs_display_ = false;
     has_pending_frame_ = false;
     did_not_draw_counter_ = 0;
     is_pumping_frames_ = false;
     io_surface_texture_ = 0;
-    io_surface_texture_dirty_ = false;
     cgl_renderer_id_ = 0;
 
     [self setBackgroundColor:CGColorGetConstantColor(kCGColorWhite)];
     [self setAnchorPoint:CGPointMake(0, 0)];
     // Setting contents gravity is necessary to prevent the layer from being
     // scaled during dyanmic resizes (especially with devtools open).
     [self setContentsGravity:kCAGravityTopLeft];
     if ([self respondsToSelector:(@selector(setContentsScale:))])
       [self setContentsScale:scale_factor];
   }
@@ skipping 36 matching lines @@
   has_pending_frame_ = true;
   needs_display_ = true;
   helper_->ResetTimer();
 
   frame_pixel_size_ = pixel_size;
 
   // If this is a new IOSurface, open the IOSurface and mark that the
   // GL texture needs to bind to the new surface.
   if (!io_surface_ || io_surface_id != IOSurfaceGetID(io_surface_)) {
     io_surface_.reset(IOSurfaceLookup(io_surface_id));
-    io_surface_texture_dirty_ = true;
     if (!io_surface_) {
+      [self resetTextureAndPostDestroy];
       content::GpuDataManagerImpl::GetInstance()->AddLogMessage(
           logging::LOG_ERROR,
           "IOSurfaceLayer",
           "Failed to open IOSurface in gotFrameWithIOSurface");
       if (client_)
         client_->IOSurfaceLayerHitError();
     }
   }
 
   // If reqested, draw immediately and don't bother trying to use the
@@ skipping 114 matching lines @@
         std::string("Failed to create pixel format object with CGL error ") +
             to_string(static_cast<int>(cgl_error)));
     return NULL;
   }
   return CGLRetainPixelFormat(pixel_format);
 }
 
 - (void)releaseCGLContext:(CGLContextObj)glContext {
   // The GL context is being destroyed, so mark the resources as needing to be
   // recreated.
-  io_surface_texture_ = 0;
-  io_surface_texture_dirty_ = true;
-  cgl_renderer_id_ = 0;
+  [self resetTextureAndPostDestroy];
   [super releaseCGLContext:glContext];
 }
 
 - (void)setNeedsDisplay {
   needs_display_ = true;
   [super setNeedsDisplay];
 }
 
 - (void)drawInCGLContext:(CGLContextObj)glContext
              pixelFormat:(CGLPixelFormatObj)pixelFormat
             forLayerTime:(CFTimeInterval)timeInterval
              displayTime:(const CVTimeStamp*)timeStamp {
   TRACE_EVENT0("browser", "IOSurfaceLayer::drawInCGLContext");
 
   // Create the texture if it has not been created in this context yet.
   if (!io_surface_texture_) {
+    DCHECK(!io_surface_texture_io_surface_);
+    DCHECK(!io_surface_texture_context_);
     glGenTextures(1, &io_surface_texture_);
     glBindTexture(GL_TEXTURE_RECTANGLE_ARB, io_surface_texture_);
     glTexParameteri(
         GL_TEXTURE_RECTANGLE_ARB, GL_TEXTURE_MIN_FILTER, GL_NEAREST);
     glTexParameteri(
         GL_TEXTURE_RECTANGLE_ARB, GL_TEXTURE_MAG_FILTER, GL_NEAREST);
     glBindTexture(GL_TEXTURE_RECTANGLE_ARB, 0);
-    io_surface_texture_dirty_ = true;
+    io_surface_texture_context_.reset(CGLRetainContext(glContext));
   }
+  DCHECK(io_surface_texture_context_ == glContext);
 
   // Associate the IOSurface with this texture, if the underlying IOSurface has
   // been changed.
-  if (io_surface_texture_dirty_ && io_surface_) {
+  if (io_surface_ != io_surface_texture_io_surface_) {
     glBindTexture(GL_TEXTURE_RECTANGLE_ARB, io_surface_texture_);
     CGLError cgl_error = CGLTexImageIOSurface2D(
         glContext,
         GL_TEXTURE_RECTANGLE_ARB,
         GL_RGBA,
         IOSurfaceGetWidth(io_surface_),
         IOSurfaceGetHeight(io_surface_),
         GL_BGRA,
         GL_UNSIGNED_INT_8_8_8_8_REV,
         io_surface_.get(),
         0 /* plane */);
     glBindTexture(GL_TEXTURE_RECTANGLE_ARB, 0);
-    if (cgl_error != kCGLNoError) {
+    if (cgl_error == kCGLNoError) {
+      io_surface_texture_io_surface_ = io_surface_;
+    } else {
+      [self resetTextureAndPostDestroy];
       content::GpuDataManagerImpl::GetInstance()->AddLogMessage(
           logging::LOG_ERROR,
           "IOSurfaceLayer",
           std::string("CGLTexImageIOSurface2D failed with CGL error ") +
               to_string(cgl_error));
-      glDeleteTextures(1, &io_surface_texture_);
-      io_surface_texture_ = 0;
       if (client_)
         client_->IOSurfaceLayerHitError();
     }
-  } else if (io_surface_texture_) {
-    glDeleteTextures(1, &io_surface_texture_);
-    io_surface_texture_ = 0;
+  } else if (!io_surface_) {
+    [self resetTextureAndPostDestroy];
   }
 
   // Fill the viewport with the texture. The viewport must be smaller or equal
   // than the texture, because it is resized as frames arrive.
   if (io_surface_texture_) {
     GLint viewport[4];
     glGetIntegerv(GL_VIEWPORT, viewport);
     gfx::Size viewport_pixel_size(viewport[2], viewport[3]);
     DCHECK_LE(
         viewport_pixel_size.width(),
@@ skipping 68 matching lines @@
 
   needs_display_ = false;
   [super drawInCGLContext:glContext
               pixelFormat:pixelFormat
              forLayerTime:timeInterval
               displayTime:timeStamp];
 
   [self ackPendingFrame];
 }
 
+- (void)resetTextureAndPostDestroy {
+  if (!io_surface_texture_) {
+    DCHECK(!io_surface_texture_context_);
+    DCHECK(!io_surface_texture_io_surface_);
+    return;
+  }
+
+  // Post a task to clean up the texture on a clean stack.
+  if (io_surface_texture_context_)
+    CGLRetainContext(io_surface_texture_context_);
+  if (io_surface_texture_io_surface_)
+    CFRetain(io_surface_texture_io_surface_);

[Ken Russell (switch to Gerrit), 2014/09/10 01:04:08]

I'm dubious that these calls to CGLRetainContext and CFRetain are balanced
correctly. Since the Scoped[CF]TypeRefs are bound as pass-by-value, base::Bind()
should be creating a copy of the Scoped[CF]TypeRef objects, and thereby calling
CGLRetainContext/CFRetain in the copy constructor. The reset() calls in
DestroyTexture should then call CGLReleaseContext / CFRelease. Can you please
double-check this perhaps with the OpenGL Driver Monitor and make sure we aren't
leaking IOSurfaces (or check the reference counts of the objects manually in
DestroyTexture)?

[ccameron, 2014/09/10 01:30:02]

They're completely and embarrassingly wrong. Fixed.

+  base::MessageLoop::current()->PostTask(
+      FROM_HERE,
+      base::Bind(&DestroyTexture,
+                 io_surface_texture_context_,
+                 io_surface_texture_io_surface_,
+                 io_surface_texture_));
+
+  // Reset the texture state.
+  io_surface_texture_ = 0;
+  io_surface_texture_context_.reset();
+  io_surface_texture_io_surface_.reset();
+  cgl_renderer_id_ = 0;
+}
+
 @end