fix: Android prevent crash when cleaning up uninitialized SDK on destroy #1871
Conversation
* Wrap removeHandlers() and removeObservers() calls in try-catch blocks within onHostDestroy() and onCatalystInstanceDestroy() to handle cases where the native OneSignal SDK throws "Must call 'initWithContext' before use" during activity destruction. * Also move the oneSignalInitDone flag further down past the native `OneSignal.initWithContext(context, appId)` call.
|
Change seem reasonable. I would consider updating/adding test for these. |
abdulraqeeb33
left a comment
abdulraqeeb33
left a comment
There was a problem hiding this comment.
also can we directly check the init from the OneSignal class?
| removeHandlers(); | ||
| removeObservers(); | ||
| } catch (Exception e) { | ||
| Logging.debug("OneSignal SDK not fully initialized. Could not remove handlers/observers: " + e.getMessage(), null); |
There was a problem hiding this comment.
This exception seems like a catch all.
I think we should first check if the SDK is initalized or not and then catch.
if(!oneSignalInitDone) {
Logging.debug("OneSignal React-Native SDK not initialized yet. Could not remove observers.", null);
return;
}
try {
removeHandlers();
removeObservers();
} catch (Exception e) {
Logging.debug("Could not remove handlers/observers: " + e.getMessage(), null);
}
There was a problem hiding this comment.
I moved the init done flag further down so that the SDK is done initializing. This hopefully is enough to prevent the crash without a try catch, which may be excessive after the init done flag change.
| @Override | ||
| public void onCatalystInstanceDestroy() { | ||
| removeHandlers(); | ||
| removeObservers(); |
| }; | ||
|
|
||
| private void removeObservers() { | ||
| if(!oneSignalInitDone) { |
@fadi-george When you worked on tests, did you get native code to be tested? |
I only added typescript tests |
Description
One Line Summary
Fix reported crashes when handlers / observers are removed on destroy and the SDK is uninitialized or cleaned up already.
Details
To address #1554
oneSignalInitDoneflag further down past the nativeOneSignal.initWithContext(context, appId)call. The native SDK already handles if initWithContext is called while it is still initializing.Motivation
Bug fix, fix crash
Scope
When activity being destroyed
Testing
Manual testing
Manually ran app in Android 35 emulator, app initializes, gets IAMs, get notifications.
Affected code checklist
Checklist
Overview
Testing
Final pass
This change is