microsoft / microsoft/terminal

We play weird ownership games with SignalTextChanged, and we don't know why

Open
#13,896 1 comment 1 reaction 0 assignees View on GitHub

Nobody has claimed this yet.

Area-Accessibility Area-CodeHealth Area-TerminalControl Needs-Tag-Fix Priority-3 Product-Terminal
Dominant language
C++
Stars
105k
Forks
9.6k
Avg merge
3d 17h
Merged PRs (30d)
29

Description

See context: #13876, [MSFT:39994969](https://task.ms/39994969)

See discussion:
> I'm not really in favor of this PR. There shouldn't be situation to begin with were NVDA calls us and the existence of `TermControl` is the make/break of whether WT crashes... As such I'd prefer fixing whatever UIA APIs we have that might crash WT, including any assumptions we have that WinUI won't suddenly call `AppHost::~AppHost` despite being 50 calls deep in the stack.

> > Should we file a workitem to fix this more soundly? Should we take this as a "stopgap" until we can fix it more soundly?
>
> Maybe I was too terse. 😄 I'm in favor of merging this, just not in favor of this PR because it's a stopgap. I think we should just continue to work on cleaning up our pointer safety and data sharing behavior. Maybe even do it consciously at some point instead of doing it on a case by case basis. IMO we should remove any use of `this` (including implicit use) if it can be avoided and find and if possible remove implicit recursion like this one were class X calls Y which calls back into X (e.g. introduce class Z which is used by both X and Y, removing the need for recursion).

> > Maybe even do it consciously at some point instead of doing it on a case by case basis.
>
> YES

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with issue #13876 and the linked MSFT work item, then trace SignalTextChanged, TermControl, and the UIA calls discussed here. The work is done when the ownership, pointer-safety, and data-sharing behavior no longer depend on the described stopgap or implicit recursive calls.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
cli, operating-systems
Issue type
Refactor
Difficulty
5/5
Estimated time
Over a week
Activity status
Stale
Clarity
Needs clarification
Newbie friendliness
20/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.