1447866 - IME crash on text input with e10s

Status: RESOLVED FIXED

(GeckoView :: IME, defect, P1)

Description

[Eugen Sawin [:esawin]]

STR
1. Launch geckoview_example on https://google.com with multiprocess enabled.
2. Input a search query.

Crashes with:

Assertion failure: !aEvent->AsKeyboardEvent() || aEvent->mFlags.mIsSynthesizedForTests || aEvent->AsKeyboardEvent()->AreAllEditCommandsInitialized() (Non-sysnthesized keyboard events should have edit commands for all types before dispatched), at /home/esawin/dev/g/widget/PuppetWidget.cpp:346

That's a regression from https://hg.mozilla.org/mozilla-central/rev/00456fae72f5

Comment 1

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1446478-2.0-Adjust-docs-for-changed-onLoadReques.patch

In case that this is not an easy fix, we should disable the key events in the meanwhile to fix the regression.

Comment 2

[Eugen Sawin [:esawin]]

Attachment: 0001-Bug-1447866-1.0-Disable-dummy-key-events-during-comp.patch

Sorry, this is this correct patch.

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Ah, really sorry for my regression.

However, perhaps, the right approach to fix this bug is, call WidgetKeyboardEvent.PreventNativeKeyBindings() before dispatching eKeyDown and eKeyUp event. Then, AreAllEditCommandsInitialized() will return true in this assertion and no shortcut keys won't be executed with NativeKeyBindings.

Comment 4

[Eugen Sawin [:esawin]]

Would you be able to write the patch?
I can test and review it or forward it to Jim when he's back from PTO.

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sure.

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=97a2aea0bfed9fcc6a2156dfe47f6c06c86651f2

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: Bug 1447866 - GeckoEditableSupport::SendIMEDummyKeyEvent() should set native key bindings to none before dispatching keyboard events which are marked as "processed by IME"

When EditorEventListener receives keyboard events, it tries to execute
native key bindings which are stored by each keyboard event.  For performance
reason in e10s, TabParent initializes native key binding information of every
keyboard event before sending it to the remote process.  Therefore,
PuppetWidget checks if every keyboard event has native key binding information.
However, the native key binding information of dummy keyboard events marked as
"processed by IME" on Android are never initialized before sending PuppetWidget.
Therefore, we hit MOZ_ASSERT in PuppetWidget.

This patch makes GeckoEditableSuppor::SendIMEDummyKeyEvent() set native key
binding information to "none" before dispatching every keyboard event since
keyboard events during composition shouldn't cause any edit actions.

Review commit: https://reviewboard.mozilla.org/r/230530/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/230530/

Comment 8

[Eugen Sawin [:esawin]]

Comment on attachment 8961661 [details]
Bug 1447866 - GeckoEditableSupport::SendIMEDummyKeyEvent() should set native key bindings to none before dispatching keyboard events which are marked as "processed by IME"

https://reviewboard.mozilla.org/r/230530/#review236120

It fixes the issue, thanks!

::: commit-message-9b69a:12
(Diff revision 1)
> +PuppetWidget checks if every keyboard event has native key binding information.
> +However, the native key binding information of dummy keyboard events marked as
> +"processed by IME" on Android are never initialized before sending PuppetWidget.
> +Therefore, we hit MOZ_ASSERT in PuppetWidget.
> +
> +This patch makes GeckoEditableSuppor::SendIMEDummyKeyEvent() set native key

missing t

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8961661 [details]
Bug 1447866 - GeckoEditableSupport::SendIMEDummyKeyEvent() should set native key bindings to none before dispatching keyboard events which are marked as "processed by IME"

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/230530/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d83bf99d438d
GeckoEditableSupport::SendIMEDummyKeyEvent() should set native key bindings to none before dispatching keyboard events which are marked as "processed by IME" r=esawin

Comment 11

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/d83bf99d438d

Comment 12

[Chris Peterson [:cpeterson]]

Moving some IME bugs to the new GeckoView::IME component.