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

Stop DoSing users with renderer errors #13995

Merged
5 commits merged into from
Sep 16, 2022
Merged

Stop DoSing users with renderer errors #13995

5 commits merged into from
Sep 16, 2022

Conversation

lhecker
Copy link
Member

@lhecker lhecker commented Sep 14, 2022

If a rendering engine constantly throws error we'll effectively
denial-of-service our users by drowning them in warning popups.
This commit fixes the issue by limiting the retries in all cases.

Issue found in: #13985

Validation Steps Performed

  • Add a THROW_HR(E_INVALIDARG); in AtlasEngine::StartPaint()
  • Launch Windows Terminal
  • Only one warning popup shows up ✅
  • Rendering is disabled until one clicks "resume" ✅

@lhecker lhecker added Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues Issue-Bug It either shouldn't be doing this or needs an investigation. Product-Terminal The new Windows Terminal. Priority-2 A description (P2) labels Sep 14, 2022
@DHowett
Copy link
Member

DHowett commented Sep 14, 2022

Good fix, love it

@DHowett
Copy link
Member

DHowett commented Sep 14, 2022

Hmm your renderer change for error reporting and timeouts seem to have caused the unit tests to take 30 minutes to run

I hope that they don't rely on render errors to do .. something. 😆

@lhecker lhecker added the AutoMerge Marked for automatic merge by the bot when requirements are met label Sep 15, 2022
@ghost
Copy link

ghost commented Sep 15, 2022

Hello @lhecker!

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.

@DHowett
Copy link
Member

DHowett commented Sep 15, 2022

I don't understand why this change requires us to propagate a render thread to the tests. Why is that?

@lhecker
Copy link
Member Author

lhecker commented Sep 15, 2022 •
edited
Loading

I don't understand why this change requires us to propagate a render thread to the tests. Why is that?

On rendering failure it calls _pThread->DisablePainting();. Some tests apparently cause exactly such failures, and if TAEF isolation is disabled then all subsequent tests may fail as well.

I'm dumb.

@ghost ghost merged commit 81e2bc9 into main Sep 16, 2022
@ghost ghost deleted the dev/lhecker/renderer-fixup branch September 16, 2022 14:17
DHowett pushed a commit that referenced this pull request Sep 16, 2022
@lhecker @DHowett
Stop DoSing users with renderer errors (#13995)
8f013d7 
If a rendering engine constantly throws error we'll effectively
denial-of-service our users by drowning them in warning popups.
This commit fixes the issue by limiting the retries in all cases.

Issue found in: #13985

## Validation Steps Performed
* Add a `THROW_HR(E_INVALIDARG);` in `AtlasEngine::StartPaint()`
* Launch Windows Terminal
* Only one warning popup shows up ✅
* Rendering is disabled until one clicks "resume" ✅

(cherry picked from commit 81e2bc9)
Service-Card-Id: 85653502
Service-Version: 1.16
DHowett pushed a commit that referenced this pull request Sep 21, 2022
@lhecker @DHowett
Stop DoSing users with renderer errors (#13995)
85f789f 
If a rendering engine constantly throws error we'll effectively
denial-of-service our users by drowning them in warning popups.
This commit fixes the issue by limiting the retries in all cases.

Issue found in: #13985

## Validation Steps Performed
* Add a `THROW_HR(E_INVALIDARG);` in `AtlasEngine::StartPaint()`
* Launch Windows Terminal
* Only one warning popup shows up ✅
* Rendering is disabled until one clicks "resume" ✅

(cherry picked from commit 81e2bc9)
Service-Card-Id: 85653502
Service-Version: 1.16
(cherry picked from commit 8f013d7)
@ghost
Copy link

ghost commented Sep 23, 2022

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

Handy links:

@DHowett DHowett mentioned this pull request Sep 26, 2022
Closed
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@DHowett DHowett DHowett approved these changes

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

Assignees
No one assigned
Labels
Area-Rendering Text rendering, emoji, complex glyph & font-fallback issues 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-2 A description (P2) Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lhecker @DHowett @carlos-zamora