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

Fix: Throw exception if RangeSelector min and max args are equal #576

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Lamparter
Copy link

@Lamparter Lamparter commented Dec 1, 2024

Fixes #366

PR Type: Bugfix


This PR fixes an issue where DWM crashes if the arguments for both min and max of the range selector control are equal.

What is the current behavior?

When the Maximum and Minimum arguments of RangeSelector are equal, DWM crashes. This can be reproduced by even the Toolkit gallery app.

What is the new behavior?

If the max and min args of the range selector are equal, an exception will be thrown.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Created a feature/dev branch in your fork (vs. submitting directly from a commit on main)
  • Based off latest main branch of toolkit
  • Tested code with current supported SDKs
  • Contains NO breaking changes

@sylveon
Copy link

sylveon commented Dec 1, 2024

Should probably change the gallery app to prevent this as well (with the fix, gallery app would just crash)

@Arlodotexe
Copy link
Member

Arlodotexe commented Dec 1, 2024

Should probably change the gallery app to prevent this as well (with the fix, gallery app would just crash)

The same would happen in any app. The issue is in the RangeSelector control, it's crashing because it threw an unhandled exception. A fix in the component should be enough.

@Lamparter
Copy link
Author

Lamparter commented Dec 1, 2024

But don't you need to handle the exception in the WCT gallery app?

That's what I meant in my message in the dev-chat channel in uwpc

@Arlodotexe
Copy link
Member

@michael-hawker There seems to be some undefined behavior here. Should the control simply ignore when Min == Max, should it prevent that tick, or something else?

@michael-hawker
Copy link
Member

We should pattern off Slider's behavior here for what we should do. In the base case there, setting min/max to the same value is allowed, and the value is just locked to those values as well:

image

So, I think the best expected behavior is the same here; we should honor that same underlying contract.

Therefore, we should understand what's not working in this scenario to enable that vs. just throwing a different exception.

This would also be worth doing anyway, as we should root-cause the underlying issue; as we should file a platform bug around the DWM crash, as that shouldn't happen regardless. (I wonder if this repros on WinUI 3 as well...)

@Lamparter is this still something you could/would be willing to help us with?

@Lamparter
Copy link
Author

Sure!

@michael-hawker
Copy link
Member

@Lamparter thanks! Let us know if you need any assistance and how we can help support you.

Once we understand the root cause of what's causing the underlying crash, we can open/raise a platform issue on the WinUI repo too with a minimal repro without the toolkit maybe.

Then, we can hopefully just find a good workaround in our toolkit code here to prevent that scenario from occurring when the edge case bounds are setup, and add a test or two to help us ensure we don't regress in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Setting RangeSelector to 100-100 with max value to 1 crashes DWM.
4 participants