1892600 - Crash in [@ CEditSessionBase::CEditSessionBase] when TableTextService.dll ends composition when we disassociate IME context from the window after committing composition

Status: RESOLVED WORKSFORME

(External Software Affecting Firefox :: Other, defect)

Description

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

Crash report: https://crash-stats.mozilla.org/report/index/00a5baa1-6f99-44c7-8dba-0fda10240326

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0  TableTextService.dll  CEditSessionBase::CEditSessionBase  
1  TableTextService.dll  CTableTextService::_EndComposition  
2  TableTextService.dll  CTableTextService::OnCompositionTerminated  
3  textinputframework.dll  CComposition::_SendOnTerminated  
4  textinputframework.dll  CComposition::_Terminate  
5  textinputframework.dll  CInputContext::_TerminateCompositionWithLock  
6  textinputframework.dll  CInputContext::_PseudoSyncEditSessionQiCallback  
7  textinputframework.dll  CInputContext::_QueueItem  
8  textinputframework.dll  CInputContext::TerminateComposition  
9  msctf.dll  CEditSessionCompositionComplete::CompComplete  

By querying Nightly crashes reported within the last 2 months, here are some insights about the signature:

• First crash report: 2024-03-14
• Process type: Parent
• Is startup crash: No
• Has user comments: No
• Is null crash: Yes - all crashes happened on null or near null memory address

Comment 1

[Gian-Carlo Pascutto [:gcp]]

Taking a wild swing at a reasonable place to put IME issues.

Comment 2

[Edgar Chen [:edgar]]

Looks like the volume is quite low; let's set it to S3 for now. We can increase the severity later if we determine it's a critical issue.

Comment 3

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

This is rare, but this is a parent process crash, which might make it a bit more serious.

But I'm not sure this is actionable.

Comment 4

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

Looks like that it's a bug of TSF or TableTextService.dll. According to the stack trace, it tries to end composition managed by TableTestService.dll when we disassociate IMContext from the window to disable IME on the widget. Associating/disassociating IMContext on focused window is not usual behavior of Win32 apps. Therefore, it's not tested enough even by Microsoft. However, if we know there is a composition, we should've already cancels existing composition before disable IME. So, I guess that TableTextService.dll may have a hidden composition like a dead key handling. Perhaps, we need to call ::ImmNotifyIME(himc, NI_COMPOSITIONSTR, CPS_CANCEL) before disassociating IMContext (although it could cause dispatching composition events or something if they don't just reset their composition state).

Comment 5

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

Or, we do wrong things here.
https://searchfox.org/mozilla-central/rev/38377227b8f96fda8f418db614e6a8aa67d01c31/widget/windows/TSFTextStore.cpp#6589-6607

(Chromium does similar things, so I think that we do enough things.)

Comment 6

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

Ah, we may need to use ITfDocumentMgr::EnumContexts instead of ITfDocumentMgr::GetTop.
https://learn.microsoft.com/en-us/windows/win32/api/msctf/nf-msctf-itfdocumentmgr-enumcontexts

Comment 7

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

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6) from comment #6)

Ah, we may need to use ITfDocumentMgr::EnumContexts instead of ITfDocumentMgr::GetTop.
https://learn.microsoft.com/en-us/windows/win32/api/msctf/nf-msctf-itfdocumentmgr-enumcontexts

Hmm, perhaps, I'm wrong because MSDN says that the number of contexts is 2 at most.
https://learn.microsoft.com/en-us/windows/win32/api/msctf/nf-msctf-itfdocumentmgr-push

The TSF manager and text services only interact with the context at the top of the stack. Normally, only the main document context is on the stack. Occasionally, it is necessary to add a second context to the stack. For example, when a text service must display a modal UI, such as a candidate list. During this time, the text service will add its context to the stack. When the text service UI is no longer required, the text service removes the context from the stack. The main context then returns to the top of the stack. To simplify this process and prevent multiple modal UIs from being displayed, there is a maximum of two contexts allowed on the stack.

Comment 8

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

Err, this may be the cause:
https://searchfox.org/mozilla-central/rev/dbd654fa899a56a6a2e92f325c4608020e80afae/widget/windows/WinIMEHandler.cpp#298-299

if (TSFTextStore::IsComposingOn(aWindow)) {
  TSFTextStore::CommitComposition(false);

So, if there is a hidden composition from our point of view, we could fail to commit the composition managed by TableTextService.dll.

Comment 9

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

Attachment: Bug 1892600 - Make `IMEHandler` try to commit compositions in all contexts in the TSF mode before disassociating IM context r=m_kato!

Currently, we try to commit composition only when we know there is a composition
in TSFTextStore. However, according to the crash report, TIP may have a
hidden composition and may fail to manage ref-count of something when we
disassociate IM context from the window when they have a hidden composition.
Therefore, we could avoid the crash if we try to commit all composition in the
all contexts.

Note that a document manager can have at most 2 contexts, according to MSDN [1].

1. https://learn.microsoft.com/en-us/windows/win32/api/msctf/nf-msctf-itfdocumentmgr-push

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d41c7473fb8b
Make `IMEHandler` try to commit compositions in all contexts in the TSF mode before disassociating IM context r=m_kato

Comment 11

[tszentpeteri]

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

Comment 12

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

The leave-open keyword is there and there is no activity for 6 months.
:masayuki, maybe it's time to close this bug?
For more information, please visit BugBot documentation.

Comment 13

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

Well, unfortunately, there are some remaining crash reports:

• bp-a119b3a1-f224-496b-80f8-af08e0241112
• bp-9419e19f-475b-4f48-bd22-51d350241106
• bp-c2b078b3-f2e5-4945-98c8-c37ca0240919

All are like this:

CEditSessionBase::CEditSessionBase(CTableTextService*, ITfContext*)
CTableTextService::_EndComposition(ITfContext*)
CTableTextService::OnCompositionTerminated(unsigned long, ITfComposition*)
CComposition::_SendOnTerminated(unsigned long, unsigned long)
CComposition::_Terminate(unsigned long)
CInputContext::_TerminateCompositionWithLock(ITfCompositionView*, unsigned long)
CInputContext::_PseudoSyncEditSessionQiCallback
CInputContext::_QueueItem
CInputContext::TerminateComposition
CEditSessionCompositionComplete::CompComplete(unsigned long)
CEditSessionObject::DoEditSession(unsigned long)
CInputContext::_DoEditSession(unsigned long, ITfEditSession*, unsigned long)
CInputContext::_EditSessionQiCallback(CInputContext*, _TS_QUEUE_ITEM*, QiCallbackCode)
CInputContext::OnLockGranted(unsigned long)
CACPWrap::OnLockGranted(unsigned long)
CTextStoreImpl::RequestLock(unsigned long, long*)
CInputContext::_QueueItem(_TS_QUEUE_ITEM*, int, long*)
CInputContext::RequestEditSession(unsigned long, ITfEditSession*, unsigned long, long*)
CEditSessionObject::RequestEditSession(unsigned long, long*)
EscbCompComplete(IMCLock&, unsigned long, ITfContext*, tag_LIBTHREAD*, int)
CicBridge::Notify(TLS*, ITfThreadMgr_P*, HIMC__*, unsigned long, unsigned long, unsigned long)
NotifyIME
ImmNotifyIME
ImeSetActiveContext
ImmSetActiveContext
ImmAssociateContextEx
mozilla::widget::IMEContext::Disassociate()
mozilla::widget::IMEHandler::AssociateIMEContext(nsWindow*, bool)
mozilla::widget::IMEHandler::SetInputContext(nsWindow*, mozilla::widget::InputContext&, mozilla::widget::InputContextAction const&)

So, when we disassociate the IME context from the window, CTableTextService may have a hidden composition and the clean up code hits the bug. I need to write a follow up patch for IMEHandler::AssociateIMEContext.

Comment 14

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

Oh, we're doing it.

According to the number of crash reports for a year, my patch couldn't fix anything. I have no more idea about this.

Comment 15

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

Closing because no crashes reported for 12 weeks.