Speculative fix for sticky keys overlay crash.

ash/sticky_keys/sticky_keys_overlay.cc

 // Copyright 2014 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 "ash/sticky_keys/sticky_keys_overlay.h"
 
 #include "ash/shell.h"
 #include "ash/shell_window_ids.h"
 #include "ash/sticky_keys/sticky_keys_controller.h"
 #include "base/strings/string_util.h"
@@ skipping 85 matching lines @@
 
   SetEnabledColor(label_color);
   SetDisabledColor(label_color);
   SetFontList(font_list().DeriveWithStyle(style));
 }
 
 ///////////////////////////////////////////////////////////////////////////////
 //  StickyKeysOverlayView
 class StickyKeysOverlayView : public views::WidgetDelegateView {
  public:
+  // This object is not owned by the views hiearchy or by the widget. The
+  // StickyKeysOverlay is the only class that should create and destroy this
+  // view.
   StickyKeysOverlayView();
 
   ~StickyKeysOverlayView() override;
 
   // views::WidgetDelegateView overrides:
   void OnPaint(gfx::Canvas* canvas) override;
+  void DeleteDelegate() override;
 
   void SetKeyState(ui::EventFlags modifier, StickyKeyState state);
 
   StickyKeyState GetKeyState(ui::EventFlags modifier);
 
   void SetModifierVisible(ui::EventFlags modifier, bool visible);
   bool GetModifierVisible(ui::EventFlags modifier);
 
  private:
   void AddKeyLabel(ui::EventFlags modifier, const std::string& key_label);
 
   typedef std::map<ui::EventFlags, StickyKeyOverlayLabel*> ModifierLabelMap;
   ModifierLabelMap modifier_label_map_;
 
   DISALLOW_COPY_AND_ASSIGN(StickyKeysOverlayView);
 };
 
 StickyKeysOverlayView::StickyKeysOverlayView() {
+  // The parent StickyKeysOverlay object owns this view.
+  set_owned_by_client();

[James Cook, 2014/12/04 22:00:45]

Doesn't the WidgetDelegateView constructor do this for you?

[Tim Song, 2014/12/04 23:53:37]

I just realized that there is no reason to inherit from WidgetDelegateView
anymore, so this class will inherit directly from views::View.

+
   const gfx::Font& font =
       ui::ResourceBundle::GetSharedInstance().GetFont(kKeyLabelFontStyle);
   int font_size = font.GetFontSize();
   int font_padding = font.GetHeight() - font.GetBaseline();
 
   // Text should have a margin of 0.5 times the font size on each side, so
   // the spacing between two labels will be the same as the font size.
   int horizontal_spacing = font_size / 2;
   int vertical_spacing = font_size / 2 - font_padding;
   int child_spacing = font_size - 2 * font_padding;
@@ skipping 19 matching lines @@
 StickyKeysOverlayView::~StickyKeysOverlayView() {}
 
 void StickyKeysOverlayView::OnPaint(gfx::Canvas* canvas) {
   SkPaint paint;
   paint.setStyle(SkPaint::kFill_Style);
   paint.setColor(SkColorSetARGB(0xB3, 0x55, 0x55, 0x55));
   canvas->DrawRoundRect(GetLocalBounds(), 2, paint);
   views::WidgetDelegateView::OnPaint(canvas);
 }
 
+void StickyKeysOverlayView::DeleteDelegate() {
+  // This view object is owned by StickyKeysOverlay, so no-op here.
+}
+
 void StickyKeysOverlayView::SetKeyState(ui::EventFlags modifier,
                                         StickyKeyState state) {
   ModifierLabelMap::iterator it = modifier_label_map_.find(modifier);
   DCHECK(it != modifier_label_map_.end());
   if (it != modifier_label_map_.end()) {
     StickyKeyOverlayLabel* label = it->second;
     label->SetKeyState(state);
   }
 }
 
@@ skipping 25 matching lines @@
 
 ///////////////////////////////////////////////////////////////////////////////
 //  StickyKeysOverlay
 StickyKeysOverlay::StickyKeysOverlay()
     : is_visible_(false),
       overlay_view_(new StickyKeysOverlayView),
       widget_size_(overlay_view_->GetPreferredSize()) {
   views::Widget::InitParams params;
   params.type = views::Widget::InitParams::TYPE_POPUP;
   params.opacity = views::Widget::InitParams::TRANSLUCENT_WINDOW;
   params.ownership = views::Widget::InitParams::WIDGET_OWNS_NATIVE_WIDGET;

[Jun Mukai, 2014/12/04 21:58:54]

WIDGET_OWNS_NATIVE_WIDGET is not used usually.  I believe the standard way is:
- the default ownership
- this class observes the widget, and clears the field like overlay_widget_ at
OnWidgetDestroying()

[Tim Song, 2014/12/04 23:53:37]

I'm using WIDGET_OWNS_NATIVE_WIDGET primarily so if you turn off sticky keys, we
can clean up all the resources the feature was using. It seems more difficult to
do this without owning the widget.

[Jun Mukai, 2014/12/05 00:10:36]

I am still not so sure what's the problem.
That sounds like that's because StickyKeysOverlay owns the widget as scoped_ptr,
but I am suggesting it (and overlay_view_) don't have to be.

I feel they can be raw pointers, and StickyKeysOverlay's dtor can just call
overlay_widget_->Close() which cleans up all the resources used for that widget.
 Is there any specific reason to avoid that?

[Tim Song, 2014/12/05 01:03:14]

I guess my concern is that it's more code to do this if the class doesn't own
the widget. What is the benefit of passing ownership of the the widget?

[Jun Mukai, 2014/12/05 02:23:07]

The biggest benefit would be that you don't have to care about view ownership
anymore. You can remove the comments and you don't have to care about the order
of destruction of the fields in a class. Also I believe the code size would be
not so different, just a few lines at most.

[Tim Song, 2014/12/05 19:29:51]

I feel after being burned already, keeping ownership of both the widget and the
view bound to this object is the safest thing to do. It's not easy to reason
about the lifetime otherwise.

   params.accept_events = false;
   params.keep_on_top = true;
   params.remove_standard_frame = true;
-  params.delegate = overlay_view_;
+  params.delegate = overlay_view_.get();
   params.bounds = CalculateOverlayBounds();
   params.parent = Shell::GetContainer(Shell::GetTargetRootWindow(),
                                       kShellWindowId_OverlayContainer);
   overlay_widget_.reset(new views::Widget);
   overlay_widget_->Init(params);
   overlay_widget_->SetVisibilityChangedAnimationsEnabled(false);
-  overlay_widget_->SetContentsView(overlay_view_);
+  overlay_widget_->SetContentsView(overlay_view_.get());

[Jun Mukai, 2014/12/04 21:58:54]

Because overlay_view_ is a WidgetDelegateView, it should inherit
GetContentsView() to return itself instead of setting here.

[Tim Song, 2014/12/04 23:53:37]

Done. I stopped inheriting from WidgetDelegateView and inherit from View
directly.

   overlay_widget_->GetNativeView()->SetName("StickyKeysOverlay");
 }
 
 StickyKeysOverlay::~StickyKeysOverlay() {}
 
 void StickyKeysOverlay::Show(bool visible) {
   if (is_visible_ == visible)
     return;
 
   is_visible_ = visible;
@@ skipping 36 matching lines @@
 void StickyKeysOverlay::SetModifierKeyState(ui::EventFlags modifier,
                                             StickyKeyState state) {
   overlay_view_->SetKeyState(modifier, state);
 }
 
 StickyKeyState StickyKeysOverlay::GetModifierKeyState(
     ui::EventFlags modifier) {
   return overlay_view_->GetKeyState(modifier);
 }
 
+views::Widget* StickyKeysOverlay::GetWidgetForTesting() {
+  return overlay_widget_.get();
+}
+
 gfx::Rect StickyKeysOverlay::CalculateOverlayBounds() {
   int x = is_visible_ ? kHorizontalOverlayOffset : -widget_size_.width();
   return gfx::Rect(gfx::Point(x, kVerticalOverlayOffset), widget_size_);
 }
 
 void StickyKeysOverlay::OnLayerAnimationEnded(
     ui::LayerAnimationSequence* sequence) {
   ui::LayerAnimator* animator = overlay_widget_->GetLayer()->GetAnimator();
   if (animator)
     animator->RemoveObserver(this);
   if (!is_visible_)
     overlay_widget_->Hide();
 }
 
 void StickyKeysOverlay::OnLayerAnimationAborted(
     ui::LayerAnimationSequence* sequence) {
   ui::LayerAnimator* animator = overlay_widget_->GetLayer()->GetAnimator();
   if (animator)
     animator->RemoveObserver(this);
 }
 
 void StickyKeysOverlay::OnLayerAnimationScheduled(
     ui::LayerAnimationSequence* sequence) {
 }
 
 }  // namespace ash