1892600 - Crash in [@ CEditSessionBase::CEditSessionBase]

Status: ASSIGNED

(Core :: DOM: UI Events & Focus Handling, 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)(away 4/27 - 5/6)]

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)(away 4/27 - 5/6)]

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)(away 4/27 - 5/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

Comment 7

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

(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)(away 4/27 - 5/6)]

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)(away 4/27 - 5/6)]

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