1242331 - [e10s][TSF] When typing fast, IME composition may be committed unexpectedly and input won't cause text input after that

Status: RESOLVED FIXED

(Core :: Widget: Win32, defect)

Description

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

I sometimes reproduce this bug when the content process is busy.

For example, I can reproduce this bug with following STR:

1. Launch debug build
2. Open TweetDeck
3. Open Twitter.com
4. Type Japanese with TSF and commit it
5. Turn off IME and type spacebar
6. Turn on IME and type some Japanese characters

Do very fast during #4 - #6. Then, the text change notification for #5 will be notified during #6. At this time, TSFTextStore tries to commit the composition forcibly but after that, TSF won't work until removing "hidden" composition string by Backspace key presses.

Comment 1

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

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

Comment 2

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

Attachment: part.1 Remove unused methods of IMENotification

First, we should remove *unused* methods which makes both SelectionChangeDataBase and TextChangeDataBase have same members.

Comment 3

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

Attachment: part.2 Rename TextChangeDataBase::mCausedByComposition to mCausedOnlyByComposition

TextChangeDataBase may be merged multiple text changes in the editor. So, making mCausedByComposition's meaning clearer, we should rename it to mCausedOnlyByComposition.

Comment 4

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

Attachment: part.3 Rename TextChangeDataBase::mOccurredDuringComposition to mIncludingChangesDuringComposition

mOccurredDuringComposition isn't unclear like mCausedByComposition. However, a view from widget, this should indicate if the change includes some changes which is NOT caused by composition during the *latest* composition.

Therefore, I rename this to mIncludingChangesDuringComposition. Although, mIncludingNonCompositionChangesDuringComposition is better for self-documentation, but it's too long for our coding rules :-(

Comment 5

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

Attachment: part.4 Add TextChangeDataBase::mIncludingChangesWithoutComposition

We should think that any changes which occurred before starting current composition is changed by *unknown* sources. By this approach, widget can notify IME of text changes as not caused by *current* composition even if they are late to notify IME due to asynchronous e10s.

So, for making such delayed notifications notify IME later, we should record it to mIncludingChangesWithoutComposition. I have a plan to do that in next Q2.

Comment 6

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

Attachment: part.5 TSFTextStore should ignore text changes not caused by composition but not occurred during current composition

If (merged) text change notification is received during composition, even if the notification includes some changes which occurred *before* starting composition, TSFTextStore should ignore it.

Ideally, TSFTextStore should store the text changes and notify TSF of them after ending current composition and adjust offset of queries from TIP with the changes. However, it requires a lot of work. I have a plan to redesign TSFTextStore for that, but I don't have much time. I'll perhaps do it in Q2. So, for now, we should just ignore the delayed notifications. (I don't find any regressions with Japanese TIPs.)

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8712120 [details] [diff] [review]
part.2 Rename TextChangeDataBase::mCausedByComposition to mCausedOnlyByComposition

+    // Note that TextChangeDataBase may be merged two or more changes
+    // especially in e10s mode.
sounds a bit odd.

Perhaps 
Note that TextChangeDataBase may be the result of merging two or more changes
especially in e10s mode.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8712122 [details] [diff] [review]
part.3 Rename TextChangeDataBase::mOccurredDuringComposition to mIncludingChangesDuringComposition

mIncludingNonCompositionChangesDuringComposition would be indeed better but starting to get rather long, so mIncludingChangesDuringComposition is fine.

"there is some changes" s/is/are/

-      mOccurredDuringComposition = aOccurredDuringComposition;
+      mIncludingChangesDuringComposition =
+        !aCausedByComposition && aOccurredDuringComposition;
Aha, this isn't just rename

+  // mIncludingChangesDuringComposition should be true when at least one of
+  // merged changes occurred during the latest composition.
'.. one of the merged...'
But should it be
"mIncludingChangesDuringComposition should be true when at least one of
 the merged non-composition changes occurred during the latest composition."



"IME doesn't want need outdated text changes as text change during"
IME doesn't want outdated...


"So, we should treat the outdated text changes occurred not during composition."
that is a bit oddly said, but don't have good suggestion to fix that. Perhaps drop that sentence.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8712125 [details] [diff] [review]
part.4 Add TextChangeDataBase::mIncludingChangesWithoutComposition

>+    // mIncludingChangesWithoutComposition is true if at least one change which
>+    // is not caused when there is no composition.
>+    bool mIncludingChangesWithoutComposition;
s/if at least one/if there is at least one/


s/which is not caused when there is no composition/which did occur when there wasn't composition ongoing/
Something like that perhaps?


>   if (!newData.mCausedOnlyByComposition &&
>       !newData.mIncludingChangesDuringComposition) {
>     // If new change is neither caused by composition nor occurred during
>     // composition, set mIncludingChangesDuringComposition to false because
>     // IME doesn't want need outdated text changes as text change during
>     // current composition.  So, we should treat the outdated text changes
>     // occurred not during composition.
>+    if (oldData.mIncludingChangesDuringComposition) {
>+      mIncludingChangesWithoutComposition = true;
>+    }
I don't understand this 'if'. Why don't we set mIncludingChangesWithoutComposition always true here?
Please clarify and add some comment or fix. I'd like to see an update patch for this.

Comment 10

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

Attachment: part.1 Remove unused methods of IMENotification (r=smaug)

Comment 11

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

Attachment: part.2 Rename TextChangeDataBase::mCausedByComposition to mCausedOnlyByComposition (r=smaug)

Comment 12

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

Attachment: part.3 Rename TextChangeDataBase::mOccurredDuringComposition to mIncludingChangesDuringComposition (r=smaug)

Comment 13

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

Attachment: part.4 Add TextChangeDataBase::mIncludingChangesWithoutComposition

Indeed, you're right. In that case, newData.mIncludingChangesWithoutComposition should be always true. Therefore, mIncludingChangesWithoutComposition of the instance should be set to true above.

I just added MOZ_ASSERT()s into the if block.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8712524 [details] [diff] [review]
part.4 Add TextChangeDataBase::mIncludingChangesWithoutComposition

"...did occur where there wasn't composition ongoing..."
s/where/when/

(I think it should be also 'a composition', not just 'composition')

Comment 15

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/b21fb00a6138ea698c6fd54053b2aa73db012d1e
Bug 1242331 part.1 Remove unused methods of IMENotification r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/66b71325dfbf5cf32f2eb20a724d0dfcd805ccd0
Bug 1242331 part.2 Rename TextChangeDataBase::mCausedByComposition to mCausedOnlyByComposition r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/3442a539cc8b8f2a7ab504f1e3fad11541579652
Bug 1242331 part.3 Rename TextChangeDataBase::mOccurredDuringComposition to mIncludingChangesDuringComposition r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/04d8a95c6a5e19ca4debf62a052e8011cf6af478
Bug 1242331 part.4 Add TextChangeDataBase::mIncludingChangesWithoutComposition r=smaug

https://hg.mozilla.org/integration/mozilla-inbound/rev/2432259685a715b5ed9376bd2709a44246f90c8c
Bug 1242331 part.5 TSFTextStore should ignore text changes not caused by composition but not occurred during current composition r=m_kato

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b21fb00a6138
https://hg.mozilla.org/mozilla-central/rev/66b71325dfbf
https://hg.mozilla.org/mozilla-central/rev/3442a539cc8b
https://hg.mozilla.org/mozilla-central/rev/04d8a95c6a5e
https://hg.mozilla.org/mozilla-central/rev/2432259685a7

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b21fb00a6138
https://hg.mozilla.org/mozilla-central/rev/66b71325dfbf
https://hg.mozilla.org/mozilla-central/rev/3442a539cc8b
https://hg.mozilla.org/mozilla-central/rev/04d8a95c6a5e
https://hg.mozilla.org/mozilla-central/rev/2432259685a7

Comment 18

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

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

Comment 19

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

Attachment: part.4 Add TextChangeDataBase::mIncludingChangesWithoutComposition (r=smaug)

Comment 20

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

Comment on attachment 8712520 [details] [diff] [review]
part.1 Remove unused methods of IMENotification (r=smaug)

Approval Request Comment
[Feature/regressing bug #]: regression of e10s
[User impact if declined]: when user composing with IME, the composition string may be committed suddenly. (of course, unexpectedly.)
[Describe test coverage new/current, TreeHerder]: landed m-c, and tested manually with try build of aurora.
[Risks and why]: The cause is, typed text is handled by content asynchronously. Therefore, when user composes text with IME, previous text change before current composing may be notified from content process. Then, our TSF module commits composition forcibly for preventing TSF or TIP (IME for TSF) to be confused. This patch ignores text changes before starting current composition. Therefore, this prevents delayed notification from content process blocks user's composition.  However, there is a demerit.  The new behavior may cause showing candidate window or suggest window of TIP at wrong position because content process may still not handle whole input events.  However, that's smaller problem than this bug.  So, I strongly recommend land this patch.
[String/UUID change made/needed]: Nothing.

Comment 21

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

Comment on attachment 8712522 [details] [diff] [review]
part.2 Rename TextChangeDataBase::mCausedByComposition to mCausedOnlyByComposition (r=smaug)

Approval Request Comment: See comment 20.

Comment 22

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

Comment on attachment 8712523 [details] [diff] [review]
part.3 Rename TextChangeDataBase::mOccurredDuringComposition to mIncludingChangesDuringComposition (r=smaug)

Approval Request Comment: See comment 20.

Comment 23

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

Comment on attachment 8718861 [details] [diff] [review]
part.4 Add TextChangeDataBase::mIncludingChangesWithoutComposition (r=smaug)

Approval Request Comment: See comment 20.

Comment 24

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

Comment on attachment 8712129 [details] [diff] [review]
part.5 TSFTextStore should ignore text changes not caused by composition but not occurred during current composition

Approval Request Comment: See comment 20.

Comment 25

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8712520 [details] [diff] [review]
part.1 Remove unused methods of IMENotification (r=smaug)

Has had manual testing, should improve text editing for some users with e10s.
OK to uplift these patches to aurora.

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/34f8a6632e87
https://hg.mozilla.org/releases/mozilla-aurora/rev/f5920d193c30
https://hg.mozilla.org/releases/mozilla-aurora/rev/f7e1f93e24fa
https://hg.mozilla.org/releases/mozilla-aurora/rev/c2498ddc8681
https://hg.mozilla.org/releases/mozilla-aurora/rev/8c13e772a426

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/34f8a6632e87
https://hg.mozilla.org/releases/mozilla-aurora/rev/f5920d193c30
https://hg.mozilla.org/releases/mozilla-aurora/rev/f7e1f93e24fa
https://hg.mozilla.org/releases/mozilla-aurora/rev/c2498ddc8681
https://hg.mozilla.org/releases/mozilla-aurora/rev/8c13e772a426