Retain tray bubble's rounded corners when the bubble animates out

ui/views/bubble/tray_bubble_view.cc

Index: ui/views/bubble/tray_bubble_view.cc
diff --git a/ui/views/bubble/tray_bubble_view.cc b/ui/views/bubble/tray_bubble_view.cc
index 8c402fea06b3dd35b808204e5db4dc84ea0a88d2..7fc5451447a64af5f72fd5bbf8bee08e9ddf7dd4 100644
--- a/ui/views/bubble/tray_bubble_view.cc
+++ b/ui/views/bubble/tray_bubble_view.cc
@@ -179,7 +179,7 @@ class TrayBubbleContentMask : public ui::LayerDelegate {
   explicit TrayBubbleContentMask(int corner_radius);
   virtual ~TrayBubbleContentMask();
 
-  ui::Layer* layer() { return &layer_; }
+  void set_layer(ui::Layer* layer) { layer_ = layer; }
 
   // Overridden from LayerDelegate.
   virtual void OnPaintLayer(gfx::Canvas* canvas) OVERRIDE;
@@ -187,25 +187,25 @@ class TrayBubbleContentMask : public ui::LayerDelegate {
   virtual base::Closure PrepareForLayerBoundsChange() OVERRIDE;
 
  private:
-  ui::Layer layer_;
+  ui::Layer* layer_;

[Jun Mukai, 2013/10/05 01:23:50]

one more notes:
the original layer of the TrayBubbleView can be recreated through
RecreateLayer(), and the caller maintains the ownership of the old layer.
That means that the lifetime of |layer_| can be unrelated to this class and
potentially cause a use-after-free.

Right now, this RecreateLayer is used for the disappearing animation of
TrayBubbleView so TrayBubbleContentMask is removed earlier than the layer, but
there is no guarantee.

For example:
ui::Layer* foo = tray_bubble->RecreateLayer();
// some process
delete foo;
tray_bubble->SizeToContents();

this code is legal and causes a use-after-free with your code (because |layer_|
is owned by foo).

This type of recreating layer is used to show the window on both displays during
the drag-move across the displays.

--

Of course a tray-bubble is tied to the shelf of a display and no
'drag-across-the-displays' situation happens right now. However, I think it's
still better to take that into account. One idea is to remove |layer_| pointer
to guarantee use-after-free can't happen. Another idea is to add a new method
such like 'OnLayerRecreated()' to views::View and update |layer_| properly at
that point.

[michaelpg, 2013/12/10 02:04:03]

Yes. I've removed the layer_ pointer. I believe it doesn't matter any more which
object(s) are deleted first.

   SkScalar corner_radius_;
 
   DISALLOW_COPY_AND_ASSIGN(TrayBubbleContentMask);
 };
 
 TrayBubbleContentMask::TrayBubbleContentMask(int corner_radius)
-    : layer_(ui::LAYER_TEXTURED),
-      corner_radius_(corner_radius) {
-  layer_.set_delegate(this);
+  : layer_(NULL),
+    corner_radius_(corner_radius) {
 }
 
 TrayBubbleContentMask::~TrayBubbleContentMask() {
-  layer_.set_delegate(NULL);
+  if (layer_)
+    layer_->set_delegate(NULL);
 }
 
 void TrayBubbleContentMask::OnPaintLayer(gfx::Canvas* canvas) {
   SkPath path;
-  path.addRoundRect(gfx::RectToSkRect(gfx::Rect(layer()->bounds().size())),
+  path.addRoundRect(gfx::RectToSkRect(gfx::Rect(layer_->bounds().size())),
                     corner_radius_, corner_radius_);
   SkPaint paint;
   paint.setAlpha(255);
@@ -349,8 +349,15 @@ void TrayBubbleView::InitializeAndShowBubble() {
   SetAlignment(params_.arrow_alignment);
   bubble_border_->UpdateArrowOffset();
 
-  if (get_use_acceleration_when_possible())
-    layer()->parent()->SetMaskLayer(bubble_content_mask_->layer());
+  if (get_use_acceleration_when_possible()) {
+    ui::Layer* mask_layer = new ui::Layer(ui::LAYER_TEXTURED);
+
+    // Set up the TrayBubbleContentMask as the mask layer's delegate.
+    bubble_content_mask_->set_layer(mask_layer);
+    mask_layer->set_delegate(bubble_content_mask_.get());

[piman, 2013/10/04 18:05:33]

nit: I would do this in TrayBubbleContentMask::set_layer, which is more
symmetrical since the destructor does reset the delegate.

TrayBubbleContentMask::SetLayer(ui::Layer* layer) {
  DCHECK(layer);
  DCHECK(!layer_);  // alternatively, if (layer_) layer_->set_delegate(NULL);
  layer_ = layer;
  layer_->set_delegate(this);
}

[michaelpg, 2013/12/10 02:04:03]

I'm keeping the actual layer outside of TrayBubbleContentMask and just using it
as the delegate to paint the mask. I've also made TrayBubbleView reset the
delegate now.

+
+    layer()->parent()->SetMaskLayer(scoped_ptr<ui::Layer>(mask_layer));
+  }
 
   GetWidget()->Show();
   UpdateBubble();
@@ -359,7 +366,7 @@ void TrayBubbleView::InitializeAndShowBubble() {
 void TrayBubbleView::UpdateBubble() {
   SizeToContents();
   if (get_use_acceleration_when_possible())
-    bubble_content_mask_->layer()->SetBounds(layer()->bounds());
+    layer()->parent()->layer_mask_layer()->SetBounds(layer()->bounds());
   GetWidget()->GetRootView()->SchedulePaint();
 }