Fix crash in GotAcceleratedIOSurfaceFrame

The root cause here is that it was assumed that creation of a
CompositingIOSurfaceMac and CompositingIOSurfaceContext
cannot not fail when, in fact, it can. This resulted in a NULL
reference later on.

Fix part of this by making CompositingIOSurfaceLayer's init function
fail if the CompositingIOSurfaceContext fails.

Fix the remaining part of this by removing the NULL references from
BrowserCompositorViewMacInternal::GotAcceleratedIOSurfaceFrame,
and make the function call AcceleratedLayerHitError if any errors are
encountered, which will cause the layer to re-attempt the frame with
a new context.

Also, split AcceleratedLayerDidDrawFrame into two functions,
AcceleratedLayerDidDrawFrame and AcceleratedLayerHitError, rather
than parameterizing success using a bool, since that naming was
deceptive.

BUG=401630

Review URL: https://codereview.chromium.org/447113004

Cr-Commit-Position: refs/heads/master@{#288328}
git-svn-id: svn://svn.chromium.org/chrome/trunk/src@288328 0039d316-1c4b-4281-b951-d872f2087c98

content/browser/compositor/browser_compositor_view_private_mac.h

@@ -49,7 +49,8 @@ class BrowserCompositorViewMacInternal
 private:
   // CompositingIOSurfaceLayerClient implementation:
   virtual bool AcceleratedLayerShouldAckImmediately() const OVERRIDE;
-  virtual void AcceleratedLayerDidDrawFrame(bool succeeded) OVERRIDE;
+  virtual void AcceleratedLayerDidDrawFrame() OVERRIDE;
+  virtual void AcceleratedLayerHitError() OVERRIDE;
 
   void GotAcceleratedCAContextFrame(
       CAContextID ca_context_id, gfx::Size pixel_size, float scale_factor);
@@ -57,6 +58,16 @@ private:
   void GotAcceleratedIOSurfaceFrame(
       IOSurfaceID io_surface_id, gfx::Size pixel_size, float scale_factor);
 
+  // Remove a layer from the heirarchy and destroy it. Because the accelerated
+  // layer types may be replaced by a layer of the same type, the layer to
+  // destroy is parameterized, and, if it is the current layer, the current
+  // layer is reset.
+  void DestroyCAContextLayer(
+      base::scoped_nsobject<CALayerHost> ca_context_layer);
+  void DestroyIOSurfaceLayer(
+      base::scoped_nsobject<CompositingIOSurfaceLayer> io_surface_layer);
+  void DestroySoftwareLayer();
+
   // The client of the BrowserCompositorViewMac that is using this as its
   // internals.
   BrowserCompositorViewMacClient* client_;

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

content/browser/renderer_host/compositing_iosurface_layer_mac.h

@@ -22,8 +22,17 @@ class CompositingIOSurfaceContext;
 // BrowserCompositorViewMac).
 class CompositingIOSurfaceLayerClient {
  public:
+  // Used to indicate that the layer should attempt to draw immediately and
+  // should (even if the draw is elided by the system), ack the frame
+  // immediately.
   virtual bool AcceleratedLayerShouldAckImmediately() const = 0;
-  virtual void AcceleratedLayerDidDrawFrame(bool succeeded) = 0;
+
+  // Called when a frame is drawn or when, because the layer is not visible, it
+  // is known that the frame will never drawn.
+  virtual void AcceleratedLayerDidDrawFrame() = 0;
+
+  // Called when an error prevents the frame from being drawn.
+  virtual void AcceleratedLayerHitError() = 0;
 };
 
 // CompositingIOSurfaceLayerHelper provides C++ functionality needed for the

content/browser/renderer_host/compositing_iosurface_layer_mac.mm

@@ -102,7 +102,10 @@ void CompositingIOSurfaceLayerHelper::AckPendingFrame(bool success) {
   if (!has_pending_frame_)
     return;
   has_pending_frame_ = false;
-  client_->AcceleratedLayerDidDrawFrame(success);
+  if (success)
+    client_->AcceleratedLayerDidDrawFrame();
+  else
+    client_->AcceleratedLayerHitError();
   // A trace value of 0 indicates that there is no longer a pending swap ack.
   TRACE_COUNTER_ID1("browser", "PendingSwapAck", this, 0);
 }
@@ -174,13 +177,19 @@ void CompositingIOSurfaceLayerHelper::EndPumpingFrames() {
                             iosurface
         withScaleFactor:(float)scale_factor
              withClient:(content::CompositingIOSurfaceLayerClient*)client {
+  DCHECK(iosurface);
   if (self = [super init]) {
     helper_.reset(new content::CompositingIOSurfaceLayerHelper(client, self));
 
     iosurface_ = iosurface;
     context_ = content::CompositingIOSurfaceContext::Get(
         content::CompositingIOSurfaceContext::kCALayerContextWindowNumber);
-    DCHECK(context_);
+    if (!context_) {
+      LOG(ERROR) << "Failed create CompositingIOSurfaceContext";
+      [self resetClient];
+      [self release];
+      return nil;
+    }
 
     [self setBackgroundColor:CGColorGetConstantColor(kCGColorWhite)];
     [self setAnchorPoint:CGPointMake(0, 0)];

content/browser/renderer_host/render_widget_host_view_mac.h

@@ -351,7 +351,8 @@ class CONTENT_EXPORT RenderWidgetHostViewMac
 
   // CompositingIOSurfaceLayerClient implementation.
   virtual bool AcceleratedLayerShouldAckImmediately() const OVERRIDE;
-  virtual void AcceleratedLayerDidDrawFrame(bool succeeded) OVERRIDE;
+  virtual void AcceleratedLayerDidDrawFrame() OVERRIDE;
+  virtual void AcceleratedLayerHitError() OVERRIDE;
 
   // gfx::DisplayObserver implementation.
   virtual void OnDisplayAdded(const gfx::Display& new_display) OVERRIDE;

content/browser/renderer_host/render_widget_host_view_mac.mm

@@ -2314,14 +2314,21 @@ bool RenderWidgetHostViewMac::AcceleratedLayerShouldAckImmediately() const {
   return false;
 }
 
-void RenderWidgetHostViewMac::AcceleratedLayerDidDrawFrame(bool succeeded) {
+void RenderWidgetHostViewMac::AcceleratedLayerDidDrawFrame() {
   if (!render_widget_host_)
     return;
 
   SendPendingLatencyInfoToHost();
   SendPendingSwapAck();
-  if (!succeeded)
-    GotAcceleratedCompositingError();
+}
+
+void RenderWidgetHostViewMac::AcceleratedLayerHitError() {
+  if (!render_widget_host_)
+    return;
+  // Perform all acks that would have been done if the frame had succeeded, to
+  // un-block the renderer.
+  AcceleratedLayerDidDrawFrame();
+  GotAcceleratedCompositingError();
 }
 
 ////////////////////////////////////////////////////////////////////////////////