1166323 - [e10s] Assertion failure: aCompositionEvent->message == (2200), at IMEStateManager.cpp:935

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

+++ This bug was initially created as a clone of Bug #1121313 +++

I still see bug 1121313 every time I try to use IME.  I'm using Ubuntu, with ibus IME.

Steps to reproduce:
 1. load this bug
 2. click in the "Status Whiteboard" field
 3. press Super+Space to switch to Chinese Pinyin IME
 4. Type "beijing"
 5. press space to select the characters

Actual results: crash due to assertion failure:

Assertion failure: aCompositionEvent->message == (2200), at /home/dbaron/builds/ssd/mozilla-central/mozilla/dom/events/IMEStateManager.cpp:935

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron [:dbaron] ⏰UTC-4 (busy, returning May 21) from comment #0)
>  5. press space to select the characters

sorry, step 5 should have been:

 5. press "1" to select the characters


aCompositionEvent->message is actually 2204 when we expect it to be 2200.

Comment 2

[Makoto Kato [:m_kato]]

Humm, GTK widget is still compositing state, but child process isn't.  This isn't same as bug 1121313.

Comment 3

[Makoto Kato [:m_kato]]

Some composition events is discard by old hack of Fennec e10s, so this assertion occurs...

Comment 4

[Makoto Kato [:m_kato]]

After IME blur, some IME signals are fired before IME focus again.  So some "important" messages are discarded, then, at finally, IME state is broken.

Comment 5

[Makoto Kato [:m_kato]]

Original code was added by bug 599550 for Fennec e10s.

Comment 6

[Makoto Kato [:m_kato]]

Attachment: IME message should be discard correctly during no focus

Comment 7

[Makoto Kato [:m_kato]]

Comment on attachment 8610987 [details] [diff] [review]
IME message should be discard correctly during no focus

IME event message is discard during no focus by Bug 599550.  But the last IME event message isn't discard before getting IME blur.

Comment 8

[Makoto Kato [:m_kato]]

Also this situation is the following.

1. User changes focus from content to chrome
2. ibus-pinyin fires some signals (empty composition)
3. Gecko fires IME blur before 2. isn't processed on content
4. 2. is discard except to the last event (end composition).

Comment 9

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

I don't understand the patch well.

So, do you try to discard the following messages in PuppetWidget::DispatchEvent()? If so, I think that the TextComposition instance will be alive forever.

When TextComposition::RequestToCommit() is called, it sends a request to commit (or cancel) to the widget. However, it depends on each platform and/or the native IME whether the composition will be committed synchronously or asynchronously. For compatibility between platforms, TextComposition emulates to commit or cancel composition when the request isn't handled synchronously (this is always in e10s). In this case, TextComposition doesn't kill itself. Instead, it waits following messages and receive the messages because it shouldn't restart composition with delayed events. After it receives all remaining messages, it will kill itself. So, we have two ways to fix that.

1. PuppetWidget emulates synchronous commit/cancel for TextComposition and ignore following messages.
2. PuppetWidget should dispatch all events even after sending IME blur notification.

I like the #2 better if it doesn't case any regressions.

Comment 10

[Makoto Kato [:m_kato]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) (working slowly due to bad health condition) from comment #9)
> 1. PuppetWidget emulates synchronous commit/cancel for TextComposition and
> ignore following messages.
> 2. PuppetWidget should dispatch all events even after sending IME blur
> notification.
> 
> I like the #2 better if it doesn't case any regressions.

I think that better way is that bug 599550's code is removed.  Sequence number hack was for Android e10s.  Since current IMEStateMangaer expects IME event receives correct order, so this hack causes any unexpected behavior.

And I already test #2 way, the it works well.  Patch is coming.

Comment 11

[Makoto Kato [:m_kato]]

Attachment: Remove IME sequence number

Comment 12

[Makoto Kato [:m_kato]]

Comment on attachment 8611037 [details] [diff] [review]
Remove IME sequence number

Remove IME sequence number

Comment 13

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

Comment on attachment 8611037 [details] [diff] [review]
Remove IME sequence number

I really love this! But could you remove WidgetCompositionEvent::mSeqno and WidgetSelectionEvent::mSeqno?
http://mxr.mozilla.org/mozilla-central/ident?i=mSeqno

Anyway, we should ask review to Jim Chen too.

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

Comment on attachment 8611037 [details] [diff] [review]
Remove IME sequence number

Review of attachment 8611037 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM! I assume you already tested e10s IME with this patch.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1dc5cbdf0dd8

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1dc5cbdf0dd8

Comment 17

[Makoto Kato [:m_kato]]

Attachment: Fix mistake previous landed

Comment 18

[Makoto Kato [:m_kato]]

Comment on attachment 8644070 [details] [diff] [review]
Fix mistake previous landed

need review?  Also, I should fix aurora tool.

Comment 19

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8644070 [details] [diff] [review]
Fix mistake previous landed

Review of attachment 8644070 [details] [diff] [review]:
-----------------------------------------------------------------

Great - thanks. And please do uplift to aurora.

Comment 20

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9618f92995ab

Comment 21

[Makoto Kato [:m_kato]]

Comment on attachment 8644070 [details] [diff] [review]
Fix mistake previous landed

Approval Request Comment
[Feature/regressing bug #]:
Bug 1166323

[User impact if declined]:
Original landing of this bug has expected change by my mistake.  So revert to correct.

If debug build, we should assert this code if no EV root.

[Describe test coverage new/current, TreeHerder]:
landed in m-i. and this code is only debug build

[Risks and why]: 
No.  reverted to original.

[String/UUID change made/needed]:
No

Comment 22

[Makoto Kato [:m_kato]]

Comment on attachment 8644070 [details] [diff] [review]
Fix mistake previous landed

cancel due to backed out

Comment 23

[Carsten Book [:Tomcat]]

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=12612244&repo=mozilla-inbound

Comment 24

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e166fef1a0f

Comment 25

[Makoto Kato [:m_kato]]

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

Comment 26

[:Cykesiopka]

(In reply to Makoto Kato [:m_kato] from comment #25)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfa8884f9423

FWIW, this probably isn't enough - the issue here is that Bug 1164609 needs to be fixed first, or the assert here will fire at runtime.

I will attach my patch there shortly. Sorry for the trouble!

Comment 27

[Makoto Kato [:m_kato]]

(In reply to Cykesiopka from comment #26)
> (In reply to Makoto Kato [:m_kato] from comment #25)
> > https://treeherder.mozilla.org/#/jobs?repo=try&revision=cfa8884f9423
> 
> FWIW, this probably isn't enough - the issue here is that Bug 1164609 needs
> to be fixed first, or the assert here will fire at runtime.
> 
> I will attach my patch there shortly. Sorry for the trouble!

Thanks for your comment. I will wait for fixing bug 1164609.

Comment 28

[Makoto Kato [:m_kato]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43e9b4e2b03d

It seems to be fixed.  I will re-land this.

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7e77838e7e2e

Comment 30

[Makoto Kato [:m_kato]]

Comment on attachment 8644070 [details] [diff] [review]
Fix mistake previous landed

Approval Request Comment
[Feature/regressing bug #]:
Bug 1166323

[User impact if declined]:
On debug build, EV root error assertion doesn't work.

When landing this bug's fix, it had unexpected code change.

[Describe test coverage new/current, TreeHerder]:
landed in m-i

[Risks and why]: 
No.  This is debug build only

[String/UUID change made/needed]:
No.

Comment 31

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Looks like this bug was reopened even though it was fixed in Nightly41, resetting status-firefox41 to "Affected".

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7e77838e7e2e

Comment 33

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8644070 [details] [diff] [review]
Fix mistake previous landed

[Triage Comment]
Debug build only assertions, should be safe to uplift to Beta41.

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/5440624ee3a8