+++ 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

(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.

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

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

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.

Original code was added by bug 599550 for Fennec e10s.

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.

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).

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.

(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 on attachment 8611037 [details] [diff] [review] Remove IME sequence number Remove IME sequence number

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 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 on attachment 8644070 [details] [diff] [review] Fix mistake previous landed need review? Also, I should fix aurora tool.

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 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 on attachment 8644070 [details] [diff] [review] Fix mistake previous landed cancel due to backed out

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

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

(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!

(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.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=43e9b4e2b03d It seems to be fixed. I will re-land this.

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.

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

Comment on attachment 8644070 [details] [diff] [review] Fix mistake previous landed [Triage Comment] Debug build only assertions, should be safe to uplift to Beta41.