Speculative fix for sticky keys overlay crash.

The new test case reproduces the same stack trace as in the bug, so it's very
probable that this case is causing the crash.

BUG=435600

Committed: https://crrev.com/9391c0e45bc3ae50008d0aebf11437550e7f38c6
Cr-Commit-Position: refs/heads/master@{#306504}

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

Cr-Commit-Position: refs/heads/master@{#307140}

ash/root_window_controller.cc

@@ -162,7 +162,8 @@ void ReparentAllWindows(aura::Window* src, aura::Window* dst) {
       kShellWindowId_AlwaysOnTopContainer,
       kShellWindowId_SystemModalContainer,
       kShellWindowId_LockSystemModalContainer,
-      kShellWindowId_UnparentedControlContainer, };
+      kShellWindowId_UnparentedControlContainer,
+      kShellWindowId_OverlayContainer, };
   for (size_t i = 0; i < arraysize(kContainerIdsToMove); i++) {
     int id = kContainerIdsToMove[i];
     aura::Window* src_container = Shell::GetContainer(src, id);

ash/sticky_keys/sticky_keys_overlay.cc

@@ -101,13 +101,16 @@ void StickyKeyOverlayLabel::SetKeyState(StickyKeyState state) {
 
 ///////////////////////////////////////////////////////////////////////////////
 //  StickyKeysOverlayView
-class StickyKeysOverlayView : public views::WidgetDelegateView {
+class StickyKeysOverlayView : public views::View {
  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:
+  // views::View overrides:
   void OnPaint(gfx::Canvas* canvas) override;
 
   void SetKeyState(ui::EventFlags modifier, StickyKeyState state);
@@ -127,6 +130,9 @@ class StickyKeysOverlayView : public views::WidgetDelegateView {
 };
 
 StickyKeysOverlayView::StickyKeysOverlayView() {
+  // The parent StickyKeysOverlay object owns this view.
+  set_owned_by_client();
+
   const gfx::Font& font =
       ui::ResourceBundle::GetSharedInstance().GetFont(kKeyLabelFontStyle);
   int font_size = font.GetFontSize();
@@ -163,7 +169,7 @@ void StickyKeysOverlayView::OnPaint(gfx::Canvas* canvas) {
   paint.setStyle(SkPaint::kFill_Style);
   paint.setColor(SkColorSetARGB(0xB3, 0x55, 0x55, 0x55));
   canvas->DrawRoundRect(GetLocalBounds(), 2, paint);
-  views::WidgetDelegateView::OnPaint(canvas);
+  views::View::OnPaint(canvas);
 }
 
 void StickyKeysOverlayView::SetKeyState(ui::EventFlags modifier,
@@ -215,14 +221,13 @@ StickyKeysOverlay::StickyKeysOverlay()
   params.accept_events = false;
   params.keep_on_top = true;
   params.remove_standard_frame = true;
-  params.delegate = overlay_view_;
   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());
   overlay_widget_->GetNativeView()->SetName("StickyKeysOverlay");
 }
 
@@ -279,6 +284,10 @@ StickyKeyState StickyKeysOverlay::GetModifierKeyState(
   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_);

ash/sticky_keys/sticky_keys_overlay.h

@@ -51,6 +51,10 @@ class ASH_EXPORT StickyKeysOverlay : public ui::LayerAnimationObserver {
   // animating, the returned value is the target of the animation.
   bool is_visible() { return is_visible_; }
 
+  // Returns the underlying views::Widget for testing purposes. The returned
+  // widget is owned by StickyKeysOverlay.
+  views::Widget* GetWidgetForTesting();
+
  private:
   // Returns the current bounds of the overlay, which is based on visibility.
   gfx::Rect CalculateOverlayBounds();
@@ -62,8 +66,7 @@ class ASH_EXPORT StickyKeysOverlay : public ui::LayerAnimationObserver {
 
   bool is_visible_;
   scoped_ptr<views::Widget> overlay_widget_;
-  // Ownership of |overlay_view_| is passed to the view heirarchy.
-  StickyKeysOverlayView* overlay_view_;
+  scoped_ptr<StickyKeysOverlayView> overlay_view_;
   gfx::Size widget_size_;
 };
 

ash/sticky_keys/sticky_keys_overlay_unittest.cc

@@ -4,18 +4,17 @@
 
 #include "ash/sticky_keys/sticky_keys_overlay.h"
 
+#include "ash/display/display_controller.h"
+#include "ash/display/display_manager.h"
 #include "ash/shell.h"
 #include "ash/sticky_keys/sticky_keys_controller.h"
 #include "ash/test/ash_test_base.h"
 #include "ui/events/event.h"
+#include "ui/views/widget/widget.h"
 
 namespace ash {
 
-class StickyKeysOverlayTest : public test::AshTestBase {
- public:
-  StickyKeysOverlayTest() {}
-  virtual ~StickyKeysOverlayTest() {}
-};
+using StickyKeysOverlayTest = test::AshTestBase;
 
 TEST_F(StickyKeysOverlayTest, OverlayVisibility) {
   StickyKeysOverlay overlay;
@@ -41,6 +40,40 @@ TEST_F(StickyKeysOverlayTest, ModifierKeyState) {
             overlay.GetModifierKeyState(ui::EF_COMMAND_DOWN));
 }
 
+// This test addresses the crash report at crbug.com/435600, speculated to be
+// caused by using sticky keys with multiple displays.
+TEST_F(StickyKeysOverlayTest, OverlayNotDestroyedAfterDisplayRemoved) {
+  // Add a secondary display to the left of the primary one.
+  UpdateDisplay("1280x1024,1980x1080");
+  DisplayManager* display_manager = Shell::GetInstance()->display_manager();
+  DisplayIdPair display_ids = display_manager->GetCurrentDisplayIdPair();
+  int64_t primary_display_id = display_ids.first;
+  int64_t secondary_display_id = display_ids.second;
+  display_manager->SetLayoutForCurrentDisplays(
+      DisplayLayout(DisplayLayout::LEFT, 0));
+
+  // The overlay should belong to the secondary root window.
+  StickyKeysOverlay overlay;
+  views::Widget* overlay_widget = overlay.GetWidgetForTesting();
+  DisplayController* display_controller =
+      Shell::GetInstance()->display_controller();
+  EXPECT_EQ(display_controller->GetRootWindowForDisplayId(secondary_display_id),
+            overlay_widget->GetNativeWindow()->GetRootWindow());
+
+  // Removing the second display should move the overlay to the primary root
+  // window.
+  UpdateDisplay("1280x1024");
+  EXPECT_EQ(display_controller->GetRootWindowForDisplayId(primary_display_id),
+            overlay_widget->GetNativeWindow()->GetRootWindow());
+
+  overlay.SetModifierKeyState(ui::EF_SHIFT_DOWN, STICKY_KEY_STATE_ENABLED);
+  EXPECT_EQ(STICKY_KEY_STATE_ENABLED,
+            overlay.GetModifierKeyState(ui::EF_SHIFT_DOWN));
+  overlay.SetModifierKeyState(ui::EF_SHIFT_DOWN, STICKY_KEY_STATE_DISABLED);
+  EXPECT_EQ(STICKY_KEY_STATE_DISABLED,
+            overlay.GetModifierKeyState(ui::EF_SHIFT_DOWN));
+}
+
 // Additional sticky key overlay tests that depend on chromeos::EventRewriter
 // are now in chrome/browser/chromeos/events/event_rewriter_unittest.cc .