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

components/arc/ime/arc_ime_service.cc

Index: components/arc/ime/arc_ime_service.cc
diff --git a/components/arc/ime/arc_ime_service.cc b/components/arc/ime/arc_ime_service.cc
index 71783c33fbceeb7056d77e4a5d3036de421bf49e..eef216848ddea3bfcae3b8b52ae9e51317aeb7c1 100644
--- a/components/arc/ime/arc_ime_service.cc
+++ b/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,27 @@ 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));
+
+  // 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);
   }
 }