Let ImeAdapterAndroid have the same lifecycle as its Java peer

content/browser/renderer_host/render_widget_host_view_android.cc

Index: content/browser/renderer_host/render_widget_host_view_android.cc
diff --git a/content/browser/renderer_host/render_widget_host_view_android.cc b/content/browser/renderer_host/render_widget_host_view_android.cc
index 0404809aee851394babc0f21a464ff167e212e2f..299a23984e2c49cd15feab798972c0cad57481b3 100644
--- a/content/browser/renderer_host/render_widget_host_view_android.cc
+++ b/content/browser/renderer_host/render_widget_host_view_android.cc
@@ -53,6 +53,7 @@
 #include "content/browser/renderer_host/compositor_impl_android.h"
 #include "content/browser/renderer_host/dip_util.h"
 #include "content/browser/renderer_host/frame_metadata_util.h"
+#include "content/browser/renderer_host/ime_adapter_android.h"
 #include "content/browser/renderer_host/input/input_router_impl.h"
 #include "content/browser/renderer_host/input/synthetic_gesture_target_android.h"
 #include "content/browser/renderer_host/input/web_input_event_builders_android.h"
@@ -449,7 +450,7 @@ RenderWidgetHostViewAndroid::RenderWidgetHostViewAndroid(
       is_window_activity_started_(true),
       is_in_vr_(false),
       content_view_core_(nullptr),
-      ime_adapter_android_(this),
+      ime_adapter_android_(nullptr),
       cached_background_color_(SK_ColorWHITE),
       view_(this),
       last_compositor_frame_sink_id_(kUndefinedCompositorFrameSinkId),
@@ -496,6 +497,7 @@ RenderWidgetHostViewAndroid::~RenderWidgetHostViewAndroid() {
   if (content_view_core_)
     content_view_core_->RemoveObserver(this);
   SetContentViewCore(NULL);
+  ime_adapter_android_ = nullptr;
   DCHECK(ack_callbacks_.empty());
   DCHECK(!delegated_frame_host_);
 }
@@ -703,8 +705,11 @@ void RenderWidgetHostViewAndroid::SetIsLoading(bool is_loading) {
   // is TabContentsDelegate.
 }
 
-long RenderWidgetHostViewAndroid::GetNativeImeAdapter() {
-  return reinterpret_cast<intptr_t>(&ime_adapter_android_);
+void RenderWidgetHostViewAndroid::ConnectImeAdapter(
+    ImeAdapterAndroid* ime_adapter) {
+  ime_adapter_android_ = ime_adapter;
+  if (ime_adapter)
+    ime_adapter->SetRenderWidgetHostViewAndroid(weak_ptr_factory_.GetWeakPtr());
 }
 
 // -----------------------------------------------------------------------------
@@ -725,10 +730,9 @@ void RenderWidgetHostViewAndroid::OnUpdateTextInputStateCalled(
     return;
 
   content_view_core_->UpdateImeAdapter(
-      GetNativeImeAdapter(), static_cast<int>(state.type), state.flags,
-      state.mode, state.value, state.selection_start, state.selection_end,
-      state.composition_start, state.composition_end, state.show_ime_if_needed,
-      state.reply_to_request);
+      static_cast<int>(state.type), state.flags, state.mode, state.value,
+      state.selection_start, state.selection_end, state.composition_start,
+      state.composition_end, state.show_ime_if_needed, state.reply_to_request);
 }
 
 void RenderWidgetHostViewAndroid::OnImeCompositionRangeChanged(
@@ -744,14 +748,16 @@ void RenderWidgetHostViewAndroid::OnImeCompositionRangeChanged(
   for (const gfx::Rect& rect : info->character_bounds)
     character_bounds.emplace_back(rect);
 
-  ime_adapter_android_.SetCharacterBounds(character_bounds);
+  DCHECK(ime_adapter_android_);
+  ime_adapter_android_->SetCharacterBounds(character_bounds);
 }
 
 void RenderWidgetHostViewAndroid::OnImeCancelComposition(
     TextInputManager* text_input_manager,
     RenderWidgetHostViewBase* updated_view) {
   DCHECK_EQ(text_input_manager_, text_input_manager);
-  ime_adapter_android_.CancelComposition();
+  DCHECK(ime_adapter_android_);
+  ime_adapter_android_->CancelComposition();
 }
 
 void RenderWidgetHostViewAndroid::OnTextSelectionChanged(
@@ -1004,7 +1010,8 @@ void RenderWidgetHostViewAndroid::SetMultiTouchZoomSupportEnabled(
 void RenderWidgetHostViewAndroid::FocusedNodeChanged(
     bool is_editable_node,
     const gfx::Rect& node_bounds_in_screen) {
-  ime_adapter_android_.FocusedNodeChanged(is_editable_node);
+  DCHECK(ime_adapter_android_);
+  ime_adapter_android_->FocusedNodeChanged(is_editable_node);
 }
 
 void RenderWidgetHostViewAndroid::RenderProcessGone(
@@ -1862,6 +1869,8 @@ void RenderWidgetHostViewAndroid::SetContentViewCore(
       parent_view->GetLayer()->AddChild(view_.GetLayer());
     }
     content_view_core_ = content_view_core;
+    ConnectImeAdapter(content_view_core ? content_view_core->ime_adapter()

[boliu, 2017/03/23 04:17:01]

I think non-active RWHVA can still have non null IAA? this is only cleared on in
RWHVA destructor afaict?

[Jinsuk Kim, 2017/03/23 07:25:10]

When RenderViewHost is swapped out, SetContentViewCore(nullptr) is called on the
old one that becomes non-active. Doesn't that takes care of it?

[boliu, 2017/03/23 14:18:50]

Oh I missed ContentViewCoreImpl::RenderViewHostChanged. However that doesn't
cover interstitials. Actually, I'm not sure the previous code worked well with
interstitials either..

[Jinsuk Kim, 2017/03/23 23:10:24]

Right I didn't handle non-active  ones (by nulling out its IAA). From my testing
the interstitials get destroyed immediately after being detached. Which makes me
think it won't be of concern in practice. The CL works for interstitials (see
CVCImpl::DidDetachInterstitialPage) besides that.  

If that still doesn't look enough for you, what I can do is cache the
interstitial rwhva in CVCImpl, and null out its IAA when it gets detached. This
may need more testing.

[Jinsuk Kim, 2017/03/23 23:16:22]

Oh by 'the previous code' you mean the code before my CL?
I think it worked because it is RWHVA that actively attaches its IAA to Java IA.
After the change it is the other way around (web contents connecting its IAA to
active RWHVA) - this happens when it calls RWHVA::SetContentViewCore (except
going back from interstitial to main RWHVA).

[boliu, 2017/03/23 23:17:40]

Well, what about the page in the back while the interstitial is showing?

[boliu, 2017/03/23 23:22:56]

Yes
How previous code work in the case of interstitial detach?

[Jinsuk Kim, 2017/03/24 00:01:45]

Previously the attaching/detaching piggypacked on RWHVA -> CVC FrameUpdateInfo
to make sure the active RWHVA/IAA always gets attached, and the one going
background loses java obj (and generated too many no-op attaching/detaching
attempts as well). 

[Jinsuk Kim, 2017/03/24 00:01:45]

This may not be always true but interstitial page starts with its own
content/RWHVA, and then later yields to a new, main RWVHA. So I cared about the
case of interstitial page being detached. Is it possible that on Android the
flow begins with main RWHVA -> interstitial -> main RHWVA for one content?
Couldn't test such flow since it didn't happen in testing yet.

[Jinsuk Kim, 2017/03/24 00:15:05]

Sorry I was wrong (completely mistaken). This is not true.

[Jinsuk Kim, 2017/03/24 02:34:23]

I should have said "it piggybacked on RWHVA::OnUpdateTextInputStateCalled"
instead. It's still valid to say that attach/detach happens more than necessary.
I'm splitting it from the state update in this CL.

+                                        : nullptr);
   }
 
   BrowserAccessibilityManager* manager = NULL;