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

Fixes rust-windowing/winit#2597 for good #2747

Conversation

Maximetinu
Copy link
Contributor

Remove assert that is being randomly hit by a very small number of users

This is a follow up to PR #2597. As explained in it (pls check it for context) there were a couple of asserts in this file that were unnecessary, and were causing some users to crash; while removing the assert works perfectly fine, so they don't seem to be preventing from anything serious.

In that PR, I was wary and changed the smallest amount of code possible (just removed 1 assert, out of 2). But now I've found some other users (still very few, maybe 0.001%) hitting the remaining assert. Now that time has verified that removing the first one didn't hurt and was the right call, I think that removing the remaining one is the way to go, for the same reasons explained in the previous PR, to prevent these crashes from happening anymore.

Remove assert that is being randomly hit by a very small number of users
@Maximetinu
Copy link
Contributor Author

Hello, can someone give a quick review to this one? We need it to be able to use official winit instead of our fork!

cc @madsmtm because you merged the previous one 🙏

@kchibisov
Copy link
Member

r @daxpedda

@daxpedda
Copy link
Member

I think removing the assert here is actually wrong, I can only come up with two reasons this fails:

  1. It's a race condition, like @madsmtm suspected, in which case we actually have to recreate the handler instead of just moving on like nothing happened.
  2. Our .0001 offset to deal with FP precision isn't enough.

So I would suggest the following:

  • Instead of removing the assert!, we should include the past and current scale factor, so we can make sure that the issue is not the offset (problem 2.).
  • If reproduction is an issue, we can merge it and open an issue tracking this awaiting further feedback.
  • @kchibisov I'm leaving a proper fix for this for another time because of the merge freeze, let me know if you want me to address this nevertheless.

@kchibisov
Copy link
Member

If you know how to fix you can post a fix, it's just will be merged with delay. Though, if it's short and doesn't conflict with KB v2, we could merge.

@daxpedda
Copy link
Member

@Maximetinu could you do what I suggested above and figure out if this isn't an issue with the offset but just a racing condition? Unfortunately I don't seem to be able to reproduce this on my end.

@kchibisov the fix shouldn't conflict with KB v2, I will make one as soon as we know more.

@daxpedda daxpedda added DS - web C - waiting on author Waiting for a response or another PR A - needs repro waiting for a way to reproduce labels May 28, 2023
@daxpedda daxpedda self-assigned this Jun 2, 2023
@daxpedda
Copy link
Member

daxpedda commented Jun 4, 2023

Replaced by #2850.

@daxpedda daxpedda closed this Jun 4, 2023
@Maximetinu
Copy link
Contributor Author

Maximetinu commented Jun 5, 2023

Sorry @daxpedda I never answered 🙏 I was never able to reproduce neither, just got crash reports from users hitting this randomly when resizing. I hope #2850 fixes it, thx!

@Maximetinu
Copy link
Contributor Author

Hi @daxpedda , sorry to revive such an old thread, but, months later, I got a way to consistently reproduce this crash: #3579

Or, at least, it's a crash in what seems to be the same assert.

When this happened to me months ago, it was caused by resizing the screen (but not always).

As I say in this new #3579, the same assert can also be hit by opening the print dialog

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A - needs repro waiting for a way to reproduce C - waiting on author Waiting for a response or another PR DS - web
Development

Successfully merging this pull request may close these issues.

3 participants