Workaround to prevent crashes when destroying CGL contexts

content/browser/compositor/io_surface_layer_mac.mm

Index: content/browser/compositor/io_surface_layer_mac.mm
diff --git a/content/browser/compositor/io_surface_layer_mac.mm b/content/browser/compositor/io_surface_layer_mac.mm
index c0ea9e156593cb988f1c42796385db1701760bb4..ff3368b6109ffd4ec7070b175907817a1f5ac48c 100644
--- a/content/browser/compositor/io_surface_layer_mac.mm
+++ b/content/browser/compositor/io_surface_layer_mac.mm
@@ -27,20 +27,42 @@
     LOG_IF(ERROR, gl_error != GL_NO_ERROR) << "GL Error: " << gl_error; \
   } while (0)
 
-// Helper function for logging error codes.
 namespace {
+
+// Helper function for logging error codes.
 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;
 
@@ -113,7 +135,6 @@ void IOSurfaceLayerHelper::TimerFired() {
     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)];
@@ -170,8 +191,8 @@ void IOSurfaceLayerHelper::TimerFired() {
   // 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",
@@ -306,9 +327,7 @@ void IOSurfaceLayerHelper::TimerFired() {
 - (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];
 }
 
@@ -325,6 +344,8 @@ void IOSurfaceLayerHelper::TimerFired() {
 
   // 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(
@@ -332,12 +353,13 @@ void IOSurfaceLayerHelper::TimerFired() {
     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,
@@ -350,20 +372,20 @@ void IOSurfaceLayerHelper::TimerFired() {
         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
@@ -452,4 +474,30 @@ void IOSurfaceLayerHelper::TimerFired() {
   [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