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

ui/views/bubble/tray_bubble_view.cc

 // Copyright (c) 2012 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 "ui/views/bubble/tray_bubble_view.h"
 
 #include <algorithm>
 
 #include "third_party/skia/include/core/SkCanvas.h"
 #include "third_party/skia/include/core/SkColor.h"
@@ skipping 161 matching lines @@
 
 // This mask layer clips the bubble's content so that it does not overwrite the
 // rounded bubble corners.
 // TODO(miket): This does not work on Windows. Implement layer masking or
 // alternate solutions if the TrayBubbleView is needed there in the future.
 class TrayBubbleContentMask : public ui::LayerDelegate {
  public:
   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;
   virtual void OnDeviceScaleFactorChanged(float device_scale_factor) OVERRIDE;
   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);
   paint.setStyle(SkPaint::kFill_Style);
   canvas->DrawPath(path, paint);
 }
 
 void TrayBubbleContentMask::OnDeviceScaleFactorChanged(
     float device_scale_factor) {
   // Redrawing will take care of scale factor change.
@@ skipping 123 matching lines @@
   // Inform host items (models) that their views are being destroyed.
   if (delegate_)
     delegate_->BubbleViewDestroyed();
 }
 
 void TrayBubbleView::InitializeAndShowBubble() {
   // Must occur after call to BubbleDelegateView::CreateBubble().
   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();
 }
 
 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();
 }
 
 void TrayBubbleView::SetMaxHeight(int height) {
   params_.max_height = height;
   if (GetWidget())
     SizeToContents();
 }
 
 void TrayBubbleView::SetWidth(int width) {
@@ skipping 128 matching lines @@
     const ViewHierarchyChangedDetails& details) {
   if (get_use_acceleration_when_possible() && details.is_add &&
       details.child == this) {
     details.parent->SetPaintToLayer(true);
     details.parent->SetFillsBoundsOpaquely(true);
     details.parent->layer()->SetMasksToBounds(true);
   }
 }
 
 }  // namespace views