Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1894473002:
[Abandoned/Root cause CL reverted]Fix crash: Initialize all fields of TextInputState. (Closed)
DescriptionFix crash: Initialize all fields of TextInputState.
The uninitialized values are causing crashes in multiple locations.
The initial values for selection/composition ranges are taken from its Blink
counter part: src/third_party/WebKit/public/web/WebTextInputInfo.h.
and |is_non_ime_change| set to true, under the assumption that the state
default-constructed by Chrome won't be originated from IME.
BUG=602926
Patch Set 1 #
Total comments: 5
Messages
Total messages: 13 (2 generated)
kinaba@chromium.org changed reviewers: + changwan@chromium.org, creis@chromium.org, ekaramad@chromium.org
PTAL: +ekaramad: as the author of the class in http://crrev.com/385537 +creis: as the reviwer of the CL, and as content/OWNER. +changwan: to see if the default value (esp. of is_non_ime_change) is fine
On 2016/04/15 07:14:42, kinaba wrote: > PTAL: > +ekaramad: as the author of the class in http://crrev.com/385537 > +creis: as the reviwer of the CL, and as content/OWNER. > +changwan: to see if the default value (esp. of is_non_ime_change) is fine web_contents_impl.cc may have a chance to use uninitialized value on other platforms, but it's not clear why this causes the mentioned errors on Android. Could you try to pinpoint how uninitialized values are propagated to browser?
On 2016/04/15 09:34:39, Changwan Ryu wrote: > On 2016/04/15 07:14:42, kinaba wrote: > > PTAL: > > +ekaramad: as the author of the class in http://crrev.com/385537 > > +creis: as the reviwer of the CL, and as content/OWNER. > > +changwan: to see if the default value (esp. of is_non_ime_change) is fine > > web_contents_impl.cc may have a chance to use uninitialized value on other > platforms, but it's not clear why > this causes the mentioned errors on Android. Could you try to pinpoint how > uninitialized values are propagated to browser? LGTM. This CL will fix Issue 603209 which is a crash on debug Android for JNI conversion. But this CL will do more.
changwan@ -- The TextInputState is used in RenderWidgetHostViewBase and BrowserPluginGuest. That was I guess the reason for having uninitialized values on the browser side. The issue 603209 was a crash due to is_non_ime_change not being initialized (JNI error). WebContentsImpl, InterstitialPageImpl, RWHVBase, BrowserPluginGuest all keep local copies of the struct which is not initialized at first until the first IPC arrives. https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s... File content/common/text_input_state.cc (right): https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s... content/common/text_input_state.cc:19: is_non_ime_change(true) {} 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.
https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s... File content/common/text_input_state.cc (right): https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s... content/common/text_input_state.cc:19: is_non_ime_change(true) {} 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)
LGTM for OWNERS perspective, once changwan@ approves. Ehsan, can you make sure we have a test that would have caught this as part of your https://crbug.com/602723? We clearly need to be able to catch regressions in this area. https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s... File content/common/text_input_state.cc (right): https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s... content/common/text_input_state.cc:19: is_non_ime_change(true) {} On 2016/04/15 11:59:28, ehsaaan wrote: > 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. +1 to false (from an uninformed standpoint), but I defer to changwan@.
https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s... File content/common/text_input_state.cc (right): https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s... content/common/text_input_state.cc:19: is_non_ime_change(true) {} On 2016/04/15 16:53:37, kinaba wrote: > 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) Ah, our comments overlapped. Your explanation makes sense to me.
On 2016/04/15 17:16:53, Charlie Reis wrote: > https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s... > File content/common/text_input_state.cc (right): > > https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s... > content/common/text_input_state.cc:19: is_non_ime_change(true) {} > On 2016/04/15 16:53:37, kinaba wrote: > > 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) > > Ah, our comments overlapped. Your explanation makes sense to me. Charlie -- I updated the bug tracking the missing tests.
https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s... File content/common/text_input_state.cc (right): https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s... content/common/text_input_state.cc:19: is_non_ime_change(true) {} On 2016/04/15 17:16:53, Charlie Reis wrote: > On 2016/04/15 16:53:37, kinaba wrote: > > 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) > > Ah, our comments overlapped. Your explanation makes sense to me. 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.
On 2016/04/15 18:05:15, ehsaaan wrote: > https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s... > File content/common/text_input_state.cc (right): > > https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s... > content/common/text_input_state.cc:19: is_non_ime_change(true) {} > On 2016/04/15 17:16:53, Charlie Reis wrote: > > On 2016/04/15 16:53:37, kinaba wrote: > > > 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) > > > > Ah, our comments overlapped. Your explanation makes sense to me. > > 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. The original patch is being reverted ( https://codereview.chromium.org/1652483002/) since there have been many regressions: BUG=601738, 601570, 602144, 602954, 603209, 603676, 603886, 602926, 602488.
Description was changed from ========== Fix crash: Initialize all fields of TextInputState. The uninitialized values are causing crashes in multiple locations. The initial values for selection/composition ranges are taken from its Blink counter part: src/third_party/WebKit/public/web/WebTextInputInfo.h. and |is_non_ime_change| set to true, under the assumption that the state default-constructed by Chrome won't be originated from IME. BUG=602926 ========== to ========== Fix crash: Initialize all fields of TextInputState. The uninitialized values are causing crashes in multiple locations. The initial values for selection/composition ranges are taken from its Blink counter part: src/third_party/WebKit/public/web/WebTextInputInfo.h. and |is_non_ime_change| set to true, under the assumption that the state default-constructed by Chrome won't be originated from IME. BUG=602926 ==========
Message was sent while issue was closed.
On 2016/04/15 20:24:27, ehsaaan wrote: > On 2016/04/15 18:05:15, ehsaaan wrote: > > > https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s... > > File content/common/text_input_state.cc (right): > > > > > https://codereview.chromium.org/1894473002/diff/1/content/common/text_input_s... > > content/common/text_input_state.cc:19: is_non_ime_change(true) {} > > On 2016/04/15 17:16:53, Charlie Reis wrote: > > > On 2016/04/15 16:53:37, kinaba wrote: > > > > 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) > > > > > > Ah, our comments overlapped. Your explanation makes sense to me. > > > > 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. > > The original patch is being reverted ( > https://codereview.chromium.org/1652483002/) since there have been many > regressions: > BUG=601738, 601570, 602144, 602954, 603209, 603676, 603886, 602926, 602488. Got it. Thanks for taking the brave decision to revert. Closing the current CL. |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
