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

components/arc/ime/arc_ime_service.cc

 // Copyright 2016 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 "components/arc/ime/arc_ime_service.h"
 
 #include <utility>
 
 #include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
@@ skipping 66 matching lines @@
     ui::InputMethod* test_input_method) {
   test_input_method_ = test_input_method;
 }
 
 void ArcImeService::SetArcWindowDetectorForTesting(
     std::unique_ptr<ArcWindowDetector> detector) {
   arc_window_detector_ = std::move(detector);
 }
 
 ui::InputMethod* ArcImeService::GetInputMethod() {
+  if (focused_arc_window_.windows().empty())
+    return nullptr;
+
   if (test_input_method_)
     return test_input_method_;
-  if (focused_arc_window_.windows().empty())
-    return nullptr;
   return focused_arc_window_.windows().front()->GetHost()->GetInputMethod();
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Overridden from aura::EnvObserver:
 
 void ArcImeService::OnWindowInitialized(aura::Window* new_window) {
   if (arc_window_detector_->IsArcWindow(new_window)) {
     if (!is_focus_observer_installed_) {
       exo::WMHelper::GetInstance()->AddFocusObserver(this);
       is_focus_observer_installed_ = true;
     }
   }
   keyboard::KeyboardController* keyboard_controller =
       keyboard::KeyboardController::GetInstance();
   if (keyboard_controller && keyboard_controller_ != keyboard_controller) {
     // Registering |this| as an observer only once per KeyboardController.
     keyboard_controller_ = keyboard_controller;
     keyboard_controller_->AddObserver(this);
   }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // 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;
   lost_focus = lost_focus ? lost_focus->GetToplevelWindow() : nullptr;
   if (lost_focus == gained_focus)
     return;
 
   const bool detach = (lost_focus && focused_arc_window_.Contains(lost_focus));
   const bool attach =
       (gained_focus && arc_window_detector_->IsArcTopLevelWindow(gained_focus));
+
+  // 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) {

[hidehiko, 2016/12/14 09:25:27]

I'm now slightly confused. If IME is attached to the window,

if (detach != attach) {
  if (detach)
    lost_focus->GetHost()->GetInputMethod()->DetachTextInputClient(...);
  if (attach)
    gained_focus->GetHost()->GetInputMethod()->SetFocusedTextInputClient(...);
}

seems what we need here?
Is there a case focused_arc_window_ contains two or more windows?

[kinaba, 2016/12/14 12:12:56]

On ChromeOS as of now, all windows share the same ui::InputMethod instance,
so (detach != attach) and (detaching_ime != attaching_ime) should do the same
thing.

But that's not guaranteed in Aura in general, so I think we should not rely on
that.
For instance Mustash+MultiDisplay may (or may not; I don't know really) utilize
multiple InputMethod instances.
In such a case, we do need to do both Detach and SetFocus. My old code was wrong
in this point too.
No, its size is either 0 or 1.
It could have been a raw pointer, but I just wanted to avoid the danger of
dangling pointer (whichs is what aura::WindowTracker does.)

[hidehiko, 2016/12/14 15:27:22]

I understood. Thank you for detailed explanation.

Optional: How about;

ui::InputMethod* detaching_ime = detach ?
lost_focus->GetHost()->GetInputMethod();
ui::InputMethod* attaching_ime = ...;

if (detaching_ime != attaching_ime) {
  ...
}

GetInputMethod() implicitly depends on focused_arc_window_, which is edited in
this function. I know it's commented, I personally prefer the explicit
dependency to different calling timing. It's minor thing, I think. Up to you.

[kinaba, 2016/12/15 00:58:41]

I totally agree with your concern. Added a TODO comment on that.
That said, for this time, let me go as-is.

GetInputMethod() method is working also as a point to inject the fake IME for
testing,
so we cannot just rewrite it to the direct
lost_focus->GetHost()->GetInputMethod() call.

I probably can inject the fake IME into more deeper layer instead of our custom
way, using
https://cs.chromium.org/chromium/src/ui/base/ime/input_method_factory.h?q=Cre...
but for this, we need to add more set up code related to ash/aura/views in the
test.

Since this CL is meant to be merged to M56, I want to keep the change minimal as
possible.

+    if (detaching_ime)
+      detaching_ime->DetachTextInputClient(this);
+    if (attaching_ime)
+      attaching_ime->SetFocusedTextInputClient(this);
   }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 // Overridden from arc::ArcImeBridge::Delegate
 
 void ArcImeService::OnTextInputTypeChanged(ui::TextInputType type) {
   if (ime_type_ == type)
     return;
   ime_type_ = type;
@@ skipping 186 matching lines @@
     base::i18n::TextDirection direction) {
   return false;
 }
 
 bool ArcImeService::IsTextEditCommandEnabled(
     ui::TextEditCommand command) const {
   return false;
 }
 
 }  // namespace arc