Fix crash on clicking on <select> tag

Fix crash on clicking on <select> tag

BUG=626826
TEST=Manually, no more crashes observed.

Committed: https://crrev.com/d4686b6fec71bb12842f140f617709610bcd187a
Cr-Commit-Position: refs/heads/master@{#405778}

[khmel, 2016-07-11 19:33:12]

khmel@chromium.org changed reviewers:
+ xiyuan@chromium.org

[khmel, 2016-07-11 19:33:12]

Hi Xiyuan,

PTAL

PS. Could you please recommend owner for this part?

https://codereview.chromium.org/2136193002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2136193002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:2463: if
(!window_)
Here is a race condition.

When I click on <select> tag as described in bug, RenderWidget is created and
shown. However it is done in 2 steps. First step is to create widget (IPC
redirects to RenderWidgetHostViewAura CTOR). Second step is to show widget (IPC
redirects to RenderWidgetHostViewAura::CreateAuraWindow).
In RenderWidgetHostViewAura CTOR observer for TextInputManager is registered. 

Crash happens when periodic RenderWidget::UpdateTextInputState is called between
step 1 and 2. In this case TextInputManager notifies RenderWidgetHostViewAura
which assumes that window_ is created.

[xiyuan, 2016-07-11 20:02:40]

xiyuan@chromium.org changed reviewers:
+ yusukes@chromium.org

[xiyuan, 2016-07-11 20:02:41]

+yusukes

Looks good but let's see what yusukes thinks since he is more familiar with the
code.

[Yusuke Sato, 2016-07-12 00:49:09]

yusukes@chromium.org changed reviewers:
+ shuchen@chromium.org

[Yusuke Sato, 2016-07-12 00:49:09]

+shuchen

(I'm one of the original authors of the IME code, but haven't touched it for
more than 4 years now. I think shuchen@ would be the best person for reviewing
this.)

[Shu Chen, 2016-07-13 05:56:16]

https://codereview.chromium.org/2136193002/diff/1/content/browser/renderer_ho...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2136193002/diff/1/content/browser/renderer_ho...
content/browser/renderer_host/render_widget_host_view_aura.cc:2463: if
(!window_)
On 2016/07/11 19:33:12, khmel wrote:
> Here is a race condition.
> 
> When I click on <select> tag as described in bug, RenderWidget is created and
> shown. However it is done in 2 steps. First step is to create widget (IPC
> redirects to RenderWidgetHostViewAura CTOR). Second step is to show widget
(IPC
> redirects to RenderWidgetHostViewAura::CreateAuraWindow).
> In RenderWidgetHostViewAura CTOR observer for TextInputManager is registered. 
> 
> Crash happens when periodic RenderWidget::UpdateTextInputState is called
between
> step 1 and 2. In this case TextInputManager notifies RenderWidgetHostViewAura
> which assumes that window_ is created.
> 

Thanks for fixing this!

The InputMethod is necessary to let the web contents receive the correct
keydown/keypress/keyup. So if NULL is returned here, the web contents would not
receive any key events. Thus, the typing-to-select-item feature might be broken.

Can you please confirm there are more RenderWidget::UpdateTextInputState() calls
following the step 2?

I think the ideal fix is to make sure the UpdateTextInputState() is always
called after step 2. But I'm OK with the solution here if
RenderWidget::UpdateTextInputState() is periodic and after step 2.

Btw, it will be nice if you can add a test for this change.

[khmel, 2016-07-14 00:29:09]

Hi Shu,

Thank you for your review.

I added test that fails with very high probability (Debug/Release) without the
fix.

PTAL

[Shu Chen, 2016-07-14 02:06:30]

lgtm

[khmel, 2016-07-14 03:24:06]

khmel@chromium.org changed reviewers:
+ jochen@chromium.org

[khmel, 2016-07-14 03:24:07]

Thanks Shu for review.

I still need OWNER approval. 

Adding jochen@

PTAL

[jochen (gone - plz use gerrit), 2016-07-14 15:01:51]

not lgtm due to layering violation.

can you rewrite the test as a content_browsertest?

https://codereview.chromium.org/2136193002/diff/20001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/DEPS (right):

https://codereview.chromium.org/2136193002/diff/20001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/DEPS:2: "+content/common/view_messages.h",
chrome/ can't depend on content/

https://codereview.chromium.org/2136193002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2136193002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2464: return NULL;
nullptr please

[khmel, 2016-07-14 21:06:45]

Thanks for your review.

It is not easy to move this test to content_browsertests because required
functionality to load platform apps is missed there and adding it is not trivial
task. Also I did not find any way to create webview tag in that tests. So I
refactored this test to be based on aura window observer (I think this event
better because we wait for real window appearance, not for signal to show it).
Also I added using click simulation with different duration because as I found
it really affects sequence of UpdateTextInputState calls.

PTAL

https://codereview.chromium.org/2136193002/diff/20001/chrome/browser/apps/gue...
File chrome/browser/apps/guest_view/DEPS (right):

https://codereview.chromium.org/2136193002/diff/20001/chrome/browser/apps/gue...
chrome/browser/apps/guest_view/DEPS:2: "+content/common/view_messages.h",
On 2016/07/14 15:01:51, jochen wrote:
> chrome/ can't depend on content/

Done.

https://codereview.chromium.org/2136193002/diff/20001/content/browser/rendere...
File content/browser/renderer_host/render_widget_host_view_aura.cc (right):

https://codereview.chromium.org/2136193002/diff/20001/content/browser/rendere...
content/browser/renderer_host/render_widget_host_view_aura.cc:2464: return NULL;
On 2016/07/14 15:01:51, jochen wrote:
> nullptr please

Done.

[jochen (gone - plz use gerrit), 2016-07-15 14:34:01]

lgtm

[khmel, 2016-07-15 14:56:36]

The CQ bit was checked by khmel@chromium.org

[khmel, 2016-07-15 14:56:37]

The patchset sent to the CQ was uploaded after l-g-t-m from shuchen@chromium.org
Link to the patchset: https://codereview.chromium.org/2136193002/#ps40001
(title: "refactored test")

[commit-bot: I haz the power, 2016-07-15 14:56:57]

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-07-15 16:35:20]

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

[commit-bot: I haz the power, 2016-07-15 16:35:21]

Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[khmel, 2016-07-15 16:37:16]

The CQ bit was checked by khmel@chromium.org

[commit-bot: I haz the power, 2016-07-15 16:37:58]

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-07-15 17:10:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-07-15 17:10:27]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-15 17:13:12]

Description was changed from

==========
Fix crash on clicking on <select> tag

BUG=626826
TEST=Manually, no more crashes observed.
==========

to

==========
Fix crash on clicking on <select> tag

BUG=626826
TEST=Manually, no more crashes observed.

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

[commit-bot: I haz the power, 2016-07-15 17:13:13]

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