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: native alert crash #178

Merged
merged 2 commits into from
Jun 21, 2023
Merged

fix: native alert crash #178

merged 2 commits into from
Jun 21, 2023

Conversation

kirillzyusko
Copy link
Owner

@kirillzyusko kirillzyusko commented Jun 19, 2023

📜 Description

Fixed a crash on iOS 16.5 when native alert is getting shown.

💡 Motivation and Context

The original exception occurred because of this error:

libc++abi: terminating due to uncaught exception of type folly::json::print_error: folly::toJson: JSON object value was an INF when serializing value at "progress"

progress was inf because in:

onEvent("onKeyboardMove", keyboardPosition as NSNumber, keyboardPosition / CGFloat(keyboardHeight) as NSNumber)

keyboardHeight variable was 0. When we were trying to divide any number by 0 in swfit we get Inf value, and this value can not be serialised to JS equivalent.

The reason why this value becomes 0 is the fact that we have next chain of calls:

keyboardWillHide
keyboardDidHide <- here we are setting keyboardHeight=0
keyboardWillHide <- here we setup CADisplayLink and after 16ms the crash will occur
keyboardDidHide

Based on output from above I had 3 options on how to fix the error:

1. Add .isFinite check

Good option, but it will prevent sending onMove event, though according to values from CADisplayLink movement actually happens. So we will kind of ignore actual information and it's not something that satisfy me, because it may produce other bugs in the future.

2. Update keyboardHeight in keyboardWillHide

This variant also prevents a crash, but progress value in onMove sometimes can be higher than 1 (in my tests sometimes it was 1.3+). And it also seems like it gives incorrect calculation, because it's strange to see progress > 1, when keyboard simply hides 🤷‍♂️

3. Don't set keyboardHeight to 0

To come back to 1.4.x behaviour, we don't need to reset keyboardHeight to 0. But I've added this code intentionally in 1.5.0, because in KV observer we are checking that keyboard should be open and not animating:

if displayLink != nil || keyboardHeight == 0.0 {
  return
}

I've added this check, because I've attached KV observer before any keyboard movement. The reason why I've added it in UIWindow.didBecomeVisibleNotification/UIWindow.didBecomeHiddenNotification was the fact, that these methods were called only one time (keyboard events can be fired multiple times). But I've added hasKVObserver variable in #146 so it's safe to call setupKVObserver/removeKVObserver multiple times.

So in this PR I've decided to stick to this approach - don't reset keyboardHeight to 0 + setup KV listeners in different lifecycle methods.

Should close #175

📢 Changelog

iOS

  • moved setupKVObserver/removeKVObserver to keyboaardDidAppear/keyboardWilHide lifecycle;
  • don't set keyboardHeight to 0 (this value acts as persisteKeyboardHeight on Android);

🤔 How Has This Been Tested?

Tested on:

  • iPhone 6s (iOS 15.6);
  • iPhone 14 Pro (iOS 16.2);
  • iPhone 11 (iOS 16.5);

📸 Screenshots (if appropriate):

Before After
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-06-19.at.14.55.33.mp4
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-06-19.at.14.45.26.mp4

📝 Checklist

  • CI successfully passed

@kirillzyusko kirillzyusko added 🍎 iOS iOS specific 🎯 crash Library triggers a crash of the app labels Jun 19, 2023
@kirillzyusko kirillzyusko self-assigned this Jun 19, 2023
@kirillzyusko kirillzyusko merged commit afd0809 into main Jun 21, 2023
@kirillzyusko kirillzyusko deleted the fix/175-native-alert-crash branch June 21, 2023 06:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🎯 crash Library triggers a crash of the app 🍎 iOS iOS specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

iOS crash when native alert is shown while keyboard is visible
1 participant