Fix NPE on phone boot and SIM removal
In some cases TelecomManager.getCallCapablePhoneAccounts() might return
a PhoneAccountHandle with a invalid subId on boot, which will cause
a NPE when trying to convert the subId back to a PhoneAccountHandle
in SimChangeReceiver.processSubId().
SimChangeReceiver also might be triggered with a invalid subId that
is not SubscriptionManager.INVALID_SUBSCRIPTION_ID.
SubscriptionManager.isValidSubscriptionId() should be used instead of
comparing with the constant.
In this CL the subId is validated on the above events.
PhoneAccountHandleConverter.fromSubId() will also handle invalid subId
gracefully and return null. The null return value is checked in all
usages.
Change-Id: Ie703b1a2a826a2951e8cc90fccc6badd76928bd3
Fixes: 30474294
diff --git a/src/com/android/phone/VoicemailStatus.java b/src/com/android/phone/VoicemailStatus.java
index c5cd52c..4d7d388 100644
--- a/src/com/android/phone/VoicemailStatus.java
+++ b/src/com/android/phone/VoicemailStatus.java
@@ -16,6 +16,7 @@
package com.android.phone;
+import android.annotation.Nullable;
import android.content.ContentResolver;
import android.content.ContentValues;
import android.content.Context;
@@ -23,14 +24,16 @@
import android.provider.VoicemailContract;
import android.provider.VoicemailContract.Status;
import android.telecom.PhoneAccountHandle;
-import com.android.phone.vvm.omtp.utils.PhoneAccountHandleConverter;
+import com.android.phone.vvm.omtp.VvmLog;
public class VoicemailStatus {
+ private static final String TAG = "VvmStatus";
public static class Editor {
private final Context mContext;
+ @Nullable
private final PhoneAccountHandle mPhoneAccountHandle;
private ContentValues mValues = new ContentValues();
@@ -38,8 +41,13 @@
private Editor(Context context, PhoneAccountHandle phoneAccountHandle) {
mContext = context;
mPhoneAccountHandle = phoneAccountHandle;
+ if (mPhoneAccountHandle == null) {
+ VvmLog.w(TAG, "VoicemailStatus.Editor created with null phone account, status will"
+ + " not be written");
+ }
}
+ @Nullable
public PhoneAccountHandle getPhoneAccountHandle() {
return mPhoneAccountHandle;
}
@@ -76,6 +84,9 @@
}
public void apply() {
+ if (mPhoneAccountHandle == null) {
+ return;
+ }
mValues.put(Status.PHONE_ACCOUNT_COMPONENT_NAME,
mPhoneAccountHandle.getComponentName().flattenToString());
mValues.put(Status.PHONE_ACCOUNT_ID, mPhoneAccountHandle.getId());
@@ -116,23 +127,20 @@
return new Editor(context, phoneAccountHandle);
}
- public static Editor edit(Context context, int subId) {
- return new Editor(context, PhoneAccountHandleConverter.fromSubId(subId));
- }
-
/**
* Reset the status to the "disabled" state, which the UI should not show anything for this
* subId.
*/
- public static void disable(Context context, int subId) {
- edit(context, subId)
+ public static void disable(Context context, PhoneAccountHandle phoneAccountHandle) {
+ edit(context, phoneAccountHandle)
.setConfigurationState(Status.CONFIGURATION_STATE_NOT_CONFIGURED)
.setDataChannelState(Status.DATA_CHANNEL_STATE_NO_CONNECTION)
.setNotificationChannelState(Status.NOTIFICATION_CHANNEL_STATE_NO_CONNECTION)
.apply();
}
- public static DeferredEditor deferredEdit(Context context, int subId) {
- return new DeferredEditor(context, PhoneAccountHandleConverter.fromSubId(subId));
+ public static DeferredEditor deferredEdit(Context context,
+ PhoneAccountHandle phoneAccountHandle) {
+ return new DeferredEditor(context, phoneAccountHandle);
}
}
diff --git a/src/com/android/phone/vvm/omtp/ActivationTask.java b/src/com/android/phone/vvm/omtp/ActivationTask.java
index aa262c0..716140c 100644
--- a/src/com/android/phone/vvm/omtp/ActivationTask.java
+++ b/src/com/android/phone/vvm/omtp/ActivationTask.java
@@ -126,19 +126,25 @@
Assert.isNotMainThread();
int subId = getSubId();
+ PhoneAccountHandle phoneAccountHandle = PhoneAccountHandleConverter.fromSubId(subId);
+ if (phoneAccountHandle == null) {
+ // This should never happen
+ VvmLog.e(TAG, "null phone account for subId " + subId);
+ return;
+ }
+
OmtpVvmCarrierConfigHelper helper = new OmtpVvmCarrierConfigHelper(getContext(), subId);
if (!helper.isValid()) {
VvmLog.i(TAG, "VVM not supported on subId " + subId);
- VoicemailStatus.disable(getContext(), subId);
+ VoicemailStatus.disable(getContext(), phoneAccountHandle);
return;
}
- PhoneAccountHandle phoneAccountHandle = PhoneAccountHandleConverter.fromSubId(subId);
if (!OmtpVvmSourceManager.getInstance(getContext())
.isVvmSourceRegistered(phoneAccountHandle)) {
// Only show the "activating" message if activation has not been completed before in
// this boot. Subsequent activations are more of a status check and usually does not
// concern the user.
- helper.handleEvent(VoicemailStatus.edit(getContext(), subId),
+ helper.handleEvent(VoicemailStatus.edit(getContext(), phoneAccountHandle),
OmtpEvents.CONFIG_ACTIVATING);
}
diff --git a/src/com/android/phone/vvm/omtp/OmtpVvmCarrierConfigHelper.java b/src/com/android/phone/vvm/omtp/OmtpVvmCarrierConfigHelper.java
index 5d06d1e..b4fbb3f 100644
--- a/src/com/android/phone/vvm/omtp/OmtpVvmCarrierConfigHelper.java
+++ b/src/com/android/phone/vvm/omtp/OmtpVvmCarrierConfigHelper.java
@@ -135,9 +135,13 @@
return mSubId;
}
+ @Nullable
public PhoneAccountHandle getPhoneAccountHandle() {
if (mPhoneAccountHandle == null) {
mPhoneAccountHandle = PhoneAccountHandleConverter.fromSubId(mSubId);
+ if (mPhoneAccountHandle == null) {
+ VvmLog.e(TAG, "null phone account for subId " + mSubId);
+ }
}
return mPhoneAccountHandle;
}
@@ -305,7 +309,13 @@
}
public void startActivation() {
- VoicemailStatus.edit(mContext, mSubId)
+ PhoneAccountHandle phoneAccountHandle = getPhoneAccountHandle();
+ if (phoneAccountHandle == null) {
+ // This should never happen
+ // Error logged in getPhoneAccountHandle().
+ return;
+ }
+ VoicemailStatus.edit(mContext, phoneAccountHandle)
.setType(getVvmType())
.apply();
diff --git a/src/com/android/phone/vvm/omtp/SimChangeReceiver.java b/src/com/android/phone/vvm/omtp/SimChangeReceiver.java
index ea7dfa6..25c27db 100644
--- a/src/com/android/phone/vvm/omtp/SimChangeReceiver.java
+++ b/src/com/android/phone/vvm/omtp/SimChangeReceiver.java
@@ -66,7 +66,7 @@
int subId = intent.getIntExtra(PhoneConstants.SUBSCRIPTION_KEY,
SubscriptionManager.INVALID_SUBSCRIPTION_ID);
- if (subId == SubscriptionManager.INVALID_SUBSCRIPTION_ID) {
+ if (!SubscriptionManager.isValidSubscriptionId(subId)) {
VvmLog.i(TAG, "Received SIM change for invalid subscription id.");
return;
}
@@ -83,11 +83,16 @@
}
public static void processSubId(Context context, int subId) {
+ PhoneAccountHandle phoneAccount = PhoneAccountHandleConverter.fromSubId(subId);
+ if (phoneAccount == null) {
+ // This should never happen
+ VvmLog.e(TAG, "unable to convert subId " + subId + " to PhoneAccountHandle");
+ return;
+ }
+
OmtpVvmCarrierConfigHelper carrierConfigHelper =
new OmtpVvmCarrierConfigHelper(context, subId);
if (carrierConfigHelper.isValid()) {
- PhoneAccountHandle phoneAccount = PhoneAccountHandleConverter.fromSubId(subId);
-
if (VisualVoicemailSettingsUtil.isEnabled(context, phoneAccount)) {
VvmLog.i(TAG, "Sim state or carrier config changed for " + subId);
// Add a phone state listener so that changes to the communication channels
@@ -110,7 +115,7 @@
String mccMnc = context.getSystemService(TelephonyManager.class).getSimOperator(subId);
VvmLog.d(TAG,
"visual voicemail not supported for carrier " + mccMnc + " on subId " + subId);
- VoicemailStatus.disable(context, subId);
+ VoicemailStatus.disable(context, phoneAccount);
}
}
diff --git a/src/com/android/phone/vvm/omtp/VvmBootCompletedReceiver.java b/src/com/android/phone/vvm/omtp/VvmBootCompletedReceiver.java
index fe1f4cb..6bd7fa1 100644
--- a/src/com/android/phone/vvm/omtp/VvmBootCompletedReceiver.java
+++ b/src/com/android/phone/vvm/omtp/VvmBootCompletedReceiver.java
@@ -21,7 +21,7 @@
import android.content.Intent;
import android.telecom.PhoneAccountHandle;
import android.telecom.TelecomManager;
-
+import android.telephony.SubscriptionManager;
import com.android.phone.vvm.omtp.utils.PhoneAccountHandleConverter;
/**
@@ -49,6 +49,12 @@
for (PhoneAccountHandle handle : TelecomManager.from(context)
.getCallCapablePhoneAccounts()) {
int subId = PhoneAccountHandleConverter.toSubId(handle);
+ if (!SubscriptionManager.isValidSubscriptionId(subId)) {
+ // b/30474294 getCallCapablePhoneAccounts() might return a PhoneAccountHandle with
+ // invalid subId if no SIM is inserted.
+ VvmLog.e(TAG, "phone account " + handle + " has invalid subId " + subId);
+ continue;
+ }
VvmLog.v(TAG, "processing subId " + subId);
SimChangeReceiver.processSubId(context, subId);
}
diff --git a/src/com/android/phone/vvm/omtp/protocol/Vvm3EventHandler.java b/src/com/android/phone/vvm/omtp/protocol/Vvm3EventHandler.java
index 6fac708..d95879f 100644
--- a/src/com/android/phone/vvm/omtp/protocol/Vvm3EventHandler.java
+++ b/src/com/android/phone/vvm/omtp/protocol/Vvm3EventHandler.java
@@ -113,6 +113,12 @@
OmtpEvents event) {
switch (event) {
case CONFIG_REQUEST_STATUS_SUCCESS:
+ if (status.getPhoneAccountHandle() == null) {
+ // This should never happen.
+ Log.e(TAG, "status editor has null phone account handle");
+ return true;
+ }
+
if (!VoicemailChangePinActivity
.isDefaultOldPinSet(context, status.getPhoneAccountHandle())) {
return false;
diff --git a/src/com/android/phone/vvm/omtp/scheduling/RetryPolicy.java b/src/com/android/phone/vvm/omtp/scheduling/RetryPolicy.java
index bd0c75e..4f4126a 100644
--- a/src/com/android/phone/vvm/omtp/scheduling/RetryPolicy.java
+++ b/src/com/android/phone/vvm/omtp/scheduling/RetryPolicy.java
@@ -17,8 +17,10 @@
package com.android.phone.vvm.omtp.scheduling;
import android.content.Intent;
+import android.telecom.PhoneAccountHandle;
import com.android.phone.VoicemailStatus;
import com.android.phone.vvm.omtp.VvmLog;
+import com.android.phone.vvm.omtp.utils.PhoneAccountHandleConverter;
/**
* A task with this policy will automatically re-queue itself if {@link BaseTask#fail()} has been
@@ -67,8 +69,15 @@
+ mRetryDelayMillis);
mTask.setExecutionTime(mTask.getTimeMillis() + mRetryDelayMillis);
}
- mVoicemailStatusEditor = VoicemailStatus.deferredEdit(task.getContext(),
- task.getSubId());
+ PhoneAccountHandle phoneAccountHandle = PhoneAccountHandleConverter
+ .fromSubId(task.getSubId());
+ if (phoneAccountHandle == null) {
+ VvmLog.e(TAG, "null phone account for subId " + task.getSubId());
+ // This should never happen, but continue on if it does. The status write will be
+ // discarded.
+ }
+ mVoicemailStatusEditor = VoicemailStatus
+ .deferredEdit(task.getContext(), phoneAccountHandle);
}
@Override
diff --git a/src/com/android/phone/vvm/omtp/sms/OmtpMessageReceiver.java b/src/com/android/phone/vvm/omtp/sms/OmtpMessageReceiver.java
index 7b4d2c3..0d49ebe 100644
--- a/src/com/android/phone/vvm/omtp/sms/OmtpMessageReceiver.java
+++ b/src/com/android/phone/vvm/omtp/sms/OmtpMessageReceiver.java
@@ -53,7 +53,8 @@
PhoneAccountHandle phone = PhoneAccountHandleConverter.fromSubId(subId);
if (phone == null) {
- VvmLog.i(TAG, "Received message for null phone account");
+ // This should never happen
+ VvmLog.i(TAG, "Received message for null phone account on subId " + subId);
return;
}
diff --git a/src/com/android/phone/vvm/omtp/sync/UploadTask.java b/src/com/android/phone/vvm/omtp/sync/UploadTask.java
index fc28acf..87c0a46 100644
--- a/src/com/android/phone/vvm/omtp/sync/UploadTask.java
+++ b/src/com/android/phone/vvm/omtp/sync/UploadTask.java
@@ -20,6 +20,7 @@
import android.content.Intent;
import android.telecom.PhoneAccountHandle;
import com.android.phone.VoicemailStatus;
+import com.android.phone.vvm.omtp.VvmLog;
import com.android.phone.vvm.omtp.scheduling.BaseTask;
import com.android.phone.vvm.omtp.scheduling.PostponePolicy;
import com.android.phone.vvm.omtp.utils.PhoneAccountHandleConverter;
@@ -30,6 +31,8 @@
*/
public class UploadTask extends BaseTask {
+ private static final String TAG = "VvmUploadTask";
+
private static final int POSTPONE_MILLIS = 5_000;
public UploadTask() {
@@ -52,8 +55,15 @@
@Override
public void onExecuteInBackgroundThread() {
OmtpVvmSyncService service = new OmtpVvmSyncService(getContext());
+
+ PhoneAccountHandle phoneAccountHandle = PhoneAccountHandleConverter.fromSubId(getSubId());
+ if (phoneAccountHandle == null) {
+ // This should never happen
+ VvmLog.e(TAG, "null phone account for subId " + getSubId());
+ return;
+ }
service.sync(this, OmtpVvmSyncService.SYNC_UPLOAD_ONLY,
- PhoneAccountHandleConverter.fromSubId(getSubId()), null,
- VoicemailStatus.edit(getContext(), getSubId()));
+ phoneAccountHandle, null,
+ VoicemailStatus.edit(getContext(), phoneAccountHandle));
}
}
diff --git a/src/com/android/phone/vvm/omtp/utils/PhoneAccountHandleConverter.java b/src/com/android/phone/vvm/omtp/utils/PhoneAccountHandleConverter.java
index 0474452..ad4c423 100644
--- a/src/com/android/phone/vvm/omtp/utils/PhoneAccountHandleConverter.java
+++ b/src/com/android/phone/vvm/omtp/utils/PhoneAccountHandleConverter.java
@@ -16,10 +16,13 @@
package com.android.phone.vvm.omtp.utils;
+import android.annotation.Nullable;
import android.telecom.PhoneAccountHandle;
import android.telephony.SubscriptionManager;
-
+import com.android.internal.telephony.Phone;
+import com.android.internal.telephony.PhoneFactory;
import com.android.phone.PhoneUtils;
+import com.android.phone.vvm.omtp.VvmLog;
/**
* Utility to convert between PhoneAccountHandle and subId, which is a common operation in OMTP
@@ -29,9 +32,22 @@
*/
public class PhoneAccountHandleConverter {
+ private static final String TAG = "PhoneAccountHndCvtr";
+
+ @Nullable
public static PhoneAccountHandle fromSubId(int subId) {
- return PhoneUtils.makePstnPhoneAccountHandle(
- SubscriptionManager.getPhoneId(subId));
+ if (!SubscriptionManager.isValidSubscriptionId(subId)) {
+ VvmLog.e(TAG, "invalid subId " + subId);
+ return null;
+ }
+ // Calling PhoneUtils.makePstnPhoneAccountHandle() with a phoneId might throw a NPE if the
+ // phone object cannot be found, so the Phone object should be created and checked here.
+ Phone phone = PhoneFactory.getPhone(SubscriptionManager.getPhoneId(subId));
+ if (phone == null) {
+ VvmLog.e(TAG, "Unable to find Phone for subId " + subId);
+ return null;
+ }
+ return PhoneUtils.makePstnPhoneAccountHandle(phone);
}
public static int toSubId(PhoneAccountHandle handle) {