The dreaded crash in onNativeTouch is our longest-standing and most prevalent crash on Android, and surely the most annoying too because reinstalling the app doesn’t even help, even if it was working just a moment ago.
Here’s the thing: when the user presses the screen, we need to know when that happened so we can measure things like acceleration when dragging. Android gives us that number in milliseconds since the device was switched on.
Now the problem: due to reasons out of our control, SDL only accepts values up to 2^32 (about 4.3 billion) for the timestamp. But since it’s measured since the device started and not since the app started, it’s totally expected that that number can be reached: indeed, 49.7 days after switching on the device, the number of milliseconds will be higher than the number a UInt32 can accept without overflow. So – with the code how it is at present – every time a touch event is registered from then on, the app will crash. And no amount of restarting or reinstalling the app will fix it. Only restarting the device!
This has probably become more and more relevant over the years as devices become more stable and are left on for longer periods. I’m actually amazed that the crash doesn’t happen more often than it does!
This PR fixes that issue in two ways:
By initing the UInt32 with truncatingIfNeeded – this will ensure the conversion will never crash. Instead, it will effectively wrap around from 2^32 back to 0. That has the potential for some huge negative time deltas, which might lead to glitches, so also:
By basing the timestamp on the init time of SDLActivity, rather than on when the system booted. This will ensure there will be no weirdness if you are running the app after 49.7 days of system uptime. Conversely, it is much less likely that any given app session will be up for 49.7 days.
I also added a defensive check to ensure that UIScreen.main exists before posting any events.
And I fixed some formatting weirdness left over from a previous PR – I can recommend hiding whitespace changes while reviewing this PR.
Please check if the PR fulfills these requirements
[ ] Self-review: I am confident this is the simplest and clearest way to achieve the expected behaviour
[ ] There are no dependencies on other PRs or I have linked dependencies through Zenhub
[ ] The commit messages are clean and understandable
[ ] Tests for the changes have been added (for bug fixes / features)
Fixes crash in
onNativeTouchType of change: Bug fix
Motivation (current vs expected behavior)
The dreaded crash in
onNativeTouchis our longest-standing and most prevalent crash on Android, and surely the most annoying too because reinstalling the app doesn’t even help, even if it was working just a moment ago.Here’s the thing: when the user presses the screen, we need to know when that happened so we can measure things like acceleration when dragging. Android gives us that number in milliseconds since the device was switched on.
Now the problem: due to reasons out of our control, SDL only accepts values up to 2^32 (about 4.3 billion) for the timestamp. But since it’s measured since the device started and not since the app started, it’s totally expected that that number can be reached: indeed, 49.7 days after switching on the device, the number of milliseconds will be higher than the number a
UInt32can accept without overflow. So – with the code how it is at present – every time a touch event is registered from then on, the app will crash. And no amount of restarting or reinstalling the app will fix it. Only restarting the device!This has probably become more and more relevant over the years as devices become more stable and are left on for longer periods. I’m actually amazed that the crash doesn’t happen more often than it does!
This PR fixes that issue in two ways:
truncatingIfNeeded– this will ensure the conversion will never crash. Instead, it will effectively wrap around from 2^32 back to 0. That has the potential for some huge negative time deltas, which might lead to glitches, so also:SDLActivity, rather than on when the system booted. This will ensure there will be no weirdness if you are running the app after 49.7 days of system uptime. Conversely, it is much less likely that any given app session will be up for 49.7 days.I also added a defensive check to ensure that
UIScreen.mainexists before posting any events. And I fixed some formatting weirdness left over from a previous PR – I can recommend hiding whitespace changes while reviewing this PR.Please check if the PR fulfills these requirements