Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port IME reentrancy fix from 4.8 #4985

Merged
merged 1 commit into from
Aug 17, 2021
Merged

Conversation

SamBent
Copy link
Contributor

@SamBent SamBent commented Aug 3, 2021

Addresses Issue #4984. This is a port of a servicing fix in .NET 4.7-4.8.

Description

The Text Services Framework (Cicero) uses a locking protocol for communication between an IME and WPF. When an IME wants to change the text:

• Cicero asks for a write lock (ITextStoreACP.RequestLock)
• WPF (specifically TextStore) grants the lock and calls back into Cicero (ITextStoreACPSink.OnLockGranted)
• Cicero calls other ITextStoreACP methods to make the desired changes. No one else is allowed to change the text during this time, until OnLockGranted returns.
• WPF notifies other parties about the changes, as follows (HandleCompositionEvents):
+ Undo the IME changes
+ Replay the IME changes one-by-one, this time raising public events (except to Cicero itself)
+ If the event listeners changed the text, inform the IME

The bug arises when Cicero starts the next round of changes before the previous one finishes -- more precisely, when Cicero requests a lock while WPF is still replaying IME changes from a previous lock. These normally can't happen as WPF is careful not to call Cicero during the replay, but they can happen if raising a public event causes messages to be pumped and Cicero's message hook sees a WM_KEY* message and tries to act on it. (For example, a 3rd-party TextBox listens to the TextChanged event and calls an unrelated COM component, causing COM to pump messages.)

Granting such a request has several possible outcomes, most of them bad:

  1. The TextStore is in an inconsistent state. GrantLock calls VerifyTextStoreConsistency, which calls FailFast.
  2. The TextStore is consistent, the OnLockGranted callback succeeds, but it changes the text. This change doesn't appear in the outer replay's undo/redo list, so when the replay is complete (SetFinalDocumentState) the call to VerifyTextStoreConsistency calls FailFast. [This is particularly difficult to diagnose because by the time FailFast occurs, all traces of the inner lock request are gone. The crash dump doesn't help.]
  3. The OnLockGranted callback succeeds and doesn't change the text. This has no known bad consequences.

Of course, we can't know in advance if a Write request is going to change the text - we can't distinguish (2) and (3).

To avoid these problems, defer the request. If the request was synchronous we'll simply ignore it; this may lead to other problems (unknown at this time), but it's better than FailFast.

Customer Impact

Crash while using Chinese IME.

Regression

Testing

Ad-hoc around customer scenario.
Standard regression test.

Risk

Low. Port of .NETFx servicing fix released earlier this year.

@SamBent SamBent requested a review from a team as a code owner August 3, 2021 01:13
@ghost ghost added the PR metadata: Label to tag PRs, to facilitate with triage label Aug 3, 2021
@ghost ghost requested review from fabiant3 and ryalanms August 3, 2021 01:13
@SamBent SamBent merged commit 87066bd into dotnet:main Aug 17, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Apr 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR metadata: Label to tag PRs, to facilitate with triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants