starred/terminal
1
0
Fork 0
mirror of https://github.com/microsoft/terminal.git synced 2026-02-03 21:25:34 +00:00
Code Issues 19.0k Packages Projects Releases 20 Wiki Activity

Make Pane a proper WinRT type #5622

New Issue
Open
opened 2026-01-31 00:17:39 +00:00 by claunia · 5 comments
claunia commented 2026-01-31 00:17:39 +00:00
Owner
Copy Link

Originally created by @zadjii-msft on GitHub (Dec 17, 2019).

Pretty self explanatory. Right now they're plain c++ types, but in reality they should be exposed as projected WinRT types, so they can be used across binary boundaries.

Originally created by @zadjii-msft on GitHub (Dec 17, 2019). Pretty self explanatory. Right now they're plain c++ types, but in reality they should be exposed as projected WinRT types, so they can be used across binary boundaries.
claunia added the Issue-TaskProduct-TerminalPriority-2Area-CodeHealth labels 2026-01-31 00:17:39 +00:00
claunia commented 2026-01-31 00:17:40 +00:00
Author
Owner
Copy Link

@zadjii-msft commented on GitHub (Sep 6, 2023):

We may be able to fix MSFT:451680901 and MSFT:407375662 with this. Those are both crashes in Pane because pane is setting up event handlers, using this as the callback target, but the event fires after the pane is released (which doesn't actually unregister the callback).

  1. TerminalPage::_updateThemeColors ↩︎

  2. Pane::_ControlGotFocusHandler ↩︎

@zadjii-msft commented on GitHub (Sep 6, 2023): We may be able to fix MSFT:45168090[^1] and MSFT:40737566[^2] with this. Those are both crashes in `Pane` because pane is setting up event handlers, using `this` as the callback target, but the event fires after the pane is released (which doesn't actually unregister the callback). [^1]: `TerminalPage::_updateThemeColors` [^2]: `Pane::_ControlGotFocusHandler`
claunia commented 2026-01-31 00:17:40 +00:00
Author
Owner
Copy Link

@lhecker commented on GitHub (Sep 6, 2023):

Huh, but aren't we already using enable_shared_from_this<Pane>? Of the 17 places in Pane.cpp that use this only 2 use it unsafely without event revoker: _borderFirst/Second.Tapped(). They call _FocusFirstChild and that would match the second bug right? TerminalPage sets up the GotFocus event handlers which calls _UpdateActivePane, which then invokes the ActivePaneChanged event handlers, which TerminalPage is subscribed to, which then calls _updateThemeColors. That would explain the first bug.

In short, I think we can hotfix the two bugs right now if we use event revokers for _borderFirst/Second.Tapped() in Pane.cpp. All the other event handlers already do it too. Does that make sense? Should I do it?

That aside: FWIW strictly using projected types may make implementing certain algorithms more difficult / less effective, like WalkTree which is currently being used in a number of places outside of Pane.cpp.

@lhecker commented on GitHub (Sep 6, 2023): Huh, but aren't we already using `enable_shared_from_this<Pane>`? Of the 17 places in `Pane.cpp` that use `this` only 2 use it unsafely without event revoker: `_borderFirst/Second.Tapped()`. They call `_FocusFirstChild` and that would match the second bug right? `TerminalPage` sets up the `GotFocus` event handlers which calls `_UpdateActivePane`, which then invokes the `ActivePaneChanged` event handlers, which `TerminalPage` is subscribed to, which then calls `_updateThemeColors`. That would explain the first bug. In short, I think we can hotfix the two bugs right now if we use event revokers for `_borderFirst/Second.Tapped()` in `Pane.cpp`. All the other event handlers already do it too. Does that make sense? Should I do it? That aside: FWIW strictly using projected types may make implementing certain algorithms more difficult / less effective, like `WalkTree` which is currently being used in a number of places outside of `Pane.cpp`.
claunia commented 2026-01-31 00:17:40 +00:00
Author
Owner
Copy Link

@zadjii-msft commented on GitHub (Sep 6, 2023):

I think these guys might be part of it too:
6cff135f37/src/cascadia/TerminalApp/Pane.cpp (L46-L48)

Because they're registered to this, they don't get revoked when the pane is Closed.

(I can't recall if I got those in a drive-by for #997 or not)

@zadjii-msft commented on GitHub (Sep 6, 2023): I think these guys might be part of it too: https://github.com/microsoft/terminal/blob/6cff135f376482b3c4fb83089063e6ff0687a651/src/cascadia/TerminalApp/Pane.cpp#L46-L48 Because they're registered to `this`, they don't get revoked when the pane is Closed. (I can't recall if I got those in a drive-by for #997 or not)
claunia commented 2026-01-31 00:17:41 +00:00
Author
Owner
Copy Link

@lhecker commented on GitHub (Sep 6, 2023):

Oh, is there a difference between a pane closing and a pane being deallocated? I hoped it would only be a problem when a pane is deallocated and the this pointer is actually invalid.

@lhecker commented on GitHub (Sep 6, 2023): Oh, is there a difference between a pane closing and a pane being deallocated? I hoped it would only be a problem when a pane is deallocated and the `this` pointer is actually invalid.
claunia commented 2026-01-31 00:17:41 +00:00
Author
Owner
Copy Link

@zadjii-msft commented on GitHub (Sep 6, 2023):

Wait now I honestly don't know. It seems like releasing the last ref to the shared_ptr would dtor the Pane, and that would automatically clean up the _gotFocusRevoker & _lostFocusRevoker, so that they couldn't be called back at this point. Maybe there's a time slice where the smart pointer is invalid, but before the pane is actually deallocated. Maybe, if at exactly that slice, the shared_from_this call explodes

@zadjii-msft commented on GitHub (Sep 6, 2023): Wait now I honestly don't know. It _seems_ like releasing the last ref to the `shared_ptr` would `dtor` the Pane, and that would automatically clean up the `_gotFocusRevoker` & `_lostFocusRevoker`, so that they couldn't be called back at this point. Maybe there's a time slice where the smart pointer is invalid, but before the pane is actually deallocated. Maybe, if at _exactly_ that slice, the `shared_from_this` call explodes
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
⛺ Reserved
A11yCO
A11yMAS
A11ySev1
A11ySev2
A11ySev3
A11yTTValidated
A11yUsable
A11yVoiceAccess
A11yWCAG
Area-Accessibility
Area-AtlasEngine
Area-AzureShell
Area-Build
Area-Build
Area-Chat
Area-CmdPal
Area-CodeHealth
Area-Commandline
Area-CookedRead
Area-DefApp
Area-Extensibility
Area-Fonts
Area-GroupPolicy
Area-i18n
Area-Input
Area-Interaction
Area-Interop
Area-Localization
Area-Output
Area-Performance
Area-Portable
Area-Quality
Area-Remoting
Area-Rendering
Area-Schema
Area-Server
Area-Settings
Area-SettingsUI
Area-ShellExtension
Area-ShellExtension
Area-ShellExtension
Area-Suggestions
Area-Suggestions
Area-TerminalConnection
Area-TerminalControl
Area-Theming
Area-UserInterface
Area-VT
Area-Windowing
Area-WPFControl
AutoMerge
Blocking-Ingestion
Culprit-Centennial
Culprit-WinUI
Disability-All
Disability-Blind
Disability-LowVision
Disability-Mobility
External-Blocked-WinUI3
Fixed
Gathering-Data
good first issue
HCL-E+D
HCL-WindowsTerminal
Help Wanted
Impact-Compatibility
Impact-Compliance
Impact-Correctness
Impact-Visual
In-PR
InclusionBacklog
InclusionBacklog-Windows TerminalWin32
InclusionCommitted-202206
Issue-Bug
Issue-Docs
Issue-Feature
Issue-Feature
Issue-Question
Issue-Samples
Issue-Scenario
Issue-Task
Needs-Attention
Needs-Author-Feedback
Needs-Bisect
Needs-Discussion
Needs-Repro
Needs-Tag-Fix
Needs-Tag-Fix
Needs-Triage
No-Recent-Activity
Priority-0
Priority-1
Priority-2
Priority-3
Product-Cmd.exe
Product-Colortool
Product-Colortool
Product-Colortool
Product-Conhost
Product-Conpty
Product-Meta
Product-Powershell
Product-Terminal
Product-WSL
pull-request
Mirrored from GitHub Pull Request
 Resolution-Answered
Resolution-By-Design
Resolution-Duplicate
Resolution-External
Resolution-Fix-Available
Resolution-Fix-Committed
Resolution-No-Repro
Resolution-Won't-Fix
Severity-Blocking
Severity-Crash
Severity-DataLoss
spam
this-will-be-a-breaking-change
Tracking-External
WindowsTerminal_Win32
Work-Item
zAskModeBug
zInbox-Bug
No Label Area-CodeHealth Issue-Task Priority-2 Product-Terminal
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
claunia
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: starred/terminal#5622
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?