Closed Bug 1166323 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: User events and focus handling, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
e10s m8+ ---
firefox41 --- fixed
firefox42 --- fixed

People

(Reporter: dbaron, Assigned: m_kato)

References

Details

(Keywords: inputmethod)

Attachments

(2 files, 1 obsolete file)

+++ 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.
Whiteboard: 北京
Whiteboard: 北京
Humm, GTK widget is still compositing state, but child process isn't.  This isn't same as bug 1121313.
Assignee: nobody → m_kato
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.
Attachment #8610987 - Flags: review?(masayuki)
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.
Attachment #8610987 - Attachment is obsolete: true
Attachment #8610987 - Flags: review?(masayuki)
Comment on attachment 8611037 [details] [diff] [review]
Remove IME sequence number

Remove IME sequence number
Attachment #8611037 - Flags: review?(masayuki)
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.
Attachment #8611037 - Flags: review?(nchen)
Attachment #8611037 - Flags: review?(masayuki)
Attachment #8611037 - Flags: review+
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.
Attachment #8611037 - Flags: review?(nchen) → review+
https://hg.mozilla.org/mozilla-central/rev/1dc5cbdf0dd8
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Depends on: 1184890
Comment on attachment 8644070 [details] [diff] [review]
Fix mistake previous landed

need review?  Also, I should fix aurora tool.
Attachment #8644070 - Flags: review?(dkeeler)
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.
Attachment #8644070 - Flags: review?(dkeeler) → review+
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
Attachment #8644070 - Flags: approval-mozilla-aurora?
Comment on attachment 8644070 [details] [diff] [review]
Fix mistake previous landed

cancel due to backed out
Attachment #8644070 - Flags: approval-mozilla-aurora?
(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.
Attachment #8644070 - Flags: approval-mozilla-aurora?
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.
Attachment #8644070 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
Target Milestone: mozilla41 → mozilla42
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.