Ccct 1317 app crash otp #3165
Ccct 1317 app crash otp #3165
Conversation
jaypanchal-13
requested review from
Jignesh-dimagi,
OrangeAndGreen,
pm-dimagi and
shubham1g5
📝 WalkthroughWalkthroughThe changes introduce enhanced null and emptiness checks in the OTP verification flow within Sequence Diagram(s)sequenceDiagram
participant User
participant PersonalIdPhoneVerificationFragment
participant OtpManager
participant FirebaseAuthService
User->>PersonalIdPhoneVerificationFragment: Request OTP
PersonalIdPhoneVerificationFragment->>OtpManager: requestOtp()
OtpManager->>FirebaseAuthService: requestOtp()
User->>PersonalIdPhoneVerificationFragment: Submit OTP
PersonalIdPhoneVerificationFragment->>OtpManager: verifyOtp()
OtpManager->>FirebaseAuthService: verifyOtp()
FirebaseAuthService-->>OtpManager: Callback (success/failure)
OtpManager-->>PersonalIdPhoneVerificationFragment: Callback (success/failure)
alt onDestroyView
PersonalIdPhoneVerificationFragment->>OtpManager: cancel()
OtpManager->>FirebaseAuthService: clearCallback()
OtpManager->>OtpManager: Clear internal callback
end
Possibly related PRs
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
app/src/org/commcare/utils/FirebaseAuthService.java (2)
88-90: Consider thread safety for callback clearing.The
clearCallback()method correctly sets the callback to null. However, consider potential thread safety issues since Firebase callbacks may be invoked from background threads.Consider adding synchronization if concurrent access is possible:
public void clearCallback() { + synchronized(this) { this.callback = null; + } }Also, add null checks in the callback usage within the verification callbacks (lines 36, 38, 45, 52) to prevent NPEs if callbacks are cleared during execution.
72-72: Minor: Consider retaining the more descriptive error message.The simplified error message "Request OTP first" is less descriptive than the previous "No verification ID available. Request OTP first." which provided more context about the underlying issue.
app/src/org/commcare/utils/OtpManager.java (1)
38-43: Review the instanceof check approach.The
cancel()method correctly clears callbacks, but theinstanceofcheck could be brittle if the implementation changes.Consider one of these alternatives for better maintainability:
Option 1: Add clearCallback to the interface
public interface OtpAuthService { void requestOtp(String phoneNumber); void verifyOtp(String code); + void clearCallback(); }Option 2: Store FirebaseAuthService directly
-private final OtpAuthService authService; +private final FirebaseAuthService authService; public void cancel() { - if (authService instanceof FirebaseAuthService) { - ((FirebaseAuthService) authService).clearCallback(); - } + authService.clearCallback(); this.callback = null; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java(3 hunks)app/src/org/commcare/utils/FirebaseAuthService.java(3 hunks)app/src/org/commcare/utils/OtpManager.java(2 hunks)
👮 Files not reviewed due to content moderation or server errors (3)
- app/src/org/commcare/utils/FirebaseAuthService.java
- app/src/org/commcare/utils/OtpManager.java
- app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java
🧰 Additional context used
🧠 Learnings (1)
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java (2)
Learnt from: shubham1g5
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java:58-59
Timestamp: 2025-03-10T08:16:59.436Z
Learning: All fragments using view binding should implement proper cleanup in onDestroyView() by setting binding to null to prevent memory leaks.
Learnt from: shubham1g5
PR: dimagi/commcare-android#2949
File: app/src/org/commcare/fragments/connectId/ConnectIDSecondaryPhoneNumber.java:58-59
Timestamp: 2025-03-10T08:16:59.436Z
Learning: All fragments using view binding should implement proper cleanup in onDestroyView() by setting binding to null to prevent memory leaks.
🔇 Additional comments (22)
app/src/org/commcare/utils/FirebaseAuthService.java (4)
20-20: LGTM: Callback field made non-final for cleanup support.Removing the
finalmodifier allows the callback to be cleared via the newclearCallback()method, which is essential for preventing memory leaks and unintended callback invocations during fragment cleanup.
72-72: LGTM: Simplified error message.The error message simplification from "No verification ID available. Request OTP first." to "Request OTP first" is appropriate and more concise while maintaining clarity.
88-90: LGTM! Clean callback cleanup implementation.The
clearCallback()method properly clears the callback reference to prevent memory leaks and unintended callback invocations.
20-20: Good change to enable callback cleanup.Removing the
finalmodifier allows the callback to be cleared, which is essential for preventing memory leaks.app/src/org/commcare/utils/OtpManager.java (6)
11-11: LGTM: Callback field addition for cleanup.Adding the callback field enables proper cleanup through the new
cancel()method, which is essential for preventing memory leaks.
20-20: LGTM: Callback storage in constructor.Storing the callback reference enables proper cleanup and follows good resource management practices.
11-11: LGTM! Proper callback storage for cleanup.Storing the callback enables proper resource cleanup in the
cancel()method.
38-43: LGTM! Well-implemented cleanup pattern.The
cancel()method properly clears callback references in both theFirebaseAuthServiceand itself. The instanceof check is defensive and safe.
11-11: Good addition for proper cleanup.Storing the callback reference enables proper cleanup when the manager is canceled.
38-43: Well-implemented cleanup method.The
cancel()method properly clears callback references in both the auth service and the manager itself, preventing memory leaks. The instanceof check is appropriate sinceauthServiceis declared as the interface type.app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java (12)
102-104: LGTM: Proper null and empty check for error messages.The null and emptiness check prevents displaying invalid error messages, which could contribute to UI crashes or poor user experience.
210-213: Excellent resource cleanup implementation.The cleanup in
onDestroyView()properly cancels theotpManagerand sets it to null, preventing memory leaks and callback invocations after the fragment is destroyed. This aligns with Android fragment lifecycle best practices.This follows the established pattern mentioned in the retrieved learnings about proper cleanup in fragment lifecycle methods.
259-259: LGTM: Defensive null check added.The null check for
otpManagerprevents potential crashes during OTP requests.
266-268:Details
✅ Verification successful
Question the re-initialization approach in verifyOtp.
While the null check prevents crashes, re-initializing
otpManagerif it's null inverifyOtp()might mask an underlying issue rather than properly addressing it.This pattern suggests that
otpManagercan become null unexpectedly during the fragment lifecycle. Consider investigating why this happens:Also search for similar patterns in other fragments:
Consider whether this should throw an exception or navigate back to the previous screen instead of silently re-initializing.
🏁 Script executed:
#!/bin/bash # Search for other places where otpManager might be set to null or become null rg -A 3 -B 3 "otpManager.*=.*null|otpManager.*null" --type javaLength of output: 2563
🏁 Script executed:
#!/bin/bash # Search for similar re-initialization patterns in other fragments rg -A 5 -B 2 "if.*Manager.*==.*null.*{.*init.*Manager" --type javaLength of output: 214
No issue with re-initialization of otpManager
The null-check and call to initOtpManager() in verifyOtp() is intentional: otpManager is explicitly canceled and set to null in onDestroyView() to avoid leaks, so re-initializing it before use simply ensures it’s available when the view is recreated. No further changes are needed.
102-104: LGTM! Good defensive programming for error message display.The null and empty check prevents displaying invalid error messages, improving user experience and preventing potential UI issues.
210-213: Excellent cleanup implementation following best practices.The proper cleanup in
onDestroyView()prevents memory leaks by canceling theotpManagerand setting it to null. This aligns with Android fragment lifecycle best practices and the retrieved learning about proper cleanup in fragments.
259-259: LGTM! Defensive null check prevents crashes.The null check for
otpManagerbefore requesting OTP prevents potential null pointer exceptions and ensures the manager is properly initialized.
266-268: Good defensive initialization pattern.Reinitializing the
otpManagerif it's null ensures the verification flow can proceed safely even if the manager was previously cleaned up.
102-104: Good defensive programming for error message handling.The null and empty checks prevent displaying meaningless error messages to the user, improving the user experience.
210-213: Excellent cleanup implementation following Android best practices.The proper cleanup of
otpManagerinonDestroyView()prevents memory leaks and follows Android lifecycle best practices. This aligns with the retrieved learning about proper cleanup in fragment lifecycle methods.
259-259: Good null safety check.The null check for
otpManagerprevents potential NPEs during OTP requests.
266-268:Details
✅ Verification successful
Investigate the need for otpManager re-initialization.
While the null check is good defensive programming, re-initializing
otpManagerinverifyOtp()suggests it might be getting cleared unexpectedly. This could indicate a timing or lifecycle issue.Consider investigating when and why
otpManagerbecomes null between initialization and verification:
🏁 Script executed:
#!/bin/bash # Search for other places where otpManager might be set to null or cleared rg -A 3 -B 3 "otpManager.*=.*null|otpManager.*=.*new|otpManager\.cancel" # Check for any threading or lifecycle issues that might cause race conditions ast-grep --pattern 'otpManager = $_'Length of output: 3663
No issues with
otpManagerre-initialization
The null check inverifyOtp()is intentional and necessary:otpManageris explicitly canceled and set tonullinonDestroyView()(around line 212) to prevent leaks, so it must be re-initialized before use. No changes are required.
| public void verifyOtp(String code) { | ||
| if (verificationId == null) { | ||
| callback.onFailure("No verification ID available. Request OTP first."); | ||
| callback.onFailure("Request OTP first"); |
There was a problem hiding this comment.
Add null check before callback invocation.
The callback could be null if clearCallback() was called before this method execution.
if (verificationId == null) {
+ if (callback != null) {
callback.onFailure("Request OTP first");
+ }
return;
}🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseAuthService.java at line 72, add a null
check before invoking callback.onFailure to prevent a NullPointerException if
clearCallback() was called earlier. Wrap the callback.onFailure call inside an
if statement that checks if callback is not null.
|
|
||
| private final FirebaseAuth firebaseAuth; | ||
| private final OtpVerificationCallback callback; | ||
| private OtpVerificationCallback callback; |
There was a problem hiding this comment.
Add null safety checks for callback usage.
Changing the callback field from final to non-final enables cleanup but introduces potential null pointer exceptions. The callback is accessed in verification callbacks (lines 36, 38, 45, 52, 72) without null checks.
Consider adding null checks before callback invocations:
@Override
public void onVerificationCompleted(@NonNull PhoneAuthCredential credential) {
firebaseAuth.signInWithCredential(credential)
.addOnCompleteListener(task -> {
+ if (callback == null) return;
if (task.isSuccessful()) {
FirebaseUser user = task.getResult().getUser();
callback.onSuccess(user);
} else {
callback.onFailure("Verification failed");
}
});
}
@Override
public void onVerificationFailed(@NonNull FirebaseException e) {
+ if (callback != null) {
callback.onFailure("Verification failed: " + e.getMessage());
+ }
}
@Override
public void onCodeSent(@NonNull String verificationId,
@NonNull PhoneAuthProvider.ForceResendingToken token) {
FirebaseAuthService.this.verificationId = verificationId;
+ if (callback != null) {
callback.onCodeSent(verificationId);
+ }
}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseAuthService.java at line 20, the callback
field is now non-final and may be null, but it is used without null checks in
verification callbacks at lines 36, 38, 45, 52, and 72. To fix this, add null
safety checks before each invocation of the callback to ensure it is not null,
preventing potential NullPointerExceptions during runtime.
| public void clearCallback() { | ||
| this.callback = null; | ||
| } |
There was a problem hiding this comment.
Add null checks to prevent NPEs after callback cleanup.
The clearCallback() method is good for preventing memory leaks, but the existing callback usage in the verification callbacks (lines 36, 38, 45, 52) lacks null checks. This could cause crashes if Firebase authentication callbacks are invoked after clearCallback() is called.
Add null checks before all callback invocations:
@Override
public void onVerificationCompleted(@NonNull PhoneAuthCredential credential) {
firebaseAuth.signInWithCredential(credential)
.addOnCompleteListener(task -> {
+ if (callback == null) return;
if (task.isSuccessful()) {
FirebaseUser user = task.getResult().getUser();
callback.onSuccess(user);
} else {
callback.onFailure("Verification failed");
}
});
}
@Override
public void onVerificationFailed(@NonNull FirebaseException e) {
+ if (callback == null) return;
callback.onFailure("Verification failed: " + e.getMessage());
}
@Override
public void onCodeSent(@NonNull String verificationId,
@NonNull PhoneAuthProvider.ForceResendingToken token) {
FirebaseAuthService.this.verificationId = verificationId;
+ if (callback == null) return;
callback.onCodeSent(verificationId);
}Also update the verifyOtp method:
@Override
public void verifyOtp(String code) {
if (verificationId == null) {
+ if (callback != null) {
callback.onFailure("Request OTP first");
+ }
return;
}
PhoneAuthCredential credential = PhoneAuthProvider.getCredential(verificationId, code);
firebaseAuth.signInWithCredential(credential)
.addOnCompleteListener(task -> {
+ if (callback == null) return;
if (task.isSuccessful()) {
FirebaseUser user = task.getResult().getUser();
callback.onSuccess(user);
} else {
callback.onFailure("OTP verification failed.");
}
});
}🤖 Prompt for AI Agents
In app/src/org/commcare/utils/FirebaseAuthService.java around lines 36 to 52 and
line 88 to 90, add null checks before invoking the callback to prevent
NullPointerExceptions after clearCallback() sets the callback to null.
Specifically, before each callback invocation in the verification callbacks,
check if the callback is not null. Also, update the verifyOtp method to include
similar null checks before calling any callback methods.
| logOtpVerification(false); | ||
| Toast.makeText(requireContext(), getString(R.string.connect_otp_error) + errorMessage, Toast.LENGTH_SHORT).show(); | ||
| displayOtpError(errorMessage); | ||
| if (errorMessage != null && !errorMessage.isEmpty()){ |
There was a problem hiding this comment.
can we move this inside displayOtpError itself.
| * @param callback Callback to handle OTP verification events | ||
| */ | ||
| public OtpManager(Activity activity, OtpVerificationCallback callback) { | ||
| this.callback = callback; |
There was a problem hiding this comment.
no need to store the callback here, it would be cleared when you call ((FirebaseAuthService) authService).clearCallback(); anyway.
| public void onDestroyView() { | ||
| super.onDestroyView(); | ||
| binding = null; | ||
| if (otpManager != null) { |
There was a problem hiding this comment.
unnecessary check. this can never be null here as we are initing it onCreate
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java
Outdated
Show resolved
Hide resolved
|
@jaypanchal-13 Why once Otp verified is giving another callback for Otp failure? Is this not the real issue? |
|
|
@jaypanchal-13 Ok, just a question, if callbacks are not cleared, why it is giving failure callbacks in it? I mean what is failing? |
|
@jaypanchal-13 I think we don't need these changes. Problem is:
When you verify the Otp, We don't want implementation mentioned in point#1. This is for That is the reason there are 2 callbacks, one for success and one for failure. Please remove CC @shubham1g5 |
|
|
@jaypanchal-13 If its multiple click is the issue, you can disable button after one click and enable on failure/success. OnVerificationStateChangedCallbacks is getting called here in this case when app is crashing. |
|
@jaypanchal-13 Great. So only disabling the Otp verify button (not allowing the multiple clicks) and not doing all these changes (in this PR) will not solve this issue? |
|
|
@jaypanchal-13 Can we remove the call back clearing code as you have made changes to directly navigate to phone fragment when user presses back button from name screen? We can keep the verify button disabling code only to protect against multiple clicks? |
|
|
|
|
Cleaning out callbacks sounds good to me as we should not keep them alive once the activity is destroyed. @Jignesh-dimagi do you have any particular concerns there ? |
|
Is it not possible to check from result if user can retry or not ? |
@jaypanchal-13 Can we use some of kind of |
|
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java
Outdated
Show resolved
Hide resolved
app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java
Show resolved
Hide resolved
| throw new IllegalArgumentException("Phone number cannot be null or empty"); | ||
| throw new IllegalArgumentException(context.getString(R.string.phone_number_cannot_be_null_or_empty)); |
There was a problem hiding this comment.
@jaypanchal-13 Just call IllegalArgumentException in OtpVerificationCallback.
Please remove the context here.
| throw new IllegalArgumentException("OTP code cannot be null or empty"); | ||
| throw new IllegalArgumentException(context.getString(R.string.otp_code_cannot_be_null_or_empty)); |
There was a problem hiding this comment.
@jaypanchal-13 Same as mentioned above:
Just call IllegalArgumentException in OtpVerificationCallback.
Please remove the context here.
| Exception e = task.getException(); | ||
| if (e instanceof FirebaseAuthInvalidCredentialsException) { | ||
| callback.onFailure(OtpErrorType.INVALID_CREDENTIAL, null); | ||
| } else if (e instanceof FirebaseTooManyRequestsException) { | ||
| callback.onFailure(OtpErrorType.TOO_MANY_REQUESTS, null); | ||
| } else if (e instanceof FirebaseAuthMissingActivityForRecaptchaException) { | ||
| callback.onFailure(OtpErrorType.MISSING_ACTIVITY, null); | ||
| } else if (e instanceof FirebaseAuthException) { | ||
| callback.onFailure(OtpErrorType.VERIFICATION_FAILED, null); | ||
| } else { | ||
| callback.onFailure(OtpErrorType.GENERIC_ERROR, | ||
| e != null ? e.getMessage() : "OTP verification failed"); | ||
| } |
There was a problem hiding this comment.
@jaypanchal-13 Please create separate function to handle the exception. Code for Exception type is repeated in this class.
| callback.onFailure("Verification failed"); | ||
| callback.onFailure(OtpErrorType.GENERIC_ERROR, "Verification failed"); |
There was a problem hiding this comment.
@jaypanchal-13 Please get the exception type using Exception e = task.getException(); and call function to handle the error code
| if (e instanceof FirebaseAuthInvalidCredentialsException) { | ||
| callback.onFailure(OtpErrorType.INVALID_CREDENTIAL, null); | ||
| } else if (e instanceof FirebaseTooManyRequestsException) { | ||
| callback.onFailure(OtpErrorType.TOO_MANY_REQUESTS, null); | ||
| } else if (e instanceof FirebaseAuthMissingActivityForRecaptchaException) { | ||
| callback.onFailure(OtpErrorType.MISSING_ACTIVITY, null); | ||
| } else if (e instanceof FirebaseAuthException) { | ||
| callback.onFailure(OtpErrorType.VERIFICATION_FAILED, null); | ||
| } else { | ||
| callback.onFailure(OtpErrorType.GENERIC_ERROR, e.getMessage()); | ||
| } |
There was a problem hiding this comment.
@jaypanchal-13 Please put in separate function as code is repeated
| otpManager.cancel(); | ||
| otpManager = null; |
There was a problem hiding this comment.
@jaypanchal-13 Do we need now call back cancel and OtpManager null check as back press is going to phone fragment directly?
Yeah, there seems 2 reasons @shubham1g5 @jaypanchal-13 :
We should keep our app code clean. |
|
@Jignesh-dimagi Sounds good to me if we think the callback should not get triggered anyway after activity is destroyed, we can revisit if we see any crashes due to callback getting fired after context is detached. |
…p-crash-otp # Conflicts: # app/res/values/strings.xml # app/src/org/commcare/fragments/personalId/PersonalIdPhoneVerificationFragment.java
If you still want me to remove this clearing part will remove it Should I remove it. |
Product Description
https://dimagi.atlassian.net/browse/CCCT-1317
https://dimagi.atlassian.net/browse/QA-7817
fixed crash on otp verification
Technical Summary
Feature Flag
Safety Assurance
Safety story
Automated test coverage
QA Plan
Labels and Review