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

Crash when modal gets shown #735

Closed
exzos28 opened this issue Dec 17, 2024 · 6 comments · Fixed by #740
Closed

Crash when modal gets shown #735

exzos28 opened this issue Dec 17, 2024 · 6 comments · Fixed by #740
Assignees
Labels
🤖 android Android specific 🎯 crash Library triggers a crash of the app 🌎 modal Anything that involves Modal usage

Comments

@exzos28
Copy link

exzos28 commented Dec 17, 2024

The issue is still valid.
The project has:

  • react-native-keyboard-controller
  • @gorhom/bottom-sheet

When I open a modal window I get a crash
java.lang.RuntimeException: folly::toJson: JSON object value was an INF when serializing value at “progress”

A bit of information from the debugger:
image

  • OS: android 10
  • RN version: 0.76
  • RN architecture: new, fabric
  • JS engine: Hermes
  • Library version: 1.15.0

Additional context
#718
#714

@exzos28
Copy link
Author

exzos28 commented Dec 17, 2024

progress is INF because of persistentKeyboardHeight, it's 0.0
Do you really need a reproducible repository? Let me know if there's anything I can do to help here.

@exzos28
Copy link
Author

exzos28 commented Dec 17, 2024

Can we just check if it is infinity?

progress = abs((height / persistentKeyboardHeight)).let { if (it.isNaN() || it.isInfinite()) 0.0 else it }

@kirillzyusko
Copy link
Owner

Do you really need a reproducible repository?

Well, if you can prepare it that would be a dope!

Can we just check if it is infinity?

We can and it will fix a crash, but I would like to use it only as a last resort. If we add that check we'll send progress=0 and height=45 (for example), which will add inconsistency - if you do height- based animations you will see that animation,
and if you use progress, then no 😔 🤷‍♂️

So my assumption is that we need to figure out why those insets changes are coming 🤔 Do you think it's doable to create a reproduction example?

@kirillzyusko kirillzyusko added 🤖 android Android specific 🎯 crash Library triggers a crash of the app 🌎 modal Anything that involves Modal usage labels Dec 17, 2024
@exzos28
Copy link
Author

exzos28 commented Dec 18, 2024

Hi!

Yes, I agree that the problem is somewhere higher. However, here obviously a division by 0 and an application crash can occur. I would add a check and an error log so that the application at least doesn't crash. Even with broken animation, but that's already a softer error than a crash.

However, I can no longer reproduce this. In another run of the project I did not find the error again. Creating a reproducible example with the same versions did not yield any errors either.

So far my project is in the process of migration to 0.76 and the new arch. Let the Issue stay open, I may find something within a week or two.

@kirillzyusko
Copy link
Owner

Okay @exzos28

I also see that a similar issue has been created in #737

I feel a pain, when the app crashes, so I think it makes sense to add a dirty workaround with converting a value to 0 to avoid crash. It's better to have an incorrect animation for end user rather than a crash of the application 😔

@kirillzyusko kirillzyusko linked a pull request Dec 19, 2024 that will close this issue
2 tasks
kirillzyusko pushed a commit that referenced this issue Dec 19, 2024
## 📜 Description

Add a numerical safety test to keyboard animation progress

## 💡 Motivation and Context

It solves a random crash in production where progress is infinite

Fixes
#739
#737
#735

## 📢 Changelog

### Android

- added `isInfinite` check along with `isNaN`;

## 🤔 How Has This Been Tested?

Releasing it to production via a patch package the error report
disappeared

## 📝 Checklist

- [x] CI successfully passed
- [x] I added new mocks and corresponding unit-tests if library API was
changed
@kirillzyusko
Copy link
Owner

I merged a fix - at least it should prevent a crash (but may lead to unexpected/inconsistent UI state: #740 (comment)

Let me know if you encounter any further issues. If you can prepare a reproduction example that would be a dope, since we can manage to discover a root problem and provide a fix 🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🤖 android Android specific 🎯 crash Library triggers a crash of the app 🌎 modal Anything that involves Modal usage
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants