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

Delay load call SetThreadDescription to restore WPF renderer on Win7 #10582

Merged
1 commit merged into from
Jul 8, 2021

Conversation

miniksa
Copy link
Member

@miniksa miniksa commented Jul 7, 2021

Delay load call SetThreadDescription to restore WPF renderer on Win7

PR Checklist

  • Closes something @DHowett asked me to do.
  • I work here
  • I F5'd it on a version with this function and it still works

Detailed Description of the Pull Request / Additional comments

I keep forgetting that anything in the WPF control needs to keep working on Win7. Or more specifically... I remember this fact for the DX renderer, but not for the render thread base. Oops. Turns out this particular convenience method to set thread descriptions for visibility inside the debugger (to make my life easier) only works down to 1607 (see https://docs.microsoft.com/windows/win32/api/processthreadsapi/nf-processthreadsapi-setthreaddescription). Since it's just a debugging convenience... skipping it entirely when the procedure is not found should be fine. Also I don't try to load kernel32.dll and just get the handle of the existing module (which per the remarks at https://docs.microsoft.com/windows/win32/api/libloaderapi/nf-libloaderapi-getmodulehandlew will not increment the module reference count) because kernel32.dll pretty much has to be there or we're already in hot water.

@miniksa miniksa added Area-WPFControl Things related to the WPF version of the TermControl Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. labels Jul 7, 2021
@miniksa miniksa requested a review from DHowett July 7, 2021 23:58
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eventually I'd like to restore conhost's ability to run on Blue, but I'm okay with this for a quick fix

@DHowett
Copy link
Member

DHowett commented Jul 8, 2021

You need not wait for me to test it. This will work. :)

@miniksa
Copy link
Member Author

miniksa commented Jul 8, 2021

You need not wait for me to test it. This will work. :)

Kay

@miniksa miniksa added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jul 8, 2021
@ghost
Copy link

ghost commented Jul 8, 2021

Hello @miniksa!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 6 hours. No worries though, I will be back when the time is right! 😉

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@ghost ghost merged commit 1374396 into main Jul 8, 2021
@ghost ghost deleted the dev/miniksa/fix_dx_win7 branch July 8, 2021 07:59
DHowett pushed a commit that referenced this pull request Jul 8, 2021
…10582)

Delay load call SetThreadDescription to restore WPF renderer on Win7

## PR Checklist
* [x] Closes something @DHowett asked me to do.
* [x] I work here
* [x] I F5'd it on a version with this function and it still works

## Detailed Description of the Pull Request / Additional comments

I keep forgetting that anything in the WPF control needs to keep working on Win7. Or more specifically... I remember this fact for the DX renderer, but not for the render thread base. Oops. Turns out this particular convenience method to set thread descriptions for visibility inside the debugger (to make my life easier) only works down to 1607 (see https://docs.microsoft.com/windows/win32/api/processthreadsapi/nf-processthreadsapi-setthreaddescription). Since it's just a debugging convenience... skipping it entirely when the procedure is not found should be fine. Also I don't try to load `kernel32.dll` and just get the handle of the existing module (which per the remarks at https://docs.microsoft.com/windows/win32/api/libloaderapi/nf-libloaderapi-getmodulehandlew will not increment the module reference count) because `kernel32.dll` pretty much has to be there or we're already in hot water.

(cherry picked from commit 1374396)
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-WPFControl Things related to the WPF version of the TermControl AutoMerge Marked for automatic merge by the bot when requirements are met Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants