1453629 - Assertion failure: aCompositionEvent->mMessage != eCompositionStart running dom/base/test/chrome/test_nsITextInputProcessor.xul test

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, enhancement, P3)

Description

[Jon Coppeard (:jonco)]

I get an assertion failure running this test locally with:

MOZ_HEADLESS=1 ./mach mochitest -f chrome dom/base/test/chrome/test_nsITextInputProcessor.xul

I see TEST-UNEXPECTED-FAIL but the test run is reported as passing.

Comment 1

[Jon Coppeard (:jonco)]

Attachment: log.txt

Test output.

Comment 2

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

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

Comment 3

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

Wow, this hits really long standing bug of IME testing path.

IMEStateManager::DispatchCompositionEvent() creates TextComposition with native IME context stored by eCompositionStart event. However, when the composition ends, it looks for ending TextComposition instance with result of nsIWidget::GetNativeIMEContext() and the widget is stored by eCompositionChange(AsIs) event. The former is initialized by TextEventDispatcher::InitEvent().  When TextEventDispatcher works with TextInputProcessor, this native IME context is set to pseudo IME context which is raw pointer of TextEventDispatcher and process ID.  On the other hand, nsIWidget::GetNativeIMEContext() never returns pseudo IME context.  So, we fail to remove TextComposition when we synthesize composition for tests!

Comment 4

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

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

Comment 5

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

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

Comment 6

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

Attachment: Bug 1453629 - nsIWidget::GetNativeIMEContext() should return pseudo IME context if TextInputProcessor has a composition on the widget

Key of TextCompositionArrary to search composition on widget is native IME
context.  However, if TextInputProcessor dispatches composition events via
TextEventDispatcher, TextEventDispatcher::InitEvent() sets native IME context
of dispatching composition events to pseudo IME context (that's raw pointer
of TextEventDispatcher and process ID).

IMEStateManager::DispatchCompositionEvent() looks for existing TextComposition
with native IME context stored in WidgetCompositionEvent before dispatching
an event.  However, after dispatching it, it looks for remaining TextComposition
instance with widget stored in WidgetCompositionEvent.  Then,
TextCompositionArrary::IndexOf(nsIWidget*) will look for an instance with
the result of nsIWidget::GetNativeIMEContext() and this never returns actual
native IME context.  Therefore, IMEStateManager::DispatchCompositionEvent()
always fails to remove TextComposition instance from the array even after
a test composition is finished.

This patch moves nsIWidget::GetNativeIMEContext() to nsBaseWidget to refer
nsBaseWidget::mTextEventDispatcher makes it return pseudo IME context if
TextInputProcessor has input transaction.  Therefore, IMEStateManager becomes
always removes TextComposition from the array when every composition ends.

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

Comment 7

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

Some points:

The patch should be reviewed by smaug, but he's on vacation. So, I request the review to Makoto-san. The idea of IME context is, it depends on platforms how many composition we can have. For example, on Windows, IME can have only one but on Linux, we can have a composition per top level window. An IME context is shared between any widgets which is associated with native IME context. It was implemented in bug 1179632:
https://bugzilla.mozilla.org/attachment.cgi?id=8695807&action=diff
https://bugzilla.mozilla.org/attachment.cgi?id=8695915&action=diff

And even with fixing this bug, I see timeout of the test with headless widget.

Comment 8

[Makoto Kato [:m_kato]]

Comment on attachment 8992859 [details]
Bug 1453629 - nsIWidget::GetNativeIMEContext() should return pseudo IME context if TextInputProcessor has a composition on the widget

https://reviewboard.mozilla.org/r/257708/#review264884

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bdf8618dc2c3
nsIWidget::GetNativeIMEContext() should return pseudo IME context if TextInputProcessor has a composition on the widget r=m_kato

Comment 10

[Raul Gurzau (:RaulG)]

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