Fix crash in GotAcceleratedIOSurfaceFrame

content/browser/compositor/browser_compositor_view_private_mac.mm

Index: content/browser/compositor/browser_compositor_view_private_mac.mm
diff --git a/content/browser/compositor/browser_compositor_view_private_mac.mm b/content/browser/compositor/browser_compositor_view_private_mac.mm
index cd6ed66cc6d539eb944ad2e7eadfff6f07b4b376..8a1b392484dce56907f13a28a1910d08fd42cae8 100644
--- a/content/browser/compositor/browser_compositor_view_private_mac.mm
+++ b/content/browser/compositor/browser_compositor_view_private_mac.mm
@@ -132,7 +132,7 @@ void BrowserCompositorViewMacInternal::GotAcceleratedFrame(
 
   // If there is no client and therefore no superview to draw into, early-out.
   if (!client_) {
-    AcceleratedLayerDidDrawFrame(true);
+    AcceleratedLayerDidDrawFrame();
     return;
   }
 
@@ -178,23 +178,16 @@ void BrowserCompositorViewMacInternal::GotAcceleratedCAContextFrame(
 
   // Acknowledge the frame to unblock the compositor immediately (the GPU
   // process will do any required throttling).
-  AcceleratedLayerDidDrawFrame(true);
+  AcceleratedLayerDidDrawFrame();
 
   // If this replacing a same-type layer, remove it now that the new layer is
   // in the hierarchy.
   if (old_ca_context_layer != ca_context_layer_)
-    [old_ca_context_layer removeFromSuperlayer];
+    DestroyCAContextLayer(old_ca_context_layer);
 
   // Remove any different-type layers that this is replacing.
-  if (io_surface_layer_) {
-    [io_surface_layer_ resetClient];
-    [io_surface_layer_ removeFromSuperlayer];
-    io_surface_layer_.reset();
-  }
-  if (software_layer_) {
-    [software_layer_ removeFromSuperlayer];
-    software_layer_.reset();
-  }
+  DestroyIOSurfaceLayer(io_surface_layer_);
+  DestroySoftwareLayer();
 }
 
 void BrowserCompositorViewMacInternal::GotAcceleratedIOSurfaceFrame(
@@ -216,51 +209,64 @@ void BrowserCompositorViewMacInternal::GotAcceleratedIOSurfaceFrame(
   if (needs_new_layer) {
     scoped_refptr<content::CompositingIOSurfaceMac> iosurface =
         content::CompositingIOSurfaceMac::Create();
-    io_surface_layer_.reset([[CompositingIOSurfaceLayer alloc]
-        initWithIOSurface:iosurface
-          withScaleFactor:scale_factor
-               withClient:this]);
-    [flipped_layer_ addSublayer:io_surface_layer_];
+    if (!iosurface) {
+      LOG(ERROR) << "Failed to create CompositingIOSurfaceMac";
+    } else {
+      io_surface_layer_.reset([[CompositingIOSurfaceLayer alloc]
+          initWithIOSurface:iosurface
+            withScaleFactor:scale_factor
+                 withClient:this]);
+      if (io_surface_layer_)
+        [flipped_layer_ addSublayer:io_surface_layer_];
+      else
+        LOG(ERROR) << "Failed to create CompositingIOSurfaceLayer";
+    }
   }
 
   // Open the provided IOSurface.
-  {
+  if (io_surface_layer_) {
     bool result = true;
     gfx::ScopedCGLSetCurrentContext scoped_set_current_context(
         [io_surface_layer_ context]->cgl_context());
     result = [io_surface_layer_ iosurface]->SetIOSurfaceWithContextCurrent(
         [io_surface_layer_ context], io_surface_id, pixel_size, scale_factor);
-    if (!result)
-      LOG(ERROR) << "Failed SetIOSurface on CompositingIOSurfaceMac";
+    if (!result) {
+      DestroyIOSurfaceLayer(io_surface_layer_);
+      LOG(ERROR) << "Failed open IOSurface in CompositingIOSurfaceLayer";
+    }
+  }
+
+  // Give a final complaint if anything with the layer's creation went wrong.
+  // This frame will appear blank, the compositor will try to create another,
+  // and maybe that will go better.
+  if (!io_surface_layer_) {
+    LOG(ERROR) << "CompositingIOSurfaceLayer is nil, tab will be blank";
+    AcceleratedLayerHitError();
+  }
+
+  // Make the CALayer draw and set its size appropriately.
+  if (io_surface_layer_) {
+    [io_surface_layer_ gotNewFrame];
+
+    // Set the bounds of the accelerated layer to match the size of the frame.
+    // If the bounds changed, force the content to be displayed immediately.
+    CGRect new_layer_bounds = CGRectMake(
+        0, 0, last_swap_size_dip_.width(), last_swap_size_dip_.height());
+    bool bounds_changed = !CGRectEqualToRect(
+        new_layer_bounds, [io_surface_layer_ bounds]);
+    [io_surface_layer_ setBounds:new_layer_bounds];
+    if (bounds_changed)
+      [io_surface_layer_ setNeedsDisplayAndDisplayAndAck];
   }
-  [io_surface_layer_ gotNewFrame];
-
-  // Set the bounds of the accelerated layer to match the size of the frame.
-  // If the bounds changed, force the content to be displayed immediately.
-  CGRect new_layer_bounds = CGRectMake(
-      0, 0, last_swap_size_dip_.width(), last_swap_size_dip_.height());
-  bool bounds_changed = !CGRectEqualToRect(
-      new_layer_bounds, [io_surface_layer_ bounds]);
-  [io_surface_layer_ setBounds:new_layer_bounds];
-  if (bounds_changed)
-    [io_surface_layer_ setNeedsDisplayAndDisplayAndAck];
 
   // If this replacing a same-type layer, remove it now that the new layer is
   // in the hierarchy.
-  if (old_io_surface_layer != io_surface_layer_) {
-    [old_io_surface_layer resetClient];
-    [old_io_surface_layer removeFromSuperlayer];
-  }
+  if (old_io_surface_layer != io_surface_layer_)
+    DestroyIOSurfaceLayer(old_io_surface_layer);
 
   // Remove any different-type layers that this is replacing.
-  if (ca_context_layer_) {
-    [ca_context_layer_ removeFromSuperlayer];
-    ca_context_layer_.reset();
-  }
-  if (software_layer_) {
-    [software_layer_ removeFromSuperlayer];
-    software_layer_.reset();
-  }
+  DestroyCAContextLayer(ca_context_layer_);
+  DestroySoftwareLayer();
 }
 
 void BrowserCompositorViewMacInternal::GotSoftwareFrame(
@@ -291,15 +297,34 @@ void BrowserCompositorViewMacInternal::GotSoftwareFrame(
   last_swap_size_dip_ = ConvertSizeToDIP(scale_factor, pixel_size);
 
   // Remove any different-type layers that this is replacing.
-  if (ca_context_layer_) {
-    [ca_context_layer_ removeFromSuperlayer];
+  DestroyCAContextLayer(ca_context_layer_);
+  DestroyIOSurfaceLayer(io_surface_layer_);
+}
+
+void BrowserCompositorViewMacInternal::DestroyCAContextLayer(
+    base::scoped_nsobject<CALayerHost> ca_context_layer) {
+  if (!ca_context_layer)
+    return;
+  [ca_context_layer removeFromSuperlayer];
+  if (ca_context_layer == ca_context_layer_)
     ca_context_layer_.reset();
-  }
-  if (io_surface_layer_) {
-    [io_surface_layer_ resetClient];
-    [io_surface_layer_ removeFromSuperlayer];
+}
+
+void BrowserCompositorViewMacInternal::DestroyIOSurfaceLayer(
+    base::scoped_nsobject<CompositingIOSurfaceLayer> io_surface_layer) {
+  if (!io_surface_layer)
+    return;
+  [io_surface_layer resetClient];
+  [io_surface_layer removeFromSuperlayer];
+  if (io_surface_layer == io_surface_layer_)
     io_surface_layer_.reset();
-  }
+}
+
+void BrowserCompositorViewMacInternal::DestroySoftwareLayer() {
+  if (!software_layer_)
+    return;
+  [software_layer_ removeFromSuperlayer];
+  software_layer_.reset();
 }
 
 bool BrowserCompositorViewMacInternal::AcceleratedLayerShouldAckImmediately()
@@ -311,8 +336,7 @@ bool BrowserCompositorViewMacInternal::AcceleratedLayerShouldAckImmediately()
   return client_->BrowserCompositorViewShouldAckImmediately();
 }
 
-void BrowserCompositorViewMacInternal::AcceleratedLayerDidDrawFrame(
-    bool succeeded) {
+void BrowserCompositorViewMacInternal::AcceleratedLayerDidDrawFrame() {
   if (accelerated_output_surface_id_) {
     content::ImageTransportFactory::GetInstance()->OnSurfaceDisplayed(
         accelerated_output_surface_id_);
@@ -323,12 +347,17 @@ void BrowserCompositorViewMacInternal::AcceleratedLayerDidDrawFrame(
     client_->BrowserCompositorViewFrameSwapped(accelerated_latency_info_);
 
   accelerated_latency_info_.clear();
+}
 
-  if (!succeeded) {
-    if (io_surface_layer_)
-      [io_surface_layer_ context]->PoisonContextAndSharegroup();
-    compositor_->ScheduleFullRedraw();
-  }
+void BrowserCompositorViewMacInternal::AcceleratedLayerHitError() {
+  // Perform all acks that would have been done if the frame had succeeded, to
+  // un-block the compositor and renderer.
+  AcceleratedLayerDidDrawFrame();
+
+  // Poison the context being used and request a mulligan.
+  if (io_surface_layer_)
+    [io_surface_layer_ context]->PoisonContextAndSharegroup();
+  compositor_->ScheduleFullRedraw();
 }
 
 // static