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

Trying to define color 256 causes temporary hang #3720

Closed
egmontkob opened this issue Nov 26, 2019 · 3 comments · Fixed by #3938
Closed

Trying to define color 256 causes temporary hang #3720

egmontkob opened this issue Nov 26, 2019 · 3 comments · Fixed by #3938
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Milestone

Comments

@egmontkob
Copy link

Environment

Windows build number: 10.0.18362.0
Windows Terminal version: 0.7.3291.0

Steps to reproduce

Trying to redefine the exact RGB value for color index 256:

echo -ne '\e]4;256;rgb:ab/cd/ef\e\\'

Expected behavior

Nothing, because the index is out of range.

Actual behavior

The next shell prompt and the command as I type it doesn't appear. It takes a newline or so to get out of this state.

Note: This issue only occurs with exactly 256. With bigger numbers there's no hang.

@ghost ghost added Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting Needs-Tag-Fix Doesn't match tag requirements labels Nov 26, 2019
@zadjii-msft zadjii-msft added Product-Conhost For issues in the Console codebase Help Wanted We encourage anyone to jump in on these. Area-VT Virtual Terminal sequence support Issue-Bug It either shouldn't be doing this or needs an investigation. labels Nov 26, 2019
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Nov 26, 2019
@zadjii-msft zadjii-msft added this to the Terminal v1.0 milestone Nov 26, 2019
@zadjii-msft zadjii-msft added the Priority-1 A description (P1) label Nov 26, 2019
@j4james
Copy link
Collaborator

j4james commented Nov 27, 2019

Note that this is not a conhost issue - it only fails in Windows Terminal. The exact source of the error is in the Terminal::SetColorTableEntry method:

if (tableIndex > _colorTable.size())
{
return false;
}
_colorTable.at(tableIndex) = dwColor;

That test should be >=. It might even be safer to drop the test and rely on the at checking, but just make sure the exception is handled properly. Because even if we fix this particular test, it doesn't seem right that an exception would cause the parser to get into such a weird state.

@zadjii-msft zadjii-msft added Product-Terminal The new Windows Terminal. and removed Product-Conhost For issues in the Console codebase labels Nov 27, 2019
@zadjii-msft
Copy link
Member

Ooh, good catch. Thanks for finding that @j4james!

@DHowett-MSFT DHowett-MSFT removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Nov 30, 2019
@ghost ghost added In-PR This issue has a related PR and removed In-PR This issue has a related PR labels Dec 12, 2019
@ghost ghost added the In-PR This issue has a related PR label Dec 12, 2019
@ghost ghost closed this as completed in #3938 Dec 14, 2019
ghost pushed a commit that referenced this issue Dec 14, 2019
<!-- Enter a brief description/summary of your PR here. What does it fix/what does it change/how was it tested (even manually, if necessary)? -->
## Summary of the Pull Request
Uses the verification in `at` to ensure the index is correct (as @j4james suggests). If `at` throws, then returns false.

<!-- Other than the issue solved, is this relevant to any other issues/existing PRs? --> 
## References

<!-- Please review the items on the PR checklist before submitting-->
## PR Checklist
* [x] Closes #3720
* [x] CLA signed. If not, go over [here](https://cla.opensource.microsoft.com/microsoft/Terminal) and sign the CLA
* [ ] Tests added/passed
* [ ] Requires documentation to be updated
* [x] ~~I've~~ discussed this with core contributors already. If not checked, I'm ready to accept this work might be rejected in favor of a different grand plan. Issue number where discussion took place: #3720

<!-- Provide a more detailed description of the PR, other things fixed or any additional comments/features here -->
## Detailed Description of the Pull Request / Additional comments

<!-- Describe how you validated the behavior. Add automated tests wherever possible, but list manual validation steps taken as well -->
## Validation Steps Performed
Can no longer repro the issue after the fix.
@ghost ghost added Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release. and removed In-PR This issue has a related PR labels Dec 14, 2019
@ghost
Copy link

ghost commented Jan 14, 2020

🎉This issue was addressed in #3938, which has now been successfully released as Windows Terminal Preview v0.8.10091.0.:tada:

Handy links:

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-VT Virtual Terminal sequence support Help Wanted We encourage anyone to jump in on these. Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-1 A description (P1) Product-Terminal The new Windows Terminal. Resolution-Fix-Committed Fix is checked in, but it might be 3-4 weeks until a release.
Projects
None yet
4 participants