Browser Side Text Input State Tracking for OOPIF.

content/browser/web_contents/web_contents_impl.h

Index: content/browser/web_contents/web_contents_impl.h
diff --git a/content/browser/web_contents/web_contents_impl.h b/content/browser/web_contents/web_contents_impl.h
index 06242777d03075e3d1f04c75933689dc544bbf4d..cfa60c8658136640bf1769d6a41f7984a203609b 100644
--- a/content/browser/web_contents/web_contents_impl.h
+++ b/content/browser/web_contents/web_contents_impl.h
@@ -619,6 +619,9 @@ class CONTENT_EXPORT WebContentsImpl
                               const std::vector<uint8_t>& proto) override;
   void OnRenderFrameProxyVisibilityChanged(bool visible) override;
   void SendScreenRects() override;
+  TextInputState GetTextInputState() override;
+  void UpdateTextInputState(RenderWidgetHostViewBase* rwhv,
+                            bool text_input_state_changed) override;
 
   // RenderFrameHostManager::Delegate ------------------------------------------
 
@@ -1338,6 +1341,10 @@ class CONTENT_EXPORT WebContentsImpl
 
   bool page_scale_factor_is_one_;
 
+  // The RWHV which is currently focused.
+  TextInputState text_input_state_;
+  RenderWidgetHostViewBase* view_with_active_text_input_;

[Charlie Reis, 2016/03/15 18:32:01]

This looks like it could get stale if the RWHV went away (e.g., after a
cross-process navigation, or maybe a process crash?).  That would lead to a UaF.
 Can we find a safe way to clear it if necessary?

[EhsanK, 2016/03/15 23:51:18]

I added some notes above under your comment on
'render_widget_host_view_child_frame.cc'. There by calling
TextInputStateChanged(TextInputState()) in the destruction path for
RenderWidgetHostViewChildFrame I was trying to address this issue. Two notes:

1- If the destruction path is normal, i.e., by detaching the frame, then the
frame should lose focus and eventually the TextInputStateChanged called
organically. 

2- The line added in RWHVCF::Destroy was to address the crash scenario. It will
get called as long as RenderProcessGone is called. I tested this by manually
crashing the process and this line gets hit.

Is there a third (or fourth) scenario not reflected here? Please let me know.
Also, as explained above, I think it is best to have a method do this clean up
rather than the ambiguous line above. We might need to repeat the same process
for inner and outer WebContents since based on this design, the inner
WebContents reports/obtains the state from outer WebContents.

[Charlie Reis, 2016/03/16 16:55:13]

Makes sense, though "eventually" concerns me.  Is there a period of time where
the pointer will be stale, or do we clear it at the same time that the frame is
detached?  Let's confirm.
Great.  Yes, I missed the Destroy behavior before.  That's what I was looking
for.
I'm curious about cross-process main frame navigations.  Suppose we navigate
from a.com to b.com in the omnibox and switch processes.  The RWHV will change,
and I'm not sure if we'll get an update to this member or not.  It's not a
RWHVCF, so I don't see anything that will clear the pointer to the old RWVH
here.
Given this inner/outer case and cross-process main frame navigations, I'm
wondering if we need to add the clearing logic to all RWHVs, rather than just
RWHVCFs?

[EhsanK, 2016/03/30 20:46:03]

I tested the following:

  - Detach: An IPC comes and sets the input to NONE (tested for RWHVCF).

  - Crash: NotifyHostDelegateAboutShutdown() fixes the issue. There is one new
test.
When navigation happens, RWHV is destroyed and it resets the input state.

[Charlie Reis, 2016/03/30 21:52:47]

What code makes this happen?  I can't follow it by reading the code, given how
complex it is.

[EhsanK, 2016/03/30 23:36:33]

Sorry for the incomplete explanation. I meant to say that the method
RenderWidgetHostViewBase::NotifyHostDelegateAboutShutdown() is supposed to take
care of all such scenarios. This method is called from RWHV::Destroy() and what
it does is to
 1) Reset the text_input_state() to NONE.
 2) Notify the RenderWidgetHostDelegate (WebContentsImpl) that the state has
changed.
Since everything happens on the same thread, we should never have stale pointers
or incorrect state.

I also added a test which verifies this when the RWHV crashes by calling the
RPH::Shutdown().

I also did manually check and printf  the state for scenarios such as
removeChild and navigation. It seems like most corner cases are covered.

All in all, with having a WeakPtr instead of the raw pointer, we should be fine
with the stale pointer problem.

[Charlie Reis, 2016/03/31 05:59:16]

Thanks for clarifying, and I agree about the WeakPtr making it safer.

The one part I don't follow is how NotifyHostDelegateAboutShutdown is called
from RWHV::Destroy.  There are lots of implementations of Destroy, and I only
see it called from some of them (Android, Mac, ChildFrame, and indirectly in
Guest).  Aura calls it from RemovingFromRootWindow (and I can't see how that's
connected to Destroy), and Mus doesn't seem to call it at all.

That's why I was concerned about clearing it in a consistent way, so that we
don't end up with platform-specific bugs.  Out of curiosity, would it work to
call it from ~RenderWidgetHostViewBase, so that we don't have to worry about all
the platform subclasses getting it right in their own way?

[EhsanK, 2016/04/01 00:59:16]

It is more correct to say that NotifyHostDelegateAboutShutdown() is called after
NotifyObseversAboutShutdown() (rather than inside ::Destroy() due to the comment
inside RenderWidgetHostViewAura::Destroy()).

[EhsanK, 2016/04/01 00:59:17]

I meant to say (I think I did not publish previous draft) that this method
should be called after NotifyObserversAboutShutdown(), which happens in Destory
function. For Aura it is different. There are no explicit calls to
NotifyObserversAboutShutdown. Further, RWHV::Destroy is not always hit in the
destruction path (the note in the cc file says this happens if we close the
window).
I guess that was one scenario leading to destruction. I picked that location
becayse the next line notifies the IME. Taking another look at the code, I
believe it is safe/better to call it in the dtor of Aura now. That will
definitely be where ::Destroy. This will also address the comment in
RWHVAura::Destroy in my opinion.
Most of the code for Mus was not implemented including how the IME is going to
work so I had left it untouched and for that reason, RWHV::TextInputStateChanged
remained virtual. Now I have made it a public method of RWHV and added an
UpdateInputMethodIfNecessary to Mus but it is still empty. I also added
NotifyHostDelegateAboutShutdown to the Destroy method of RWHVMus.
The way I see it, unfortunately not. We lose the reference to
RenderWidgetHostImpl before the call to RenderWidgetHostViewBase dtor. That
reference is needed to get the WebContentsImpl.

+
   base::WeakPtrFactory<WebContentsImpl> loading_weak_factory_;
   base::WeakPtrFactory<WebContentsImpl> weak_factory_;