DO NOT SUBMIT: Handle nested message correctly for OnRequestTextInputState()

Handle nested message correctly for OnRequestTextInputState()

The calls to OnMessageReceived can be nested: OnHandleInputEvent() may
call OnRequestTextInputState() in the following scenario:

1) IME app calls ThreadedInputConnection#setComposingText().
2) IME app calls ThreadedInputConnection#endBatchEdit().
3) From 1), OnHandleInputEvent() calls JavaScript keyup event listener.
4) The event listener calls 'alert()'.
5) RenderFrameImpl::RunJavaScriptMessage() is called.
6) RenderViewImpl::SendAndRunNestedMessageLoop() is called.
7) Now RenderWidget::OnMessageReceived() calls OnRequestTextInputState()
   due to 2).

BUG=551193

[Changwan Ryu, 2016-04-07 10:38:36]

changwan@chromium.org changed reviewers:
+ aelias@chromium.org

[Changwan Ryu, 2016-04-07 10:38:37]

I found this failure case while testing something for another issue. PTAL.

[Changwan Ryu, 2016-04-07 11:48:17]

changwan@chromium.org changed reviewers:
+ sievers@chromium.org

[Changwan Ryu, 2016-04-07 11:48:18]

https://codereview.chromium.org/1865143003/diff/1/content/renderer/render_wid...
File content/renderer/render_widget.cc (left):

https://codereview.chromium.org/1865143003/diff/1/content/renderer/render_wid...
content/renderer/render_widget.cc:1665: DCHECK(!ime_event_guard_);
FYI, this DCHECK was introduced in 
https://codereview.chromium.org/1278593004/diff2/260001:360001/content/render...

[aelias_OOO_until_Jul13, 2016-04-07 14:49:00]

aelias@chromium.org changed reviewers:
+ avi@chromium.org

[aelias_OOO_until_Jul13, 2016-04-07 14:49:00]

This scenario is giving me a headache, I don't know if we want to let the crazy
requirements of alert() infect the semantics of everything in the renderer --
the IME code is complicated enough without this requirement.  Avi currently has
a project to improve alert() (see
https://docs.google.com/document/d/1wtV5rmLhbf1OZkbg7crtCt6h1mMtig_ctTQt3BLLE...
).  Avi, is there some plan to get rid of the nested message loop as part of
that?

[Avi (use Gerrit), 2016-04-07 14:50:26]

On 2016/04/07 14:49:00, aelias wrote:
> This scenario is giving me a headache, I don't know if we want to let the
crazy
> requirements of alert() infect the semantics of everything in the renderer --
> the IME code is complicated enough without this requirement.  Avi currently
has
> a project to improve alert() (see
>
https://docs.google.com/document/d/1wtV5rmLhbf1OZkbg7crtCt6h1mMtig_ctTQt3BLLE...
> ).  Avi, is there some plan to get rid of the nested message loop as part of
> that?

I'm not sure we can right now. If we want confirm/prompt dialogs to work at all,
they need to return values to a paused JavaScript, so we still need a nested
message loop.

Eventually, yes, we should kill all dialogs like this, but that's not going to
happen for a long long time. :(

[aelias_OOO_until_Jul13, 2016-04-07 14:58:37]

On 2016/04/07 at 14:50:26, avi wrote:
> I'm not sure we can right now. If we want confirm/prompt dialogs to work at
all, they need to return values to a paused JavaScript, so we still need a
nested message loop.
> 
> Eventually, yes, we should kill all dialogs like this, but that's not going to
happen for a long long time. :(

Hmm.  I'd like to propose that you implement OldSpice by rewritten alert()
dialogs to be rendered entirely by the render process.  Many goals seem to
converge in that direction.  It would solve this reentrancy problem, it would
make the dialogs naturally contained within the web-area and with a
non-system-look (as you desire), and also remove the dependency on Aura which
implies additional work/code duplication on Android.

[Avi (use Gerrit), 2016-04-07 15:05:01]

On 2016/04/07 14:58:37, aelias wrote:
> On 2016/04/07 at 14:50:26, avi wrote:
> > I'm not sure we can right now. If we want confirm/prompt dialogs to work at
> all, they need to return values to a paused JavaScript, so we still need a
> nested message loop.
> > 
> > Eventually, yes, we should kill all dialogs like this, but that's not going
to
> happen for a long long time. :(
> 
> Hmm.  I'd like to propose that you implement OldSpice by rewritten alert()
> dialogs to be rendered entirely by the render process.  Many goals seem to
> converge in that direction.  It would solve this reentrancy problem, it would
> make the dialogs naturally contained within the web-area and with a
> non-system-look (as you desire), and also remove the dependency on Aura which
> implies additional work/code duplication on Android.

Perhaps, though that opens up lots of new questions about embedders of Chromium
Content, and implementation of things like Android WebView.

[aelias_OOO_until_Jul13, 2016-04-07 15:05:15]

I guess even in that world the Blink main thread still needs to freeze and block
on another thread though.  The most natural way to do that would be using the
normal sync IPC mechanism, why isn't that how it works?

Anyway there are probably more Reasons and this is obviously veering pretty far
off-topic from this patch.  Even if alert() is not fixed, I'd rather just
tolerate that we have a crash in this situation, than land this semantic change.
 It's difficult to reason about and could introduce real bugs in real scenarios
(AFAIK this just came up in a small test page, not a real user-facing site). 
Crashes like this are probably a dime a dozen and you just happened to stumble
onto one.

[Changwan Ryu, 2016-04-08 01:21:04]

On 2016/04/07 15:05:15, aelias wrote:
> I guess even in that world the Blink main thread still needs to freeze and
block
> on another thread though.  The most natural way to do that would be using the
> normal sync IPC mechanism, why isn't that how it works?
> 
> Anyway there are probably more Reasons and this is obviously veering pretty
far
> off-topic from this patch.  Even if alert() is not fixed, I'd rather just
> tolerate that we have a crash in this situation, than land this semantic
change.
>  It's difficult to reason about and could introduce real bugs in real
scenarios
> (AFAIK this just came up in a small test page, not a real user-facing site). 
> Crashes like this are probably a dime a dozen and you just happened to stumble
> onto one.

Hmm... I agree that it's alert() and other dialogs that should really be fixed
here. However,
calling alert() in event listeners is a common pattern used for novice
JavaScript learners.
(W3schools has such an example:
http://www.w3schools.com/jsref/event_onkeypress.asp)

And when they run this type of code, the IME thread will hang until they touch
location bar or go to the overview and come back.
It will be hard to track such issues, so I'm not sure if we want to leave this
as is.

[aelias_OOO_until_Jul13, 2016-04-08 23:02:59]

You're right, we shouldn't ignore it.  I'm trying to think of the sanest way to
handle it.

How about avoiding having the IME messages handled within the nested message
loop at all?  E.g. if we detect we are currently in the nested message loop, we
would repost the message to the outermost message loop.  Maybe look around and
see if there is a preexisting pattern along those lines we could imitate?

[Changwan Ryu, 2016-04-12 17:21:01]

On 2016/04/08 23:02:59, aelias wrote:
> You're right, we shouldn't ignore it.  I'm trying to think of the sanest way
to
> handle it.
> 
> How about avoiding having the IME messages handled within the nested message
> loop at all?  E.g. if we detect we are currently in the nested message loop,
we
> would repost the message to the outermost message loop.  Maybe look around and
> see if there is a preexisting pattern along those lines we could imitate?

Hmm... I'm afraid that reposting the message in the nested message loop would
change the sequence order of input messages.
Also I tried to find a similar pattern, but MessageLoop::IsNested() is not being
seriously used outside components/scheduler/.

[aelias_OOO_until_Jul13, 2016-04-12 18:03:25]

On 2016/04/12 at 17:21:01, changwan wrote:
> Hmm... I'm afraid that reposting the message in the nested message loop would
change the sequence order of input messages.

I'm thinking that ordering-wise, reposting out actually *fixes* the sequence
order.  Imagine an IME task is already queued up for the outer message loop when
the nested loop is entered (assuming that scenario is possible at all -- I'm not
sure of the implementation details).  Then, a message arrives to the nested
message loop -- it must have been posted later in time by the browser.  If we
process it immediately (status quo behavior), then the order is inverted.  We
actually need to repost it to the end of the outer message loop to preserve
original order.

[Changwan Ryu, 2016-06-01 07:50:45]

On 2016/04/12 18:03:25, aelias wrote:
> On 2016/04/12 at 17:21:01, changwan wrote:
> > Hmm... I'm afraid that reposting the message in the nested message loop
would
> change the sequence order of input messages.
> 
> I'm thinking that ordering-wise, reposting out actually *fixes* the sequence
> order.  Imagine an IME task is already queued up for the outer message loop
when
> the nested loop is entered (assuming that scenario is possible at all -- I'm
not
> sure of the implementation details).  Then, a message arrives to the nested
> message loop -- it must have been posted later in time by the browser.  If we
> process it immediately (status quo behavior), then the order is inverted.  We
> actually need to repost it to the end of the outer message loop to preserve
> original order.

Closing as https://codereview.chromium.org/1931793002 has been merged.

[Changwan Ryu, 2016-06-01 07:51:03]

Description was changed from

==========
Handle nested message correctly for OnRequestTextInputState()

The calls to OnMessageReceived can be nested: OnHandleInputEvent() may
call OnRequestTextInputState() in the following scenario:

1) IME app calls ThreadedInputConnection#setComposingText().
2) IME app calls ThreadedInputConnection#endBatchEdit().
3) From 1), OnHandleInputEvent() calls JavaScript keyup event listener.
4) The event listener calls 'alert()'.
5) RenderFrameImpl::RunJavaScriptMessage() is called.
6) RenderViewImpl::SendAndRunNestedMessageLoop() is called.
7) Now RenderWidget::OnMessageReceived() calls OnRequestTextInputState()
   due to 2).

BUG=551193
==========

to

==========
Handle nested message correctly for OnRequestTextInputState()

The calls to OnMessageReceived can be nested: OnHandleInputEvent() may
call OnRequestTextInputState() in the following scenario:

1) IME app calls ThreadedInputConnection#setComposingText().
2) IME app calls ThreadedInputConnection#endBatchEdit().
3) From 1), OnHandleInputEvent() calls JavaScript keyup event listener.
4) The event listener calls 'alert()'.
5) RenderFrameImpl::RunJavaScriptMessage() is called.
6) RenderViewImpl::SendAndRunNestedMessageLoop() is called.
7) Now RenderWidget::OnMessageReceived() calls OnRequestTextInputState()
   due to 2).

BUG=551193
==========