ARC IME: Fix crash on shutdown due to wrong focus management.

It didn't properly detach from the input method when ARC lost focus
and no other text input client took focus until shutdown.

BUG=673809
TEST=ArcImeServiceTest.WindowFocusTracking

Review-Url: https://codereview.chromium.org/2570623005
Cr-Commit-Position: refs/heads/master@{#438720}

components/arc/ime/arc_ime_service.cc

@@ -84,10 +84,11 @@ void ArcImeService::SetArcWindowDetectorForTesting(
 }
 
 ui::InputMethod* ArcImeService::GetInputMethod() {
-  if (test_input_method_)
-    return test_input_method_;
   if (focused_arc_window_.windows().empty())
     return nullptr;
+
+  if (test_input_method_)
+    return test_input_method_;
   return focused_arc_window_.windows().front()->GetHost()->GetInputMethod();
 }
 
@@ -114,7 +115,7 @@ void ArcImeService::OnWindowInitialized(aura::Window* new_window) {
 // Overridden from exo::WMHelper::FocusChangeObserver:
 
 void ArcImeService::OnWindowFocused(aura::Window* gained_focus,
-                                   aura::Window* lost_focus) {
+                                    aura::Window* lost_focus) {
   // The Aura focus may or may not be on sub-window of the toplevel ARC++ frame.
   // To handle all cases, judge the state by always climbing up to the toplevel.
   gained_focus = gained_focus ? gained_focus->GetToplevelWindow() : nullptr;
@@ -125,20 +126,31 @@ void ArcImeService::OnWindowFocused(aura::Window* gained_focus,
   const bool detach = (lost_focus && focused_arc_window_.Contains(lost_focus));
   const bool attach =
       (gained_focus && arc_window_detector_->IsArcTopLevelWindow(gained_focus));
+
+  // TODO(kinaba): Implicit dependency in GetInputMethod as described below is
+  // confusing. Consider getting InputMethod directly from lost_ or gained_focus
+  // variables. For that, we need to change how to inject testing InputMethod.
+  //
+  // GetInputMethod() retrieves the input method associated to
+  // |forcused_arc_window_|. Hence, to get the object we are detaching from, we
+  // must call the method before updating the forcused ARC window.
+  ui::InputMethod* const detaching_ime = detach ? GetInputMethod() : nullptr;
+
   if (detach)
     focused_arc_window_.Remove(lost_focus);
   if (attach)
     focused_arc_window_.Add(gained_focus);
 
+  ui::InputMethod* const attaching_ime = attach ? GetInputMethod() : nullptr;
+
   // Notify to the input method, either when this service is detached or
   // attached. Do nothing when the focus is moving between ARC windows,
   // to avoid unpexpected context reset in ARC.
-  ui::InputMethod* const input_method = GetInputMethod();
-  if (input_method) {
-    if (detach && !attach)
-      input_method->DetachTextInputClient(this);
-    if (!detach && attach)
-      input_method->SetFocusedTextInputClient(this);
+  if (detaching_ime != attaching_ime) {
+    if (detaching_ime)
+      detaching_ime->DetachTextInputClient(this);
+    if (attaching_ime)
+      attaching_ime->SetFocusedTextInputClient(this);
   }
 }
 

components/arc/ime/arc_ime_service_unittest.cc

@@ -138,6 +138,9 @@ class ArcImeServiceTest : public testing::Test {
   std::unique_ptr<ArcImeService> instance_;
   FakeArcImeBridge* fake_arc_ime_bridge_;  // Owned by |instance_|
 
+  FakeArcWindowDetector* fake_window_detector_; // Owned by |instance_|
+  std::unique_ptr<aura::Window> arc_win_;
+
  private:
   void SetUp() override {
     arc_bridge_service_ = base::MakeUnique<ArcBridgeService>();
@@ -147,9 +150,16 @@ class ArcImeServiceTest : public testing::Test {
 
     fake_input_method_ = base::MakeUnique<FakeInputMethod>();
     instance_->SetInputMethodForTesting(fake_input_method_.get());
+
+    fake_window_detector_ = new FakeArcWindowDetector();
+    instance_->SetArcWindowDetectorForTesting(
+        base::WrapUnique(fake_window_detector_));
+    arc_win_ = fake_window_detector_->CreateFakeArcTopLevelWindow();
   }
 
   void TearDown() override {
+    arc_win_.reset();
+    fake_window_detector_ = nullptr;
     fake_arc_ime_bridge_ = nullptr;
     instance_.reset();
     arc_bridge_service_.reset();
@@ -157,6 +167,8 @@ class ArcImeServiceTest : public testing::Test {
 };
 
 TEST_F(ArcImeServiceTest, HasCompositionText) {
+  instance_->OnWindowFocused(arc_win_.get(), nullptr);
+
   ui::CompositionText composition;
   composition.text = base::UTF8ToUTF16("nonempty text");
 
@@ -184,7 +196,8 @@ TEST_F(ArcImeServiceTest, HasCompositionText) {
 }
 
 TEST_F(ArcImeServiceTest, ShowImeIfNeeded) {
-  fake_input_method_->SetFocusedTextInputClient(instance_.get());
+  instance_->OnWindowFocused(arc_win_.get(), nullptr);
+
   instance_->OnTextInputTypeChanged(ui::TEXT_INPUT_TYPE_NONE);
   ASSERT_EQ(0, fake_input_method_->count_show_ime_if_needed());
 
@@ -197,14 +210,15 @@ TEST_F(ArcImeServiceTest, ShowImeIfNeeded) {
 }
 
 TEST_F(ArcImeServiceTest, CancelComposition) {
+  instance_->OnWindowFocused(arc_win_.get(), nullptr);
+
   // The bridge should forward the cancel event to the input method.
-  fake_input_method_->SetFocusedTextInputClient(instance_.get());
   instance_->OnCancelComposition();
   EXPECT_EQ(1, fake_input_method_->count_cancel_composition());
 }
 
 TEST_F(ArcImeServiceTest, InsertChar) {
-  fake_input_method_->SetFocusedTextInputClient(instance_.get());
+  instance_->OnWindowFocused(arc_win_.get(), nullptr);
 
   // When text input type is NONE, the event is not forwarded.
   instance_->OnTextInputTypeChanged(ui::TEXT_INPUT_TYPE_NONE);
@@ -218,22 +232,18 @@ TEST_F(ArcImeServiceTest, InsertChar) {
 }
 
 TEST_F(ArcImeServiceTest, WindowFocusTracking) {
-  auto window_detector = base::MakeUnique<FakeArcWindowDetector>();
-  std::unique_ptr<aura::Window> arc_win1 =
-      window_detector->CreateFakeArcTopLevelWindow();
   std::unique_ptr<aura::Window> arc_win2 =
-      window_detector->CreateFakeArcTopLevelWindow();
+      fake_window_detector_->CreateFakeArcTopLevelWindow();
   std::unique_ptr<aura::Window> nonarc_win =
-      window_detector->CreateFakeNonArcTopLevelWindow();
-  instance_->SetArcWindowDetectorForTesting(std::move(window_detector));
+      fake_window_detector_->CreateFakeNonArcTopLevelWindow();
 
   // ARC window is focused. ArcImeService is set as the text input client.
-  instance_->OnWindowFocused(arc_win1.get(), nullptr);
+  instance_->OnWindowFocused(arc_win_.get(), nullptr);
   EXPECT_EQ(instance_.get(), fake_input_method_->GetTextInputClient());
   EXPECT_EQ(1, fake_input_method_->count_set_focused_text_input_client());
 
   // Focus is moving between ARC windows. No state change should happen.
-  instance_->OnWindowFocused(arc_win2.get(), arc_win1.get());
+  instance_->OnWindowFocused(arc_win2.get(), arc_win_.get());
   EXPECT_EQ(instance_.get(), fake_input_method_->GetTextInputClient());
   EXPECT_EQ(1, fake_input_method_->count_set_focused_text_input_client());
 
@@ -243,12 +253,12 @@ TEST_F(ArcImeServiceTest, WindowFocusTracking) {
   EXPECT_EQ(1, fake_input_method_->count_set_focused_text_input_client());
 
   // Focus came back to an ARC window. ArcImeService is re-attached.
-  instance_->OnWindowFocused(arc_win1.get(), nonarc_win.get());
+  instance_->OnWindowFocused(arc_win_.get(), nonarc_win.get());
   EXPECT_EQ(instance_.get(), fake_input_method_->GetTextInputClient());
   EXPECT_EQ(2, fake_input_method_->count_set_focused_text_input_client());
 
   // Focus is moving out.
-  instance_->OnWindowFocused(nullptr, arc_win1.get());
+  instance_->OnWindowFocused(nullptr, arc_win_.get());
   EXPECT_EQ(nullptr, fake_input_method_->GetTextInputClient());
   EXPECT_EQ(2, fake_input_method_->count_set_focused_text_input_client());
 }