Improvements to the stability of SipDelegateManager based on CTS Testing
1) new shell command to clear the carrier ImsService override for more stable testing.
2) DelegateStateTracker - dedupe DelegateRegistrationState updates if
they result in the same reg state being sent.
3) SipDelegateBinderConnection - Add linkToDeath on sip transport binder
interface to unblock pending create/destroy messages if the service
dies while waiting for a result from one of these commands.
4) SipDelegateController - ensure that when all features are denied we
do not incorrectly skip the change operation. Also, fixed some race
conditions between the transport onCreated command being sent after
onFeatureTagsChanged.
5) SipTransportController - Added OBSERVE_ROLE_HOLDERS to
AndroidManifest and some better debug logging around role changes.
Bug: 154763999
Test: atest TeleServiceTests
Change-Id: Iad6cf386943769a3b3a95a1f8e793b6e129d468c
diff --git a/src/com/android/phone/PhoneInterfaceManager.java b/src/com/android/phone/PhoneInterfaceManager.java
index 52b8f0a..beda55c 100755
--- a/src/com/android/phone/PhoneInterfaceManager.java
+++ b/src/com/android/phone/PhoneInterfaceManager.java
@@ -5253,6 +5253,36 @@
}
/**
+ * Clears any carrier ImsService overrides for the slot index specified that were previously
+ * set with {@link #setBoundImsServiceOverride(int, boolean, int[], String)}.
+ *
+ * This should only be used for testing.
+ *
+ * @param slotIndex the slot ID that the ImsService should bind for.
+ * @return true if clearing the carrier ImsService override succeeded or false if it did not.
+ */
+ @Override
+ public boolean clearCarrierImsServiceOverride(int slotIndex) {
+ int[] subIds = SubscriptionManager.getSubId(slotIndex);
+ TelephonyPermissions.enforceShellOnly(Binder.getCallingUid(),
+ "clearCarrierImsServiceOverride");
+ TelephonyPermissions.enforceCallingOrSelfModifyPermissionOrCarrierPrivilege(mApp,
+ (subIds != null ? subIds[0] : SubscriptionManager.INVALID_SUBSCRIPTION_ID),
+ "clearCarrierImsServiceOverride");
+
+ final long identity = Binder.clearCallingIdentity();
+ try {
+ if (mImsResolver == null) {
+ // may happen if the device does not support IMS.
+ return false;
+ }
+ return mImsResolver.clearCarrierImsServiceConfiguration(slotIndex);
+ } finally {
+ Binder.restoreCallingIdentity(identity);
+ }
+ }
+
+ /**
* Return the package name of the currently bound ImsService.
*
* @param slotId The slot that the ImsService is associated with.
diff --git a/src/com/android/phone/TelephonyShellCommand.java b/src/com/android/phone/TelephonyShellCommand.java
index c3f2974..d7b4c52 100644
--- a/src/com/android/phone/TelephonyShellCommand.java
+++ b/src/com/android/phone/TelephonyShellCommand.java
@@ -64,8 +64,9 @@
private static final String DATA_ENABLE = "enable";
private static final String DATA_DISABLE = "disable";
- private static final String IMS_SET_CARRIER_SERVICE = "set-ims-service";
- private static final String IMS_GET_CARRIER_SERVICE = "get-ims-service";
+ private static final String IMS_SET_IMS_SERVICE = "set-ims-service";
+ private static final String IMS_GET_IMS_SERVICE = "get-ims-service";
+ private static final String IMS_CLEAR_SERVICE_OVERRIDE = "clear-ims-service-override";
private static final String IMS_ENABLE = "enable";
private static final String IMS_DISABLE = "disable";
// Used to disable or enable processing of conference event package data from the network.
@@ -210,6 +211,11 @@
pw.println(" -d: The ImsService defined as the device default ImsService.");
pw.println(" -f: The feature type that the query will be requested for. If none is");
pw.println(" specified, the returned package name will correspond to MMTEL.");
+ pw.println(" ims clear-ims-service-override [-s SLOT_ID]");
+ pw.println(" Clear all carrier ImsService overrides. This does not work for device ");
+ pw.println(" configuration overrides. Options are:");
+ pw.println(" -s: The SIM slot ID for the registered ImsService. If no option");
+ pw.println(" is specified, it will choose the default voice SIM slot.");
pw.println(" ims enable [-s SLOT_ID]");
pw.println(" enables IMS for the SIM slot specified, or for the default voice SIM slot");
pw.println(" if none is specified.");
@@ -295,12 +301,15 @@
}
switch (arg) {
- case IMS_SET_CARRIER_SERVICE: {
+ case IMS_SET_IMS_SERVICE: {
return handleImsSetServiceCommand();
}
- case IMS_GET_CARRIER_SERVICE: {
+ case IMS_GET_IMS_SERVICE: {
return handleImsGetServiceCommand();
}
+ case IMS_CLEAR_SERVICE_OVERRIDE: {
+ return handleImsClearCarrierServiceCommand();
+ }
case IMS_ENABLE: {
return handleEnableIms();
}
@@ -547,6 +556,42 @@
return 0;
}
+ // ims clear-ims-service-override
+ private int handleImsClearCarrierServiceCommand() {
+ PrintWriter errPw = getErrPrintWriter();
+ int slotId = getDefaultSlot();
+
+ String opt;
+ while ((opt = getNextOption()) != null) {
+ switch (opt) {
+ case "-s": {
+ try {
+ slotId = Integer.parseInt(getNextArgRequired());
+ } catch (NumberFormatException e) {
+ errPw.println("ims set-ims-service requires an integer as a SLOT_ID.");
+ return -1;
+ }
+ break;
+ }
+ }
+ }
+
+ try {
+ boolean result = mInterface.clearCarrierImsServiceOverride(slotId);
+ if (VDBG) {
+ Log.v(LOG_TAG, "ims clear-ims-service-override -s " + slotId
+ + ", result=" + result);
+ }
+ getOutPrintWriter().println(result);
+ } catch (RemoteException e) {
+ Log.w(LOG_TAG, "ims clear-ims-service-override -s " + slotId
+ + ", error" + e.getMessage());
+ errPw.println("Exception: " + e.getMessage());
+ return -1;
+ }
+ return 0;
+ }
+
// ims get-ims-service
private int handleImsGetServiceCommand() {
PrintWriter errPw = getErrPrintWriter();
diff --git a/src/com/android/services/telephony/rcs/DelegateStateTracker.java b/src/com/android/services/telephony/rcs/DelegateStateTracker.java
index e287813..1d8fa3b 100644
--- a/src/com/android/services/telephony/rcs/DelegateStateTracker.java
+++ b/src/com/android/services/telephony/rcs/DelegateStateTracker.java
@@ -126,14 +126,18 @@
*/
@Override
public void onRegistrationStateChanged(DelegateRegistrationState registrationState) {
- mLastRegState = registrationState;
if (mRegistrationStateOverride > DelegateRegistrationState.DEREGISTERED_REASON_UNKNOWN) {
logi("onRegistrationStateChanged: overriding registered state to "
+ mRegistrationStateOverride);
registrationState = overrideRegistrationForDelegateChange(mRegistrationStateOverride,
registrationState);
}
- logi("onRegistrationStateChanged: sending reg state" + registrationState);
+ if (registrationState.equals(mLastRegState)) {
+ logi("onRegistrationStateChanged: skipping notification, state is the same.");
+ return;
+ }
+ mLastRegState = registrationState;
+ logi("onRegistrationStateChanged: sending reg state " + registrationState);
try {
mAppStateCallback.onFeatureTagStatusChanged(registrationState, mDelegateDeniedTags);
} catch (RemoteException e) {
@@ -168,10 +172,6 @@
int registerOverrideReason, DelegateRegistrationState state) {
Set<String> registeredFeatures = state.getRegisteredFeatureTags();
DelegateRegistrationState.Builder overriddenState = new DelegateRegistrationState.Builder();
- // Override REGISTERED only
- for (String ft : registeredFeatures) {
- overriddenState.addDeregisteringFeatureTag(ft, registerOverrideReason);
- }
// keep other deregistering/deregistered tags the same.
for (FeatureTagState dereging : state.getDeregisteringFeatureTags()) {
overriddenState.addDeregisteringFeatureTag(dereging.getFeatureTag(),
@@ -181,6 +181,10 @@
overriddenState.addDeregisteredFeatureTag(dereged.getFeatureTag(),
dereged.getState());
}
+ // Override REGISTERED only
+ for (String ft : registeredFeatures) {
+ overriddenState.addDeregisteringFeatureTag(ft, registerOverrideReason);
+ }
return overriddenState.build();
}
diff --git a/src/com/android/services/telephony/rcs/MessageTransportStateTracker.java b/src/com/android/services/telephony/rcs/MessageTransportStateTracker.java
index 531ed7d..0691ae5 100644
--- a/src/com/android/services/telephony/rcs/MessageTransportStateTracker.java
+++ b/src/com/android/services/telephony/rcs/MessageTransportStateTracker.java
@@ -159,7 +159,7 @@
* and sent over the network.
*/
@Override
- public void sendMessage(SipMessage sipMessage, int configVersion) {
+ public void sendMessage(SipMessage sipMessage, long configVersion) {
long token = Binder.clearCallingIdentity();
try {
mExecutor.execute(() -> {
diff --git a/src/com/android/services/telephony/rcs/SipDelegateBinderConnection.java b/src/com/android/services/telephony/rcs/SipDelegateBinderConnection.java
index 8ae1936..1a77f2b 100644
--- a/src/com/android/services/telephony/rcs/SipDelegateBinderConnection.java
+++ b/src/com/android/services/telephony/rcs/SipDelegateBinderConnection.java
@@ -17,11 +17,13 @@
package com.android.services.telephony.rcs;
import android.os.Binder;
+import android.os.IBinder;
import android.os.RemoteException;
import android.telephony.ims.DelegateRegistrationState;
import android.telephony.ims.DelegateRequest;
import android.telephony.ims.FeatureTagState;
import android.telephony.ims.SipDelegateImsConfiguration;
+import android.telephony.ims.SipDelegateManager;
import android.telephony.ims.aidl.ISipDelegate;
import android.telephony.ims.aidl.ISipDelegateMessageCallback;
import android.telephony.ims.aidl.ISipDelegateStateCallback;
@@ -31,7 +33,9 @@
import android.util.Log;
import java.io.PrintWriter;
+import java.util.Collections;
import java.util.List;
+import java.util.NoSuchElementException;
import java.util.Set;
import java.util.concurrent.Executor;
import java.util.function.BiConsumer;
@@ -43,7 +47,8 @@
* New instances of this class will be created and destroyed new {@link SipDelegate}s are created
* and destroyed by the {@link SipDelegateController}.
*/
-public class SipDelegateBinderConnection implements DelegateBinderStateManager {
+public class SipDelegateBinderConnection implements DelegateBinderStateManager,
+ IBinder.DeathRecipient {
private static final String LOG_TAG = "BinderConn";
protected final int mSubId;
@@ -149,6 +154,7 @@
try {
mSipTransport.createSipDelegate(mSubId, mRequestedConfig, mSipDelegateStateCallback,
cb);
+ mSipTransport.asBinder().linkToDeath(this, 0);
} catch (RemoteException e) {
logw("create called on unreachable SipTransport:" + e);
return false;
@@ -171,6 +177,11 @@
logw("destroy called on unreachable SipTransport:" + e);
mExecutor.execute(() -> notifySipDelegateDestroyed(reason));
}
+ try {
+ mSipTransport.asBinder().unlinkToDeath(this, 0);
+ } catch (NoSuchElementException e) {
+ logw("unlinkToDeath called on already unlinked binder" + e);
+ }
}
private void notifySipDelegateCreated(ISipDelegate delegate,
@@ -211,4 +222,15 @@
Log.w(SipTransportController.LOG_TAG, LOG_TAG + "[" + mSubId + "] " + log);
mLocalLog.log("[W] " + log);
}
+
+ @Override
+ public void binderDied() {
+ mExecutor.execute(() -> {
+ logw("binderDied!");
+ // Unblock any pending create/destroy operations.
+ // SipTransportController will handle the overall destruction/teardown.
+ notifySipDelegateCreated(null, Collections.emptyList());
+ notifySipDelegateDestroyed(SipDelegateManager.SIP_DELEGATE_DESTROY_REASON_SERVICE_DEAD);
+ });
+ }
}
diff --git a/src/com/android/services/telephony/rcs/SipDelegateController.java b/src/com/android/services/telephony/rcs/SipDelegateController.java
index ee9d4a7..ed50778 100644
--- a/src/com/android/services/telephony/rcs/SipDelegateController.java
+++ b/src/com/android/services/telephony/rcs/SipDelegateController.java
@@ -27,7 +27,6 @@
import android.telephony.ims.aidl.ISipTransport;
import android.telephony.ims.stub.DelegateConnectionStateCallback;
import android.telephony.ims.stub.SipDelegate;
-import android.util.ArraySet;
import android.util.LocalLog;
import android.util.Log;
import android.util.Pair;
@@ -76,7 +75,7 @@
private final LocalLog mLocalLog = new LocalLog(SipTransportController.LOG_SIZE);
private DelegateBinderStateManager mBinderConnection;
- private Set<String> mTrackedFeatureTags = new ArraySet<>();
+ private Set<String> mTrackedFeatureTags;
public SipDelegateController(int subId, DelegateRequest initialRequest, String packageName,
ISipTransport sipTransportImpl, ScheduledExecutorService executorService,
@@ -154,7 +153,11 @@
// May need to implement special case handling where SipDelegate denies all in supportedSet,
// however that should be a very rare case. For now, if that happens, just keep the
// SipDelegate bound.
- return pendingCreate.thenApplyAsync((resultPair) -> {
+ // use thenApply here because we need this to happen on the same thread that it was called
+ // on in order to ensure ordering of onCreated being called, followed by registration
+ // state changed. If not, this is subject to race conditions where registered is queued
+ // before the async processing of this future.
+ return pendingCreate.thenApply((resultPair) -> {
if (resultPair == null) {
logw("create: resultPair returned null");
return false;
@@ -164,7 +167,7 @@
mMessageTransportStateTracker.openTransport(resultPair.first, resultPair.second);
mDelegateStateTracker.sipDelegateConnected(resultPair.second);
return true;
- }, mExecutorService);
+ });
}
/**
@@ -196,7 +199,7 @@
Set<FeatureTagState> deniedSet) {
logi("Received feature tag set change, old: [" + mTrackedFeatureTags + "], new: "
+ newSupportedSet + ",denied: [" + deniedSet + "]");
- if (mTrackedFeatureTags.equals(newSupportedSet)) {
+ if (mTrackedFeatureTags != null && mTrackedFeatureTags.equals(newSupportedSet)) {
logi("changeSupportedFeatureTags: no change, returning");
return CompletableFuture.completedFuture(true);
}
diff --git a/src/com/android/services/telephony/rcs/SipTransportController.java b/src/com/android/services/telephony/rcs/SipTransportController.java
index dd06cc1..5d817ba 100644
--- a/src/com/android/services/telephony/rcs/SipTransportController.java
+++ b/src/com/android/services/telephony/rcs/SipTransportController.java
@@ -673,12 +673,18 @@
private void updateRoleCache() {
- // Only one app can fulfill the SMS role.
- String newSmsRolePackageName = mRoleManagerAdapter.getRoleHolders(RoleManager.ROLE_SMS)
- .stream().findFirst().orElse("");
+ String newSmsRolePackageName = "";
+ try {
+ // Only one app can fulfill the SMS role.
+ newSmsRolePackageName = mRoleManagerAdapter.getRoleHolders(RoleManager.ROLE_SMS)
+ .stream().findFirst().orElse("");
+ } catch (Exception e) {
+ logi("updateRoleCache: exception=" + e);
+ }
+ logi("updateRoleCache: new packageName=" + newSmsRolePackageName);
if (TextUtils.equals(mCachedSmsRolePackageName, newSmsRolePackageName)) {
- logi("onRoleHoldersChanged, skipping, role did not change");
+ logi("updateRoleCache, skipping, role did not change");
return;
}
mCachedSmsRolePackageName = newSmsRolePackageName;
@@ -793,19 +799,24 @@
unregisterListeners();
scheduleDestroyDelegates(SipDelegateManager.SIP_DELEGATE_DESTROY_REASON_SERVICE_DEAD);
} else {
+ logi("onRcsManagerChanged: registering listeners/updating role cache...");
registerListeners();
updateRoleCache();
}
}
private void registerListeners() {
- mRoleManagerAdapter.addOnRoleHoldersChangedListenerAsUser(mExecutorService, this,
- UserHandle.SYSTEM);
+ try {
+ mRoleManagerAdapter.addOnRoleHoldersChangedListenerAsUser(mExecutorService, this,
+ UserHandle.SYSTEM);
+ } catch (Exception e) {
+ logi("registerListeners: exception=" + e);
+ }
}
private void unregisterListeners() {
- mRoleManagerAdapter.removeOnRoleHoldersChangedListenerAsUser(this, UserHandle.SYSTEM);
mCachedSmsRolePackageName = "";
+ mRoleManagerAdapter.removeOnRoleHoldersChangedListenerAsUser(this, UserHandle.SYSTEM);
}
/**