[Abandoned/Root cause CL reverted]Fix crash: Initialize all fields of TextInputState.

content/common/text_input_state.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 "content/common/text_input_state.h"
 
 namespace content {
 
 TextInputState::TextInputState()
     : type(ui::TEXT_INPUT_TYPE_NONE),
       mode(ui::TEXT_INPUT_MODE_DEFAULT),
       flags(0),
+      selection_start(0),
+      selection_end(0),
+      composition_start(-1),
+      composition_end(-1),
       can_compose_inline(true),
-      show_ime_if_needed(false) {}
+      show_ime_if_needed(false),
+      is_non_ime_change(true) {}

[EhsanK, 2016/04/15 11:59:28]

changwan@ will have a better opinion, but I think this should be false. On
RenderWidget side this is always initialized. But a value of true might cause an
extra IPC from browser in Android if there is call to
RenderWidgetHostViewBase::TextInputStateChanged.

Right now this call can be made by a call to
BrowserPluginGuest::OnTextInputStateChanged from BrowserPluginGuest::OnDetach. 

If true is the way to go, the code in BrowserPluginGuest::OnDetach should be
modified.

[kinaba, 2016/04/15 16:53:37]

You're right that we should not cause sending unmatching ACK IPC to RenderWidget
by setting the value to true.

On the other hand, a value of false confuses ThreadedInputConnection, which
receives fromIME=true event when it is not processing any command from IME.
(This is the crash I mentioned in 602926#c6.)

So, either way we take, we need to take care of somewhere else. Considering
that, since the default-constructed event is by no means originated from a
change done by IME, the natural and right choice should be
"is_non_ime_change=true", in my opinion. Using it as a flag to control ACK to
the renderer outside the IPC message handler is a kind of abuse.


If you think it's fine, I'll modify the CL to properly cover
BrowserPluginGuest::OnDetach and RenderWidgetHostImpl::DetachDelegate (on
Monday)

[Charlie Reis, 2016/04/15 16:57:39]

+1 to false (from an uninformed standpoint), but I defer to changwan@.

[Charlie Reis, 2016/04/15 17:16:53]

Ah, our comments overlapped.  Your explanation makes sense to me.

[EhsanK, 2016/04/15 18:05:15]

I think based on your comment it has to be initialized to true. 
The call in BrowserPluginGuest::OnDetach is a little hack-y and is put to avoid
clean invalid text input state in WebContentsImpl after a RWHVGuest goes away.
It should eventually change after fixing crbug.com/602640. A call to
RenderWidgetHostViewBase::NotifyHostDelegateAboutShutdown will not send an IPC
so we should be fine there if |is_non_ime_change| is initialized to true. Given
that the scenario with BrowserPluginGuest::OnDetach requires a <webview> to
start with, I think the extra IPC will not be sent all the time.

 
 TextInputState::TextInputState(const TextInputState& other) = default;
 
 }  //  namespace content