Fixes NullPointerException at updateFidListener #3769
Conversation
Coverage Report 1Affected Products
Test Logs
Notes |
Size Report 1Affected Products
Test Logs
Notes |
| @@ -422,6 +422,8 @@ private void doNetworkCallIfNecessary(boolean forceRefresh) { | |||
| private synchronized void updateFidListener( | |||
| PersistedInstallationEntry prefs, PersistedInstallationEntry updatedPrefs) { | |||
| if (fidListeners.size() != 0 | |||
| && prefs.getFirebaseInstallationId() != null | |||
| && updatedPrefs.getFirebaseInstallationId() != null | |||
There was a problem hiding this comment.
How about cases where previous value was null but the new one is not null, or new value is null and previous is not?
Can these happen and should we handle them? @yoyomyo
There was a problem hiding this comment.
Thank you, @vkryachko, both cases you mentioned are possible.
When a previous value is null, and a new one is not null, we definitely want to update the listeners.
When a previous value is not null, and a new value is null, it means that an AUTH_ERROR may have occurred. The invalid FID will be cleared on the client [src].
Please feel free to disagree. For both cases, my opinion is to update the listeners, and kick the null value to individual listener to handle. In terms of NPE, maybe we could use this static equal function suggested by https://stackoverflow.com/a/11272385?
There was a problem hiding this comment.
Would passing in null to listeners be a breaking change? i.e. would they correctly handle null values? should probably do a review of usage at least in 2p(firebase) sdks
There was a problem hiding this comment.
Just creating some noise here. So the fix would be replacing !prefs.getFirebaseInstallationId().equals(updatedPrefs.getFirebaseInstallationId()) with !java.util.Objects.equals(prefs.getFirebaseInstallationId(), updatedPrefs.getFirebaseInstallationId())?
I've tried using java.util.Objects.equals(Object, Object) function, and it seems to work well with null values.
There was a problem hiding this comment.
The issue with Objects.equals is that it's only available in Java 7+ and is not available on older devices, so unfortunately you can't use it
There was a problem hiding this comment.
@argzdev Thank you! I agree with you on using !java.util.Objects.equals(prefs.getFirebaseInstallationId(), updatedPrefs.getFirebaseInstallationId()). We kick the null value to listeners because each listener would know better what to do in their individual case.
There was a problem hiding this comment.
Just to state it again, we cant use Objects.equals due to backwards compatibility issues, so we should either write our own or use com.google.android.gms.common.internal.Objects.equal() from play-services-basement
There was a problem hiding this comment.
From the same Stack Overflow link, perhaps we can use android.text.TextUtils? I'm not sure if it's backward compatible. Alternatively, we could just port the code of TextUtils.
public static boolean equals(CharSequence a, CharSequence b) {
if (a == b) return true;
int length;
if (a != null && b != null && (length = a.length()) == b.length()) {
if (a instanceof String && b instanceof String) {
return a.equals(b);
} else {
for (int i = 0; i < length; i++) {
if (a.charAt(i) != b.charAt(i)) return false;
}
return true;
}
}
return false;
}
What do you think? For com.google.android.gms.common.internal.Objects.equal(), I'll check this out.
There was a problem hiding this comment.
android.text.TextUtils sounds good to me! It's supported since API level 1. Thank you for finding this out, @argzdev.
There was a problem hiding this comment.
Got it, I've made the necessary changes. Thanks for reviewing @yoyomyo and @vkryachko!
Fixes #3760.
Adding null checker when updateFidListener is called.