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

Opening a hyperlink can cause the Terminal to hang #11100

New Issue
Closed
opened 2026-01-31 02:38:34 +00:00 by claunia · 6 comments
claunia commented 2026-01-31 02:38:34 +00:00
Owner
Copy Link

Originally created by @zadjii-msft on GitHub (Oct 21, 2020).

Originally assigned to: @miniksa on GitHub.

I originally posted this in #7691, but it turns out the hang wasn't even introduced there - it was already in main before that.

Blocking based off the hang discussion we're currently having on teams. Turns out the main thread is grabbing the write lock twice:

Once in

[0x4f] TerminalControl!winrt::Microsoft::Terminal::TerminalControl::implementation::TermControl::_PointerPressedHandler + 0x593

then again in

[0x6]   TerminalControl!winrt::Microsoft::Terminal::TerminalControl::implementation::TermControl::_TrySendKeyEvent + 0x116

because WinUI somehow routes a KeyDownEvent to TermControl during the PointerPressed???

Suggested fix:

if (point.Properties().IsLeftButtonPressed())
{
    const auto cursorPosition = point.Position();
    const auto terminalPosition = _GetTerminalPosition(cursorPosition);
    {
        // handle ALT key
        auto lock = _terminal->LockForWriting();
        _terminal->SetBlockSelection(altEnabled);
        // Release the write lock
    }
    // Re-grab the read lock
    auto lock = _terminal->LockForReading();
    ...
}
Originally created by @zadjii-msft on GitHub (Oct 21, 2020). Originally assigned to: @miniksa on GitHub. I originally posted this in #7691, but it turns out the hang wasn't even introduced there - it was already in `main` before that. <hr> Blocking based off the hang discussion we're currently having on teams. Turns out the main thread is grabbing the write lock twice: Once in ``` [0x4f] TerminalControl!winrt::Microsoft::Terminal::TerminalControl::implementation::TermControl::_PointerPressedHandler + 0x593 ``` then again in ``` [0x6] TerminalControl!winrt::Microsoft::Terminal::TerminalControl::implementation::TermControl::_TrySendKeyEvent + 0x116 ``` because WinUI somehow routes a `KeyDownEvent` to TermControl _during the `PointerPressed`???_ <hr> Suggested fix: ```c++ if (point.Properties().IsLeftButtonPressed()) { const auto cursorPosition = point.Position(); const auto terminalPosition = _GetTerminalPosition(cursorPosition); { // handle ALT key auto lock = _terminal->LockForWriting(); _terminal->SetBlockSelection(altEnabled); // Release the write lock } // Re-grab the read lock auto lock = _terminal->LockForReading(); ... } ```
claunia commented 2026-01-31 02:38:36 +00:00
Author
Owner
Copy Link

@DHowett commented on GitHub (Oct 28, 2020):

So here's a thought.

We're holding the write lock while calling _HyperlinkHandler.

We should never call a user callback under our lock. That could give the user the ability to hold us up indefinitely.

We should get the hyperlink out of the buffer ASAP, release the lock, and then call the handler.

Problem is, this is tangled up with the selection code.

@DHowett commented on GitHub (Oct 28, 2020): So here's a thought. We're holding the write lock while calling `_HyperlinkHandler`. We should _never_ call a user callback under our lock. That could give the user the ability to hold us up indefinitely. We should get the hyperlink out of the buffer ASAP, release the lock, and then call the handler. Problem is, this is tangled up with the selection code.
claunia commented 2026-01-31 02:38:36 +00:00
Author
Owner
Copy Link

@DHowett commented on GitHub (Oct 28, 2020):

Oh you clearly figured that out ;P

@DHowett commented on GitHub (Oct 28, 2020): Oh you clearly figured that out ;P
claunia commented 2026-01-31 02:38:36 +00:00
Author
Owner
Copy Link

@miniksa commented on GitHub (Oct 28, 2020):

I just had this here on my machine and the issue is that ShellExecute is pumping the message queue during its operation which is then servicing the TermControl::_CursorTimerTick as it runs around the queue looking for whatever it's looking for.

TermControl::_CursorTimerTick attempts to take the lock for writing because it's going to adjust the cursor blink state. But the ShellExecute call it is being serviced off of is a result of TermControl::_HyperlinkHandler which is called from TermControl::_PointerPressedHandler which is already holding a write lock.

So somehow the connection needs to be broken because _HyperlinkHandler first only really needs a read lock to fill its parameter. It doesn't actually need to have anything locked at all after it starts running as it already has all the state it needs.

@DHowett says that really we should probably condense the selection stuff down a layer since a lot of this is duplicated between WPF and XAML.... but that's bigger than this fix.

I think we just need to either:

  1. Edit _HyperlinkHandler to coroutine off somewhere so it can return into _PointerPressedHandler and unlock the write lock allowing the ShellExecute call to potentially pump messages with write locks in them like the _CursorTimerTick one.
  2. Change the top of _PointerPressedHandler to not take a write lock for everything. I bet that was relevant in the past, but now with the hyperlink handler, it first off really only needs a read lock and second could give that up after retrieving the URI data and before calling down further to _HyperlinkHandler, resolving the situation.
@miniksa commented on GitHub (Oct 28, 2020): I just had this here on my machine and the issue is that `ShellExecute` is pumping the message queue during its operation which is then servicing the `TermControl::_CursorTimerTick` as it runs around the queue looking for whatever it's looking for. `TermControl::_CursorTimerTick` attempts to take the lock for writing because it's going to adjust the cursor blink state. But the `ShellExecute` call it is being serviced off of is a result of `TermControl::_HyperlinkHandler` which is called from `TermControl::_PointerPressedHandler` which is already holding a write lock. So somehow the connection needs to be broken because `_HyperlinkHandler` first only really needs a read lock to fill its parameter. It doesn't actually need to have anything locked at all after it starts running as it already has all the state it needs. @DHowett says that really we should probably condense the selection stuff down a layer since a lot of this is duplicated between WPF and XAML.... but that's bigger than this fix. I think we just need to either: 1. Edit `_HyperlinkHandler` to coroutine off somewhere so it can return into `_PointerPressedHandler` and unlock the write lock allowing the `ShellExecute` call to potentially pump messages with write locks in them like the `_CursorTimerTick` one. 2. Change the top of `_PointerPressedHandler` to not take a write lock for everything. I bet that was relevant in the past, but now with the hyperlink handler, it first off really only needs a read lock and second could give that up after retrieving the URI data and before calling down further to `_HyperlinkHandler`, resolving the situation.
claunia commented 2026-01-31 02:38:37 +00:00
Author
Owner
Copy Link

@DHowett commented on GitHub (Oct 28, 2020):

Yep. Do number 1. It's easy.

@DHowett commented on GitHub (Oct 28, 2020): Yep. Do number 1. It's easy.
claunia commented 2026-01-31 02:38:37 +00:00
Author
Owner
Copy Link

@ghost commented on GitHub (Nov 11, 2020):

:tada:This issue was addressed in #8087, which has now been successfully released as Windows Terminal v1.4.3141.0.🎉

Handy links:

@ghost commented on GitHub (Nov 11, 2020): :tada:This issue was addressed in #8087, which has now been successfully released as `Windows Terminal v1.4.3141.0`.:tada: Handy links: * [Release Notes](https://github.com/microsoft/terminal/releases/tag/v1.4.3141.0) * [Store Download](https://www.microsoft.com/store/apps/9n8g5rfz9xk3?cid=storebadge&ocid=badge)
claunia commented 2026-01-31 02:38:37 +00:00
Author
Owner
Copy Link

@ghost commented on GitHub (Nov 11, 2020):

:tada:This issue was addressed in #8087, which has now been successfully released as Windows Terminal Preview v1.5.3142.0.🎉

Handy links:

@ghost commented on GitHub (Nov 11, 2020): :tada:This issue was addressed in #8087, which has now been successfully released as `Windows Terminal Preview v1.5.3142.0`.:tada: Handy links: * [Release Notes](https://github.com/microsoft/terminal/releases/tag/v1.5.3142.0) * [Store Download](https://www.microsoft.com/store/apps/9n8g5rfz9xk3?cid=storebadge&ocid=badge)
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-TerminalControl Issue-Bug Priority-0 Product-Terminal Resolution-Fix-Committed Severity-Blocking Severity-Crash
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#11100
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?