Clean up OpenLogicalChannelRecord on SIM removal
This CL cleans up the OpenLogicalChannelRecord when detecting
the UiccPort is disposed or finalized which may occor in scenarios
like SIM removal or modem reset.
In all those scenarios, the underneath logical channel tracked in the
record has been released. Without the clean-up, the client crash later may
trigger a LC closure while the LC has been re-assigned to other clients.
Bug: 335046531
Test: atest FrameworksTelephonyTests
Test: Manual test to reproduce and verify the issue sceanrios
Test: Basic functional test (activation, call, sms, data...)
Change-Id: I4c7b150e80c6509ee549785595394b2cebbd7197
diff --git a/flags/uicc.aconfig b/flags/uicc.aconfig
index 8166853..2d7b643 100644
--- a/flags/uicc.aconfig
+++ b/flags/uicc.aconfig
@@ -42,3 +42,11 @@
description: "This flag controls eSIM available memory feature."
bug:"318348580"
}
+
+# OWNER=rambowang TARGET=24Q3
+flag {
+ name: "cleanup_open_logical_channel_record_on_dispose"
+ namespace: "telephony"
+ description: "This flag cleans up the OpenLogicalChannelRecord once SIM is removed"
+ bug:"335046531"
+}
diff --git a/src/java/com/android/internal/telephony/uicc/UiccPort.java b/src/java/com/android/internal/telephony/uicc/UiccPort.java
index fd8f1c4..9e341ef 100644
--- a/src/java/com/android/internal/telephony/uicc/UiccPort.java
+++ b/src/java/com/android/internal/telephony/uicc/UiccPort.java
@@ -31,6 +31,7 @@
import com.android.internal.telephony.TelephonyComponentFactory;
import com.android.internal.telephony.flags.FeatureFlags;
import com.android.internal.telephony.flags.FeatureFlagsImpl;
+import com.android.internal.telephony.flags.Flags;
import com.android.telephony.Rlog;
import java.io.FileDescriptor;
@@ -57,8 +58,9 @@
private int mPortIdx;
private int mPhysicalSlotIndex;
- // The list of the opened logical channel record. The channels will be closed by us when
- // detecting client died without closing them in advance.
+ // The list of the opened logical channel record.
+ // The channels will be closed by us when detecting client died without closing them in advance.
+ // The same lock should be used to protect both access of the list and the individual record.
@GuardedBy("mOpenChannelRecords")
private final List<OpenLogicalChannelRecord> mOpenChannelRecords = new ArrayList<>();
@@ -101,11 +103,13 @@
}
mUiccProfile = null;
}
+ cleanupOpenLogicalChannelRecordsIfNeeded();
}
@Override
protected void finalize() {
if (DBG) log("UiccPort finalized");
+ cleanupOpenLogicalChannelRecordsIfNeeded();
}
/**
@@ -389,8 +393,10 @@
public void onLogicalChannelOpened(@NonNull IccLogicalChannelRequest request) {
OpenLogicalChannelRecord record = new OpenLogicalChannelRecord(request);
try {
- request.binder.linkToDeath(record, /*flags=*/ 0);
- addOpenLogicalChannelRecord(record);
+ synchronized (mOpenChannelRecords) {
+ request.binder.linkToDeath(record, /*flags=*/ 0);
+ mOpenChannelRecords.add(record);
+ }
if (DBG) log("onLogicalChannelOpened: monitoring client " + record);
} catch (RemoteException | NullPointerException ex) {
loge("IccOpenLogicChannel client has died, clean up manually");
@@ -406,11 +412,13 @@
*/
public void onLogicalChannelClosed(int channelId) {
OpenLogicalChannelRecord record = getOpenLogicalChannelRecord(channelId);
- if (record != null && record.mRequest != null && record.mRequest.binder != null) {
- if (DBG) log("onLogicalChannelClosed: stop monitoring client " + record);
- record.mRequest.binder.unlinkToDeath(record, /*flags=*/ 0);
- removeOpenLogicalChannelRecord(record);
- record.mRequest.binder = null;
+ synchronized (mOpenChannelRecords) {
+ if (record != null && record.mRequest != null && record.mRequest.binder != null) {
+ if (DBG) log("onLogicalChannelClosed: stop monitoring client " + record);
+ record.mRequest.binder.unlinkToDeath(record, /*flags=*/ 0);
+ record.mRequest.binder = null;
+ mOpenChannelRecords.remove(record);
+ }
}
}
@@ -428,15 +436,21 @@
return null;
}
- private void addOpenLogicalChannelRecord(OpenLogicalChannelRecord record) {
- synchronized (mOpenChannelRecords) {
- mOpenChannelRecords.add(record);
- }
- }
-
- private void removeOpenLogicalChannelRecord(OpenLogicalChannelRecord record) {
- synchronized (mOpenChannelRecords) {
- mOpenChannelRecords.remove(record);
+ /**
+ * Clean up records when logical channels underneath have been released, in cases like SIM
+ * removal or modem reset. The obsoleted records may trigger a redundant release of logical
+ * channel that may have been assigned to other client.
+ */
+ private void cleanupOpenLogicalChannelRecordsIfNeeded() {
+ if (Flags.cleanupOpenLogicalChannelRecordOnDispose()) {
+ synchronized (mOpenChannelRecords) {
+ for (OpenLogicalChannelRecord record : mOpenChannelRecords) {
+ if (DBG) log("Clean up " + record);
+ record.mRequest.binder.unlinkToDeath(record, /*flags=*/ 0);
+ record.mRequest.binder = null;
+ }
+ mOpenChannelRecords.clear();
+ }
}
}
diff --git a/tests/telephonytests/src/com/android/internal/telephony/uicc/UiccPortTest.java b/tests/telephonytests/src/com/android/internal/telephony/uicc/UiccPortTest.java
index 1a846c4..5c1993f 100644
--- a/tests/telephonytests/src/com/android/internal/telephony/uicc/UiccPortTest.java
+++ b/tests/telephonytests/src/com/android/internal/telephony/uicc/UiccPortTest.java
@@ -20,11 +20,16 @@
import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNull;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyBoolean;
+import static org.mockito.ArgumentMatchers.anyInt;
import static org.mockito.Matchers.eq;
import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
import static org.mockito.Mockito.verify;
import android.os.Binder;
+import android.platform.test.flag.junit.SetFlagsRule;
import android.telephony.TelephonyManager;
import android.testing.AndroidTestingRunner;
import android.testing.TestableLooper;
@@ -33,9 +38,11 @@
import com.android.internal.telephony.IccLogicalChannelRequest;
import com.android.internal.telephony.TelephonyTest;
+import com.android.internal.telephony.flags.Flags;
import org.junit.After;
import org.junit.Before;
+import org.junit.Rule;
import org.junit.Test;
import org.junit.runner.RunWith;
@@ -54,9 +61,13 @@
private int mPhoneId = 0;
+ @Rule
+ public final SetFlagsRule mSetFlagsRule = new SetFlagsRule();
+
@Before
public void setUp() throws Exception {
super.setUp(getClass().getSimpleName());
+ mSetFlagsRule.enableFlags(Flags.FLAG_CLEANUP_OPEN_LOGICAL_CHANNEL_RECORD_ON_DISPOSE);
mUiccCard = mock(UiccCard.class);
mIccCardStatus = mock(IccCardStatus.class);
/* initially there are no application available */
@@ -144,6 +155,20 @@
verify(mUiccProfile).iccCloseLogicalChannel(eq(CHANNEL_ID), eq(false), eq(null));
}
+ @Test
+ @SmallTest
+ public void testOnOpenLogicalChannel_withPortDisposed_noRecordLeft() {
+ IccLogicalChannelRequest request = getIccLogicalChannelRequest();
+
+ mUiccPort.onLogicalChannelOpened(request);
+ mUiccPort.dispose();
+
+ UiccPort.OpenLogicalChannelRecord record = mUiccPort.getOpenLogicalChannelRecord(
+ CHANNEL_ID);
+ assertThat(record).isNull();
+ verify(mUiccProfile, never()).iccCloseLogicalChannel(anyInt(), anyBoolean(), any());
+ }
+
private IccLogicalChannelRequest getIccLogicalChannelRequest() {
IccLogicalChannelRequest request = new IccLogicalChannelRequest();
request.channel = CHANNEL_ID;