1278084 - [non-e10s][TSF] Crash in CImeKeyEventHandler_JPN_Desktop_V1::ProcessKey

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Hallvord R. M. Steen [:hallvors]]

Attachment: Wannabe test case for bug 1267526 that found a crasher bug instead

This bug was filed from the Socorro interface and is 
report bp-f89e6cf5-b36c-46e0-a587-f25852160604.
=============================================================

Using MS IME for Japanese and typing into a TEXTAREA that has an input handler doing blur() and focus() crashes Firefox.

Test case attached.

Comment 1

[Hallvord R. M. Steen [:hallvors]]

Masayuki-san, is this one for you?

Comment 2

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

Could be. I guess that releasing active TextStore during a keydown (or a keyup) causes TSF/TIP accessing released object (I mean the root cause is a bug of TSF/TIP).

Comment 3

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

As far as I've tested, this depends on TIP. I cannot reproduce this bug with ATOK 2016.

Comment 4

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

Attachment: log at the crash (starting from dispatching last compositionupdate into the widget listener)

Comment 5

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

This isn't reproducible in e10s mode, without Microsoft IME for Japanes, and a kungFuDeathGrip in TSFTextStore::Destroy() doesn't help MS-IME.

Comment 6

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

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

Comment 7

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

Attachment: Bug 1278084 part.1 Don't release TSF objects during handling a key message

While TIP is handling a key message, TSFTextStore shouldn't release any TSF objects since it may cause hitting a bug of TIPs.  Actually, MS-IME for Japanese on Windows 10 crashes when TSFTextStore is destroyed during composition because probably it accesses some destroyed objects to request to commit composition or query contents.

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

Comment 8

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

Attachment: Bug 1278084 part.2 TSFTextStore should allow TSF to lock the document even during destroying

While a TSFTextStore instance is being destroyed, TSFTextStore::Destroy() tries to commit remaining composition and notify TSF of destroying the view.  At this moment, TSF/TIP may try to commit the composition or retrieve the contents with calling ITextStoreACP::RequestLock() but currently TSFTextStore disallows the requests to lock of them.  This means that TSFTextStore never sends composition commit events asynchronously.  Therefore, TextComposition may keep waiting remaining composition events but this causes odd behavior because they won't be fired.

For avoiding this issue, TSFTextStore should behave as normal even while it's being destroyed.  Fortunately, if there is a composition, it always has mLockedContent and mSelection.  So, it can compute expected results of TSF/TIP with them.

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

Comment 9

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

Attachment: Bug 1278084 part.3 TextComposition shouldn't decide composition string which is only an ideographic space as a placeholder

Currently, when TextComposition tries to forcibly commit composition synchronously, it cancels the composition if there is only an ideographic space since legacy Chinese IMEs for Windows were used an ideographic space as a placeholder and shows actual composition string in its owning window (called reading window).

However, Japanese TIPs basically use composition to input an ideographic space. Unfortunately, this intentional input of an ideographic space is always canceled if an editor commits to composition at every input event in TSF mode because TSF cannot commit during a call of ITextStore::RequestLock().  Additionally, we will enable e10s mode, then, on all platforms, requesting commit composition is handled asynchronously.

Therefore, we should make the hack disabled in default settings now. If we'll find a way to distinguish if an ideographic space is a placeholder, we should recover this hack. Note that such input fields cannot handle such legacy IMEs, so, disabling the hack in default settings must be fine.

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

Comment 10

[Makoto Kato [:m_kato]]

Comment on attachment 8760877 [details]
Bug 1278084 part.1 Don't release TSF objects during handling a key message

https://reviewboard.mozilla.org/r/58280/#review55274

Comment 11

[Makoto Kato [:m_kato]]

https://reviewboard.mozilla.org/r/58282/#review55272

NotifyTSFOfLayoutChangeAgain only calls NotifyTSFOfLayoutChange when during destroyed.  Is this typo?

::: widget/windows/TSFTextStore.cpp:4937
(Diff revision 1)
>  
>  void
>  TSFTextStore::NotifyTSFOfLayoutChangeAgain()
>  {
> +  // Don't notify TSF of layout change after destroyed.
> +  if (!mDestroyed) {

Typo of "if (mDestroyed)" ?

Comment 12

[Makoto Kato [:m_kato]]

Comment on attachment 8760879 [details]
Bug 1278084 part.3 TextComposition shouldn't decide composition string which is only an ideographic space as a placeholder

https://reviewboard.mozilla.org/r/58284/#review55280

Comment 13

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

https://reviewboard.mozilla.org/r/58282/#review55272

> Typo of "if (mDestroyed)" ?

Oh, yes! Thank you!!

Comment 14

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

Comment on attachment 8760878 [details]
Bug 1278084 part.2 TSFTextStore should allow TSF to lock the document even during destroying

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58282/diff/1-2/

Comment 15

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

Comment on attachment 8760879 [details]
Bug 1278084 part.3 TextComposition shouldn't decide composition string which is only an ideographic space as a placeholder

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58284/diff/1-2/

Comment 16

[Makoto Kato [:m_kato]]

Comment on attachment 8760878 [details]
Bug 1278084 part.2 TSFTextStore should allow TSF to lock the document even during destroying

https://reviewboard.mozilla.org/r/58282/#review55556

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/376e385c49d8
part.1 Don't release TSF objects during handling a key message r=m_kato
https://hg.mozilla.org/integration/mozilla-inbound/rev/33fa68d6823f
part.2 TSFTextStore should allow TSF to lock the document even during destroying r=m_kato
https://hg.mozilla.org/integration/mozilla-inbound/rev/598a325005ad
part.3 TextComposition shouldn't decide composition string which is only an ideographic space as a placeholder r=m_kato

Comment 18

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

I posted a feedback to Microsoft:
feedback-hub:?contextid=330&feedbackid=d28da551-baa3-4d4d-9f20-09e9c0277cb6&form=1&src=1

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/376e385c49d8
https://hg.mozilla.org/mozilla-central/rev/33fa68d6823f
https://hg.mozilla.org/mozilla-central/rev/598a325005ad

Comment 20

[BugBot [:suhaib / :marco/ :calixte]]

Crash volume for signature 'CImeKeyEventHandler_JPN_Desktop_V1::ProcessKey':
 - nightly (version 50): 1 crash from 2016-06-06.
 - aurora  (version 49): 25 crashes from 2016-06-07.
 - beta    (version 48): 84 crashes from 2016-06-06.
 - release (version 47): 663 crashes from 2016-05-31.
 - esr     (version 45): 103 crashes from 2016-04-07.

Crash volume on the last weeks:
             Week N-1   Week N-2   Week N-3   Week N-4   Week N-5   Week N-6   Week N-7
 - nightly          0          0          0          0          0          1          0
 - aurora           6          3          4          2          3          3          2
 - beta            14          6         11         14         13         10          3
 - release        137         74         99         95        107         81         31
 - esr             11          5          8         13         32          6          4

Affected platform: Windows