Closed Bug 1278084 Opened 3 years ago Closed 3 years ago

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

Categories

(Firefox :: General, defect, critical)

47 Branch
x86
Windows 8
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 50
Tracking Status
e10s - ---
firefox47 --- affected
firefox48 --- affected
firefox49 --- affected
firefox-esr45 --- affected
firefox50 --- fixed

People

(Reporter: hsteen, Assigned: masayuki)

Details

(Keywords: crash, inputmethod)

Crash Data

Attachments

(5 files)

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.
Masayuki-san, is this one for you?
Flags: needinfo?(masayuki)
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).
Flags: needinfo?(masayuki)
As far as I've tested, this depends on TIP. I cannot reproduce this bug with ATOK 2016.
Keywords: inputmethod
This isn't reproducible in e10s mode, without Microsoft IME for Japanes, and a kungFuDeathGrip in TSFTextStore::Destroy() doesn't help MS-IME.
Summary: Crash in CImeKeyEventHandler_JPN_Desktop_V1::ProcessKey → [non-e10s][TSF] Crash in CImeKeyEventHandler_JPN_Desktop_V1::ProcessKey
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
tracking-e10s: --- → ?
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/
Attachment #8760877 - Flags: review?(m_kato)
Attachment #8760878 - Flags: review?(m_kato)
Attachment #8760879 - Flags: review?(m_kato)
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/
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 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
Attachment #8760877 - Flags: review?(m_kato) → review+
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 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
Attachment #8760879 - Flags: review?(m_kato) → review+
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 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 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
Attachment #8760878 - Flags: review?(m_kato) → review+
Pushed by masayuki@d-toybox.com:
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
I posted a feedback to Microsoft:
feedback-hub:?contextid=330&feedbackid=d28da551-baa3-4d4d-9f20-09e9c0277cb6&form=1&src=1
https://hg.mozilla.org/mozilla-central/rev/376e385c49d8
https://hg.mozilla.org/mozilla-central/rev/33fa68d6823f
https://hg.mozilla.org/mozilla-central/rev/598a325005ad
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
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
You need to log in before you can comment on or make changes to this bug.