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.

Sign up for GitHub

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

Jump to bottom

Update wil. Fixes GDI handle leak #6229

Merged
4 commits merged into from
Jun 1, 2020
Merged

Update wil. Fixes GDI handle leak #6229

4 commits merged into from
Jun 1, 2020

Conversation

miniksa
Copy link
Member

@miniksa miniksa commented May 27, 2020

Summary of the Pull Request

When resizing the window title, a GDI object would be leaked. This has to do with our island message handler using wil to track these objects and wil having a bug.

References

microsoft/wil#100

PR Checklist

Validation Steps Performed

  • Added the GDI Objects column to Task Manager, set the Terminal to use the titleWidth size tabs, then changed the title a bunch with PowerShell. Confirmed repro before (increasing GDI count). Confirmed it's gone after (no change to object count).

@ghost ghost added Area-UserInterface Issues pertaining to the user interface of the Console or Terminal Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news. labels May 27, 2020
@miniksa miniksa self-assigned this May 27, 2020
DHowett
DHowett approved these changes May 27, 2020
View reviewed changes
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.

i didn't do this because of the SA miss.

@WSLUser
Copy link
Contributor

WSLUser commented May 28, 2020

src\cascadia\PublicTerminalCore\HwndTerminal.cpp(721,0): Error C26447: The function is declared 'noexcept' but calls function 'Log_GetLastErrorIf()' which may throw exceptions (f.6).

Missed a spot it looks like. This actually looks easier than updating gsl. It has way more build errors.

@DHowett
Copy link
Member

DHowett commented May 28, 2020

@WSLUser yeah, GSL is really annoying because they changed the definition of span to align with C++20 span. This breaks all existing consumers everywhere :)

@WSLUser
Copy link
Contributor

WSLUser commented May 28, 2020

Well we have fmt, can we use it in place of normal span?

@DHowett
Copy link
Member

DHowett commented May 28, 2020

fmt is a string formatting library, not a general-purpose safe memory region access library. 😄

@WSLUser
Copy link
Contributor

WSLUser commented May 29, 2020

Yeah I didn't think it could but was hopeful

@miniksa miniksa added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 1, 2020
@ghost
Copy link

ghost commented Jun 1, 2020

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.

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 48b3262 into master Jun 1, 2020
@ghost ghost deleted the dev/miniksa/wil branch June 1, 2020 22:29
@ghost
Copy link

ghost commented Jun 18, 2020 •
edited by miniksa
Loading

🎉Windows Terminal Preview v1.1.1671.0 has been released which incorporates this pull request.:tada:

Handy links:

@ghost ghost mentioned this pull request Jun 18, 2020
Closed
DHowett pushed a commit that referenced this pull request Jun 24, 2020
@miniksa @DHowett
Update wil. Fixes GDI handle leak (#6229)
c6fc68e 
## Summary of the Pull Request
When resizing the window title, a GDI object would be leaked. This has to do with our island message handler using `wil` to track these objects and `wil` having a bug.

## References
microsoft/wil#100

## PR Checklist
* [x] Closes #5949
* [x] I work here.
* [x] Tested manually
* [x] Doc not required.
* [x] Am core contributor.

## Validation Steps Performed
* [x] Added the GDI Objects column to Task Manager, set the Terminal to use the `titleWidth` size tabs, then changed the title a bunch with PowerShell. Confirmed repro before (increasing GDI count). Confirmed it's gone after (no change to object count).

(cherry picked from commit 48b3262)
@ghost
Copy link

ghost commented Jun 30, 2020

🎉Windows Terminal v1.0.1811.0 has been released which incorporates this pull request.:tada:

Handy links:

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@carlos-zamora carlos-zamora carlos-zamora approved these changes

@DHowett DHowett DHowett approved these changes

@leonMSFT leonMSFT Awaiting requested review from leonMSFT

Assignees

@miniksa miniksa

Labels
Area-UserInterface Issues pertaining to the user interface of the Console or Terminal 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. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Severity-Crash Crashes are real bad news.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tabs leak GDI handles on tab title resizes
5 participants
@miniksa @WSLUser @DHowett @carlos-zamora @zadjii-msft