Fix a crash occuring during the destruction of TextInputManager on Android.

Fix a crash occuring during the destruction of TextInputManager on Android.

On Android, in the observer call
RenderWidgetHostViewAndroid::OnUpdateTextInputState we were using
|updated_view| passed by TextInputManager to query TextInputState. This
is correct as long as the view is not unregistered already, which
happens when
  1- If the |active_view_| is destroyed.
  2- If TextInputManager is destoryed while there is |active_view_|
  which will lead to unregistering the view.

This patch will change the way we obtain TextInputState in
RenderWidgetHostViewAndroid. We now first verify if there is an active
view/widget and then get the state. Otherwise, the default state (of
NONE) is reported.

This patch also adds a test to catch the regression.

BUG=669375, 602723, 578168
Committed: https://crrev.com/db49e17a8e2c5874585e5a499171163ce70104ec
Cr-Commit-Position: refs/heads/master@{#435178}

[EhsanK, 2016-11-29 21:57:54]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 21:58:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2016-11-29 22:01:23]

ekaramad@chromium.org changed reviewers:
+ creis@chromium.org

[EhsanK, 2016-11-29 22:01:24]

Hello Charlie,

I was surprised to know that none of the SitePerProcessTextInputManager tests
are running on Android as they are part of interactive_ui_tests.cc.

Supposedly, if they were, this crash would have been caught by one of those
tests, or the new one added which is targeted at this issue.

I am not sure if it is still worthwhile keeping that test. Also not too sure if
we could somehow enable these tests for real.

PTAL.

[commit-bot: I haz the power, 2016-11-29 22:19:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-29 22:19:31]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[EhsanK, 2016-11-29 23:00:31]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-29 23:02:40]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Reis, 2016-11-30 00:05:23]

Thanks for the quick fix and test!  LGTM if we also remove the |view| parameter
to GetTextInputState.

Note: I think you meant 578168 in the CL description.

https://codereview.chromium.org/2536943004/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2536943004/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:774: ?
*GetTextInputManager()->GetTextInputState()
I assume it's intentional to remove |updated_view|, given your comment in
TextInputManager.

However, this appears to remove the only use of the |view| parameter to
GetTextInputState, which you added in
https://codereview.chromium.org/2354793003.  Can you remove that optional
parameter as a result?  (I much prefer to avoid optional parameters when we
can-- it's very hard to reason about when looking at either the code of the
function or the call sites on their own.)

https://codereview.chromium.org/2536943004/diff/40001/content/browser/rendere...
File content/browser/renderer_host/text_input_manager.h (right):

https://codereview.chromium.org/2536943004/diff/40001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.h:43: // this observer method
might have been called in response to unregistering
nit: this this

https://codereview.chromium.org/2536943004/diff/40001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.h:44: // the |active_view_|
from TextInputManager in which case all the the |view|
nit: the the
nit: What does "all the |view| is not registered" mean?  Rephrase?

[EhsanK, 2016-11-30 00:31:50]

Description was changed from

==========
Fix a crash occuring in the destrution path TextInputManager() occuring on
Android.

On Android, in the observer call
RenderWidgetHostViewAndroid::OnUpdateTextInputState we were using
|updated_view| passed by TextInputManager to query TextInputState. This
is correct as long as the view is not unregistered already, which
happens when
  1- If the |active_view_| is destroyed.
  2- If TextInputManager is destoryed while there is |active_view_|
  which will lead to unregistering the view.

This patch will change the way we obtain TextInputState in
RenderWidgetHostViewAndroid. We now first verify if there is an active
view/widget and then get the state. Otherwise, the default state (of
NONE) is reported.

This patch also adds a test to catch the regression.

BUG=669375, 602723, 5718168
==========

to

==========
Fix a crash occuring in the destrution path TextInputManager() occuring on
Android.

On Android, in the observer call
RenderWidgetHostViewAndroid::OnUpdateTextInputState we were using
|updated_view| passed by TextInputManager to query TextInputState. This
is correct as long as the view is not unregistered already, which
happens when
  1- If the |active_view_| is destroyed.
  2- If TextInputManager is destoryed while there is |active_view_|
  which will lead to unregistering the view.

This patch will change the way we obtain TextInputState in
RenderWidgetHostViewAndroid. We now first verify if there is an active
view/widget and then get the state. Otherwise, the default state (of
NONE) is reported.

This patch also adds a test to catch the regression.

BUG=669375, 602723, 578168
==========

[EhsanK, 2016-11-30 00:33:01]

Description was changed from

==========
Fix a crash occuring in the destrution path TextInputManager() occuring on
Android.

On Android, in the observer call
RenderWidgetHostViewAndroid::OnUpdateTextInputState we were using
|updated_view| passed by TextInputManager to query TextInputState. This
is correct as long as the view is not unregistered already, which
happens when
  1- If the |active_view_| is destroyed.
  2- If TextInputManager is destoryed while there is |active_view_|
  which will lead to unregistering the view.

This patch will change the way we obtain TextInputState in
RenderWidgetHostViewAndroid. We now first verify if there is an active
view/widget and then get the state. Otherwise, the default state (of
NONE) is reported.

This patch also adds a test to catch the regression.

BUG=669375, 602723, 578168
==========

to

==========
Fix a crash occuring during the destrution of TextInputManager on Android.

On Android, in the observer call
RenderWidgetHostViewAndroid::OnUpdateTextInputState we were using
|updated_view| passed by TextInputManager to query TextInputState. This
is correct as long as the view is not unregistered already, which
happens when
  1- If the |active_view_| is destroyed.
  2- If TextInputManager is destoryed while there is |active_view_|
  which will lead to unregistering the view.

This patch will change the way we obtain TextInputState in
RenderWidgetHostViewAndroid. We now first verify if there is an active
view/widget and then get the state. Otherwise, the default state (of
NONE) is reported.

This patch also adds a test to catch the regression.

BUG=669375, 602723, 578168
==========

[EhsanK, 2016-11-30 00:34:25]

Thanks Charlie.

FYI, the test I added actually does not run on Android. I think it might still
be worthwhile having this but unfortunately, we do not run interactive ui tests
on Android.

I will try to CQ after bots go green.

https://codereview.chromium.org/2536943004/diff/40001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_android.cc (right):

https://codereview.chromium.org/2536943004/diff/40001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_android.cc:774: ?
*GetTextInputManager()->GetTextInputState()
On 2016/11/30 00:05:23, Charlie Reis (slow) wrote:
> I assume it's intentional to remove |updated_view|, given your comment in
> TextInputManager.
> 
> However, this appears to remove the only use of the |view| parameter to
> GetTextInputState, which you added in
> https://codereview.chromium.org/2354793003.  Can you remove that optional
> parameter as a result?  (I much prefer to avoid optional parameters when we
> can-- it's very hard to reason about when looking at either the code of the
> function or the call sites on their own.)

I believe there is no use for it. I am removing it. We can always put it back
there if there is a legitimate reason for inquiring about TextInputState for a
specific view (it might come up along the path of fixing OOPIF IME for Android).

https://codereview.chromium.org/2536943004/diff/40001/content/browser/rendere...
File content/browser/renderer_host/text_input_manager.h (right):

https://codereview.chromium.org/2536943004/diff/40001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.h:43: // this observer method
might have been called in response to unregistering
On 2016/11/30 00:05:23, Charlie Reis (slow) wrote:
> nit: this this

Acknowledged.

https://codereview.chromium.org/2536943004/diff/40001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.h:44: // the |active_view_|
from TextInputManager in which case all the the |view|
On 2016/11/30 00:05:23, Charlie Reis (slow) wrote:
> nit: the the
> nit: What does "all the |view| is not registered" mean?  Rephrase?

Thanks. I rephrased this part.

[EhsanK, 2016-11-30 00:34:31]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 00:35:43]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Charlie Reis, 2016-11-30 00:57:51]

Thanks, LGTM with nit.

On 2016/11/30 00:34:25, EhsanK wrote:
> Thanks Charlie.
> 
> FYI, the test I added actually does not run on Android. I think it might still
> be worthwhile having this but unfortunately, we do not run interactive ui
tests
> on Android.

Thanks for the heads up.  I agree it's worth having anyway-- if we get to where
we can run more of these tests on Android, we'll have coverage for this bug for
free.

https://codereview.chromium.org/2536943004/diff/60001/content/browser/rendere...
File content/browser/renderer_host/text_input_manager.h (right):

https://codereview.chromium.org/2536943004/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.h:133: //
ui::TEXT_INPUT_TYPE_NONE..
nit: Two periods.

[EhsanK, 2016-11-30 01:08:21]

Thanks for the reviews!

https://codereview.chromium.org/2536943004/diff/60001/content/browser/rendere...
File content/browser/renderer_host/text_input_manager.h (right):

https://codereview.chromium.org/2536943004/diff/60001/content/browser/rendere...
content/browser/renderer_host/text_input_manager.h:133: //
ui::TEXT_INPUT_TYPE_NONE..
On 2016/11/30 00:57:51, Charlie Reis (slow) wrote:
> nit: Two periods.

Thanks! Done.

[EhsanK, 2016-11-30 01:08:39]

The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-11-30 01:09:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[EhsanK, 2016-11-30 01:19:21]

Description was changed from

==========
Fix a crash occuring during the destrution of TextInputManager on Android.

On Android, in the observer call
RenderWidgetHostViewAndroid::OnUpdateTextInputState we were using
|updated_view| passed by TextInputManager to query TextInputState. This
is correct as long as the view is not unregistered already, which
happens when
  1- If the |active_view_| is destroyed.
  2- If TextInputManager is destoryed while there is |active_view_|
  which will lead to unregistering the view.

This patch will change the way we obtain TextInputState in
RenderWidgetHostViewAndroid. We now first verify if there is an active
view/widget and then get the state. Otherwise, the default state (of
NONE) is reported.

This patch also adds a test to catch the regression.

BUG=669375, 602723, 578168
==========

to

==========
Fix a crash occuring during the destruction of TextInputManager on Android.

On Android, in the observer call
RenderWidgetHostViewAndroid::OnUpdateTextInputState we were using
|updated_view| passed by TextInputManager to query TextInputState. This
is correct as long as the view is not unregistered already, which
happens when
  1- If the |active_view_| is destroyed.
  2- If TextInputManager is destoryed while there is |active_view_|
  which will lead to unregistering the view.

This patch will change the way we obtain TextInputState in
RenderWidgetHostViewAndroid. We now first verify if there is an active
view/widget and then get the state. Otherwise, the default state (of
NONE) is reported.

This patch also adds a test to catch the regression.

BUG=669375, 602723, 578168
==========

[EhsanK, 2016-11-30 01:20:50]

The CQ bit was unchecked by ekaramad@chromium.org

[EhsanK, 2016-11-30 01:20:50]

The CQ bit was checked by ekaramad@chromium.org

[EhsanK, 2016-11-30 01:20:50]

The patchset sent to the CQ was uploaded after l-g-t-m from creis@chromium.org
Link to the patchset: https://codereview.chromium.org/2536943004/#ps80001
(title: "Fixed a typo.")

[commit-bot: I haz the power, 2016-11-30 01:21:14]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 03:11:23]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 03:11:24]

Try jobs failed on following builders:
  blimp_linux_dbg on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build
URL)
  chromeos_amd64-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  chromeos_x86-generic_chromium_compile_only_ng on
master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)
  linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_clobber_rel_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux
(JOB_TIMED_OUT, no build URL)
  linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no
build URL)

[EhsanK, 2016-11-30 04:47:10]

The CQ bit was checked by ekaramad@chromium.org

[commit-bot: I haz the power, 2016-11-30 04:47:54]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 06:49:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-11-30 06:49:09]

Exceeded global retry quota

[EhsanK, 2016-11-30 07:32:14]

The CQ bit was checked by ekaramad@chromium.org

[commit-bot: I haz the power, 2016-11-30 07:33:09]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-11-30 08:44:44]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1480491134899540,
"parent_rev": "5664e67533b242aa54a50a133fe7c92abf10b894", "commit_rev":
"76985e2fe486c1aff7ef8b12ab5882847e8b3f7b"}

[commit-bot: I haz the power, 2016-11-30 08:45:08]

Description was changed from

==========
Fix a crash occuring during the destruction of TextInputManager on Android.

On Android, in the observer call
RenderWidgetHostViewAndroid::OnUpdateTextInputState we were using
|updated_view| passed by TextInputManager to query TextInputState. This
is correct as long as the view is not unregistered already, which
happens when
  1- If the |active_view_| is destroyed.
  2- If TextInputManager is destoryed while there is |active_view_|
  which will lead to unregistering the view.

This patch will change the way we obtain TextInputState in
RenderWidgetHostViewAndroid. We now first verify if there is an active
view/widget and then get the state. Otherwise, the default state (of
NONE) is reported.

This patch also adds a test to catch the regression.

BUG=669375, 602723, 578168
==========

to

==========
Fix a crash occuring during the destruction of TextInputManager on Android.

On Android, in the observer call
RenderWidgetHostViewAndroid::OnUpdateTextInputState we were using
|updated_view| passed by TextInputManager to query TextInputState. This
is correct as long as the view is not unregistered already, which
happens when
  1- If the |active_view_| is destroyed.
  2- If TextInputManager is destoryed while there is |active_view_|
  which will lead to unregistering the view.

This patch will change the way we obtain TextInputState in
RenderWidgetHostViewAndroid. We now first verify if there is an active
view/widget and then get the state. Otherwise, the default state (of
NONE) is reported.

This patch also adds a test to catch the regression.

BUG=669375, 602723, 578168
==========

[commit-bot: I haz the power, 2016-11-30 08:45:12]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-11-30 08:50:17]

Description was changed from

==========
Fix a crash occuring during the destruction of TextInputManager on Android.

On Android, in the observer call
RenderWidgetHostViewAndroid::OnUpdateTextInputState we were using
|updated_view| passed by TextInputManager to query TextInputState. This
is correct as long as the view is not unregistered already, which
happens when
  1- If the |active_view_| is destroyed.
  2- If TextInputManager is destoryed while there is |active_view_|
  which will lead to unregistering the view.

This patch will change the way we obtain TextInputState in
RenderWidgetHostViewAndroid. We now first verify if there is an active
view/widget and then get the state. Otherwise, the default state (of
NONE) is reported.

This patch also adds a test to catch the regression.

BUG=669375, 602723, 578168
==========

to

==========
Fix a crash occuring during the destruction of TextInputManager on Android.

On Android, in the observer call
RenderWidgetHostViewAndroid::OnUpdateTextInputState we were using
|updated_view| passed by TextInputManager to query TextInputState. This
is correct as long as the view is not unregistered already, which
happens when
  1- If the |active_view_| is destroyed.
  2- If TextInputManager is destoryed while there is |active_view_|
  which will lead to unregistering the view.

This patch will change the way we obtain TextInputState in
RenderWidgetHostViewAndroid. We now first verify if there is an active
view/widget and then get the state. Otherwise, the default state (of
NONE) is reported.

This patch also adds a test to catch the regression.

BUG=669375, 602723, 578168

Committed: https://crrev.com/db49e17a8e2c5874585e5a499171163ce70104ec
Cr-Commit-Position: refs/heads/master@{#435178}
==========

[commit-bot: I haz the power, 2016-11-30 08:50:18]

Patchset 5 (id:??) landed as
https://crrev.com/db49e17a8e2c5874585e5a499171163ce70104ec
Cr-Commit-Position: refs/heads/master@{#435178}