Fix the crash of View::OnTouchEvent in the login screen

LoginExpandedPublicAccountView is a view subclass but also
a pretarget handler of Shell. This is not great since any of
touch events will invoke the final method View::OnTouchEvent().

This CL creates an event handler for this purpose rather than
using a view directly as a pretarget handler.

I expanded the test cases to examine both mouse events and touches,
and confirmed it causes the reported crashes with touches without
this fix.

Bug: 908890
Test: the new test cases
Change-Id: I45d7de7da1eca8f874f5774971a085af8f72c9eb
Reviewed-on: https://chromium-review.googlesource.com/c/1428607Reviewed-by: Jacob Dufault <jdufault@chromium.org>
Commit-Queue: Jun Mukai <mukai@chromium.org>
Cr-Commit-Position: refs/heads/master@{#625315}

ash/login/ui/login_expanded_public_account_view.cc

@@ -87,6 +87,36 @@ views::Label* CreateLabel(const base::string16& text, SkColor color) {
   return label;
 }
 
+class LoginExpandedPublicAccountEventHandler : public ui::EventHandler {
+ public:
+  explicit LoginExpandedPublicAccountEventHandler(
+      LoginExpandedPublicAccountView* view)
+      : view_(view) {
+    Shell::Get()->AddPreTargetHandler(this);
+  }
+  ~LoginExpandedPublicAccountEventHandler() override {
+    Shell::Get()->RemovePreTargetHandler(this);
+  }
+
+ private:
+  // ui::EventHandler:
+  void OnMouseEvent(ui::MouseEvent* event) override {
+    if (event->type() == ui::ET_MOUSE_PRESSED)
+      view_->ProcessPressedEvent(event->AsLocatedEvent());
+  }
+  void OnGestureEvent(ui::GestureEvent* event) override {
+    if ((event->type() == ui::ET_GESTURE_TAP ||
+         event->type() == ui::ET_GESTURE_TAP_DOWN)) {
+      view_->ProcessPressedEvent(event->AsLocatedEvent());
+    }
+  }
+  void OnKeyEvent(ui::KeyEvent* event) override { view_->OnKeyEvent(event); }
+
+  LoginExpandedPublicAccountView* view_;
+
+  DISALLOW_COPY_AND_ASSIGN(LoginExpandedPublicAccountEventHandler);
+};
+
 }  // namespace
 
 // Button with text on the left side and an icon on the right side.
@@ -648,8 +678,9 @@ LoginExpandedPublicAccountView::TestApi::monitoring_warning_icon() {
 LoginExpandedPublicAccountView::LoginExpandedPublicAccountView(
     const OnPublicSessionViewDismissed& on_dismissed)
     : NonAccessibleView(kLoginExpandedPublicAccountViewClassName),
-      on_dismissed_(on_dismissed) {
-  Shell::Get()->AddPreTargetHandler(this);
+      on_dismissed_(on_dismissed),
+      event_handler_(
+          std::make_unique<LoginExpandedPublicAccountEventHandler>(this)) {
   SetLayoutManager(
       std::make_unique<views::BoxLayout>(views::BoxLayout::kHorizontal));
   SetPreferredSize(gfx::Size(kExpandedViewWidthDp, kExpandedViewHeightDp));
@@ -681,9 +712,7 @@ LoginExpandedPublicAccountView::LoginExpandedPublicAccountView(
   AddChildView(right_pane_);
 }
 
-LoginExpandedPublicAccountView::~LoginExpandedPublicAccountView() {
-  Shell::Get()->RemovePreTargetHandler(this);
-}
+LoginExpandedPublicAccountView::~LoginExpandedPublicAccountView() = default;
 
 void LoginExpandedPublicAccountView::ProcessPressedEvent(
     const ui::LocatedEvent* event) {
@@ -759,18 +788,6 @@ void LoginExpandedPublicAccountView::OnPaint(gfx::Canvas* canvas) {
   canvas->DrawRoundRect(GetContentsBounds(), kRoundRectCornerRadiusDp, flags);
 }
 
-void LoginExpandedPublicAccountView::OnMouseEvent(ui::MouseEvent* event) {
-  if (event->type() == ui::ET_MOUSE_PRESSED)
-    ProcessPressedEvent(event->AsLocatedEvent());
-}
-
-void LoginExpandedPublicAccountView::OnGestureEvent(ui::GestureEvent* event) {
-  if ((event->type() == ui::ET_GESTURE_TAP ||
-       event->type() == ui::ET_GESTURE_TAP_DOWN)) {
-    ProcessPressedEvent(event->AsLocatedEvent());
-  }
-}
-
 void LoginExpandedPublicAccountView::OnKeyEvent(ui::KeyEvent* event) {
   if (!visible() || event->type() != ui::ET_KEY_PRESSED)
     return;

ash/login/ui/login_expanded_public_account_view.h

@@ -5,6 +5,8 @@
 #ifndef ASH_LOGIN_UI_LOGIN_EXPANDED_PUBLIC_ACCOUNT_VIEW_H_
 #define ASH_LOGIN_UI_LOGIN_EXPANDED_PUBLIC_ACCOUNT_VIEW_H_
 
+#include <memory>
+
 #include "ash/ash_export.h"
 #include "ash/login/ui/login_menu_view.h"
 #include "ash/login/ui/non_accessible_view.h"
@@ -65,8 +67,6 @@ class ASH_EXPORT LoginExpandedPublicAccountView : public NonAccessibleView {
   void OnPaint(gfx::Canvas* canvas) override;
 
   // ui::EventHandler:
-  void OnMouseEvent(ui::MouseEvent* event) override;
-  void OnGestureEvent(ui::GestureEvent* event) override;
   void OnKeyEvent(ui::KeyEvent* event) override;
 
  private:
@@ -74,6 +74,7 @@ class ASH_EXPORT LoginExpandedPublicAccountView : public NonAccessibleView {
   RightPaneView* right_pane_ = nullptr;
   OnPublicSessionViewDismissed on_dismissed_;
   PublicAccountWarningDialog* warning_dialog_ = nullptr;
+  std::unique_ptr<ui::EventHandler> event_handler_;
 
   base::WeakPtrFactory<LoginExpandedPublicAccountView> weak_factory_{this};
 

ash/login/ui/login_expanded_public_account_view_unittest.cc

@@ -34,7 +34,9 @@ constexpr char kKeyboardNameForItem1[] = "keyboard1";
 constexpr char kKeyboardIdForItem2[] = "keyboard_id2";
 constexpr char kKeyboardNameForItem2[] = "keyboard2";
 
-class LoginExpandedPublicAccountViewTest : public LoginTestBase {
+class LoginExpandedPublicAccountViewTest
+    : public LoginTestBase,
+      public ::testing::WithParamInterface<const char*> {
  protected:
   LoginExpandedPublicAccountViewTest() = default;
   ~LoginExpandedPublicAccountViewTest() override = default;
@@ -95,9 +97,16 @@ class LoginExpandedPublicAccountViewTest : public LoginTestBase {
   }
 
   void TapOnView(views::View* tap_target) {
-    GetEventGenerator()->MoveMouseTo(
-        tap_target->GetBoundsInScreen().CenterPoint());
-    GetEventGenerator()->ClickLeftButton();
+    if (GetParam() == std::string("mouse")) {
+      GetEventGenerator()->MoveMouseTo(
+          tap_target->GetBoundsInScreen().CenterPoint());
+      GetEventGenerator()->ClickLeftButton();
+    } else {
+      GetEventGenerator()->MoveTouch(
+          tap_target->GetBoundsInScreen().CenterPoint());
+      GetEventGenerator()->PressTouch();
+      GetEventGenerator()->ReleaseTouch();
+    }
   }
 
   mojom::LoginUserInfoPtr user_;
@@ -114,7 +123,7 @@ class LoginExpandedPublicAccountViewTest : public LoginTestBase {
 }  // namespace
 
 // Verifies toggle advanced view will update the layout correctly.
-TEST_F(LoginExpandedPublicAccountViewTest, ToggleAdvancedView) {
+TEST_P(LoginExpandedPublicAccountViewTest, ToggleAdvancedView) {
   public_account_->SizeToPreferredSize();
   EXPECT_EQ(public_account_->width(), kBubbleTotalWidthDp);
   EXPECT_EQ(public_account_->height(), kBubbleTotalHeightDp);
@@ -142,7 +151,7 @@ TEST_F(LoginExpandedPublicAccountViewTest, ToggleAdvancedView) {
 }
 
 // Verifies warning dialog shows up correctly.
-TEST_F(LoginExpandedPublicAccountViewTest, ShowWarningDialog) {
+TEST_P(LoginExpandedPublicAccountViewTest, ShowWarningDialog) {
   LoginExpandedPublicAccountView::TestApi test_api(public_account_);
   views::StyledLabel::TestApi styled_label_test(test_api.learn_more_label());
   EXPECT_EQ(test_api.warning_dialog(), nullptr);
@@ -150,17 +159,13 @@ TEST_F(LoginExpandedPublicAccountViewTest, ShowWarningDialog) {
 
   // Tap on the learn more link.
   views::View* link_view = styled_label_test.link_targets().begin()->first;
-  GetEventGenerator()->MoveMouseTo(
-      link_view->GetBoundsInScreen().CenterPoint());
-  GetEventGenerator()->ClickLeftButton();
+  TapOnView(link_view);
   EXPECT_NE(test_api.warning_dialog(), nullptr);
   EXPECT_TRUE(test_api.warning_dialog()->IsVisible());
 
   // When warning dialog is shown, tap outside of public account expanded view
   // should not hide it.
-  GetEventGenerator()->MoveMouseTo(
-      other_view_->GetBoundsInScreen().CenterPoint());
-  GetEventGenerator()->ClickLeftButton();
+  TapOnView(other_view_);
   EXPECT_TRUE(public_account_->visible());
   EXPECT_NE(test_api.warning_dialog(), nullptr);
   EXPECT_TRUE(test_api.warning_dialog()->IsVisible());
@@ -184,7 +189,7 @@ TEST_F(LoginExpandedPublicAccountViewTest, ShowWarningDialog) {
 }
 
 // Verifies tap on submit button will try to launch public session.
-TEST_F(LoginExpandedPublicAccountViewTest, LaunchPublicSession) {
+TEST_P(LoginExpandedPublicAccountViewTest, LaunchPublicSession) {
   LoginExpandedPublicAccountView::TestApi test_api(public_account_);
 
   // Verify the language and keyboard information is populated correctly.
@@ -205,7 +210,7 @@ TEST_F(LoginExpandedPublicAccountViewTest, LaunchPublicSession) {
 }
 
 // Verifies both language and keyboard menus shows up correctly.
-TEST_F(LoginExpandedPublicAccountViewTest, ShowLanguageAndKeyboardMenu) {
+TEST_P(LoginExpandedPublicAccountViewTest, ShowLanguageAndKeyboardMenu) {
   LoginExpandedPublicAccountView::TestApi test_api(public_account_);
   EXPECT_FALSE(user_->public_account_info->show_advanced_view);
   EXPECT_FALSE(test_api.advanced_view()->visible());
@@ -248,7 +253,7 @@ TEST_F(LoginExpandedPublicAccountViewTest, ShowLanguageAndKeyboardMenu) {
   EXPECT_EQ(nullptr, test_api.keyboard_menu_view());
 }
 
-TEST_F(LoginExpandedPublicAccountViewTest, ChangeMenuSelection) {
+TEST_P(LoginExpandedPublicAccountViewTest, ChangeMenuSelection) {
   LoginExpandedPublicAccountView::TestApi test_api(public_account_);
   user_->public_account_info->show_advanced_view = true;
   public_account_->UpdateForUser(user_);
@@ -291,4 +296,8 @@ TEST_F(LoginExpandedPublicAccountViewTest, ChangeMenuSelection) {
   EXPECT_EQ(test_api.selected_keyboard_item().value, kKeyboardIdForItem1);
 }
 
+INSTANTIATE_TEST_CASE_P(,
+                        LoginExpandedPublicAccountViewTest,
+                        ::testing::Values("mouse", "touch"));
+
 }  // namespace ash