1286730 - NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED isn't notified after committing composition

Status: RESOLVED FIXED

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

Description

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

This is a regression of bug 1275906.

The fix of bug 1275906 made NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED sent to widget asynchronously.

However, IMEStateManager checks if there is a valid TextComposition at sending it <https://dxr.mozilla.org/mozilla-central/rev/88bebcaca249aeaca9197382e89d35b02be8292e/dom/events/IMEStateManager.cpp#1452-1453,1457> but TextComposition has gone immediately after composition event is sent to the DOM tree or remote process <https://dxr.mozilla.org/mozilla-central/rev/88bebcaca249aeaca9197382e89d35b02be8292e/dom/events/IMEStateManager.cpp#1170-1172,1188,1196-1197>.

So, IMEStateManager should stop checking if there is a valid TextComposition at sending IME notifications like the others.

And this bug causes a serious regression of bug 1224994. The cached content is never synced with actual content due to this bug.

Comment 1

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

[Tracking Requested - why for this release]:
This is serious regression. TSFTextStore releases a lot of cache when it receives NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED but it's not notified after compositionend. That means TSFTextStore loses a chance of releasing unnecessary cache.

Comment 2

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

Hmm, this is non-e10s only bug. So, this shouldn't cause of bug 1286464...

Comment 3

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

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

Comment 4

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

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

Comment 5

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

Attachment: Bug 1286730 IMEStateManager::NotifyIME() should treat NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED same as the other notifications because all of them are notified by IMEContentObserver asynchronously

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

Comment 6

[Olli Pettay [:smaug][[e-mail]](vacation until Nov 16)]

Comment on attachment 8770934 [details]
Bug 1286730 IMEStateManager::NotifyIME() should treat NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED same as the other notifications because all of them are notified by IMEContentObserver asynchronously

https://reviewboard.mozilla.org/r/64236/#review61290

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d5cf90154e67
IMEStateManager::NotifyIME() should treat NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED same as the other notifications because all of them are notified by IMEContentObserver asynchronously r=smaug

Comment 8

[Marcia Knous [:marcia]]

Tracking 49/50+ for this cache regression.

Comment 9

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

Oh, this is important for e10s too. Even in e10s mode, this is reproduce in remote process, then, never sent the notification to the chrome process, of course.

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[David Bolter [:davidb] (NeedInfo me for attention)]

Please request uplift to 49.

Comment 12

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

Attachment: Patch for aurora

Approval Request Comment
[Feature/regressing bug #]: Regression of bug 1275906.
[User impact if declined]: The notification is used for a timing to clear cache in TSFTextStore. Especially, after bug 1224994, this is the only chance to clear cache. The cache may be big and doesn't include content changes without IME. So, for example, reconvert of selected text may not work as expected by this bug.
[Describe test coverage new/current, TreeHerder]: Already landed on m-c. And I dogfooded with patched Aurora build without e10s for a couple of days too.
[Risks and why]: Low because bug 1275906 made NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED treat same as other notifications at sending (made it sent asynchronously). However, actual sending path isn't changed to async-aware. This adds the notification to *existing* async path.
[String/UUID change made/needed]: Nothing.


# Thank you for your ping, I completely forgot to do that.

Comment 13

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

Ah, the reason why we need another patch for Aurora is, bug 1286489. The bug changed the log messages which are removed by the patch.

Comment 14

[Gerry Chang [:gchang]]

Comment on attachment 8772707 [details] [diff] [review]
Patch for aurora

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

This patch fixes a regression. Take it in aurora.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4468d257b795