disconnect ongoing unholdable call for transactional calls
Previously, if an ongoing unholdable call was active and a new
incoming transactional call came in, the notifications answer
button would actually disconnect the transactional call in favor of
keeping the active unholdable call.
Now, CallControlCallback requests for transactional calls can disconnect
the ongoing unholdable calls. This behavior makes things more consistent
with the legacy ConnectionService way.
Fixes: 340621152
Flag: com.android.server.telecom.flags.transactional_hold_disconnects_unholdable
Test: 7 unit tests + manual testing
(1) start unholdable call
(2) create incoming transactional call
(3) answer via notification
observe/expect: unholdable call disconnects &
transactional call becomes active
Change-Id: Id941d7f34454ca31dddf67c121762ee392c790e8
diff --git a/flags/telecom_calls_manager_flags.aconfig b/flags/telecom_calls_manager_flags.aconfig
index 28e9dd8..f46e844 100644
--- a/flags/telecom_calls_manager_flags.aconfig
+++ b/flags/telecom_calls_manager_flags.aconfig
@@ -24,3 +24,14 @@
description: "Enables simultaneous call sequencing for SIM PhoneAccounts"
bug: "327038818"
}
+
+# OWNER=tjstuart TARGET=24Q4
+flag {
+ name: "transactional_hold_disconnects_unholdable"
+ namespace: "telecom"
+ description: "Disconnect ongoing unholdable calls for CallControlCallbacks"
+ bug: "340621152"
+ metadata {
+ purpose: PURPOSE_BUGFIX
+ }
+}
diff --git a/src/com/android/server/telecom/CallsManager.java b/src/com/android/server/telecom/CallsManager.java
index feb9ee6..600f847 100644
--- a/src/com/android/server/telecom/CallsManager.java
+++ b/src/com/android/server/telecom/CallsManager.java
@@ -3836,8 +3836,7 @@
if (canHold(activeCall)) {
activeCall.hold("swap to " + call.getId());
return true;
- } else if (supportsHold(activeCall)
- && areFromSameSource(activeCall, call)) {
+ } else if (sameSourceHoldCase(activeCall, call)) {
// Handle the case where the active call and the new call are from the same CS or
// connection manager, and the currently active call supports hold but cannot
@@ -3886,43 +3885,85 @@
return false;
}
- // attempt to hold the requested call and complete the callback on the result
+ /**
+ * attempt to hold or swap the current active call in favor of a new call request. The
+ * OutcomeReceiver will return onResult if the current active call is held or disconnected.
+ * Otherwise, the OutcomeReceiver will fail.
+ */
public void transactionHoldPotentialActiveCallForNewCall(Call newCall,
- OutcomeReceiver<Boolean, CallException> callback) {
+ boolean isCallControlRequest, OutcomeReceiver<Boolean, CallException> callback) {
+ String mTag = "transactionHoldPotentialActiveCallForNewCall: ";
Call activeCall = (Call) mConnectionSvrFocusMgr.getCurrentFocusCall();
- Log.i(this, "transactionHoldPotentialActiveCallForNewCall: "
- + "newCall=[%s], activeCall=[%s]", newCall, activeCall);
+ Log.i(this, mTag + "newCall=[%s], activeCall=[%s]", newCall, activeCall);
- // early exit if there is no need to hold an active call
if (activeCall == null || activeCall == newCall) {
- Log.i(this, "transactionHoldPotentialActiveCallForNewCall:"
- + " no need to hold activeCall");
+ Log.i(this, mTag + "no need to hold activeCall");
callback.onResult(true);
return;
}
- // before attempting CallsManager#holdActiveCallForNewCall(Call), check if it'll fail early
- if (!canHold(activeCall) &&
- !(supportsHold(activeCall) && areFromSameSource(activeCall, newCall))) {
- Log.i(this, "transactionHoldPotentialActiveCallForNewCall: "
- + "conditions show the call cannot be held.");
- callback.onError(new CallException("call does not support hold",
- CallException.CODE_CANNOT_HOLD_CURRENT_ACTIVE_CALL));
- return;
- }
+ if (mFeatureFlags.transactionalHoldDisconnectsUnholdable()) {
+ // prevent bad actors from disconnecting the activeCall. Instead, clients will need to
+ // notify the user that they need to disconnect the ongoing call before making the
+ // new call ACTIVE.
+ if (isCallControlRequest && !canHoldOrSwapActiveCall(activeCall, newCall)) {
+ Log.i(this, mTag + "CallControlRequest exit");
+ callback.onError(new CallException("activeCall is NOT holdable or swappable, please"
+ + " request the user disconnect the call.",
+ CallException.CODE_CANNOT_HOLD_CURRENT_ACTIVE_CALL));
+ return;
+ }
- // attempt to hold the active call
- if (!holdActiveCallForNewCall(newCall)) {
- Log.i(this, "transactionHoldPotentialActiveCallForNewCall: "
- + "attempted to hold call but failed.");
- callback.onError(new CallException("cannot hold active call failed",
- CallException.CODE_CANNOT_HOLD_CURRENT_ACTIVE_CALL));
- return;
- }
+ if (holdActiveCallForNewCall(newCall)) {
+ // Transactional clients do not call setHold but the request was sent to set the
+ // call as inactive and it has already been acked by this point.
+ markCallAsOnHold(activeCall);
+ callback.onResult(true);
+ } else {
+ // It's possible that holdActiveCallForNewCall disconnected the activeCall.
+ // Therefore, the activeCalls state should be checked before failing.
+ if (activeCall.isLocallyDisconnecting()) {
+ callback.onResult(true);
+ } else {
+ Log.i(this, mTag + "active call could not be held or disconnected");
+ callback.onError(
+ new CallException("activeCall could not be held or disconnected",
+ CallException.CODE_CANNOT_HOLD_CURRENT_ACTIVE_CALL));
+ }
+ }
+ } else {
+ // before attempting CallsManager#holdActiveCallForNewCall(Call), check if it'll fail
+ // early
+ if (!canHold(activeCall) &&
+ !(supportsHold(activeCall) && areFromSameSource(activeCall, newCall))) {
+ Log.i(this, "transactionHoldPotentialActiveCallForNewCall: "
+ + "conditions show the call cannot be held.");
+ callback.onError(new CallException("call does not support hold",
+ CallException.CODE_CANNOT_HOLD_CURRENT_ACTIVE_CALL));
+ return;
+ }
- // officially mark the activeCall as held
- markCallAsOnHold(activeCall);
- callback.onResult(true);
+ // attempt to hold the active call
+ if (!holdActiveCallForNewCall(newCall)) {
+ Log.i(this, "transactionHoldPotentialActiveCallForNewCall: "
+ + "attempted to hold call but failed.");
+ callback.onError(new CallException("cannot hold active call failed",
+ CallException.CODE_CANNOT_HOLD_CURRENT_ACTIVE_CALL));
+ return;
+ }
+
+ // officially mark the activeCall as held
+ markCallAsOnHold(activeCall);
+ callback.onResult(true);
+ }
+ }
+
+ private boolean canHoldOrSwapActiveCall(Call activeCall, Call newCall) {
+ return canHold(activeCall) || sameSourceHoldCase(activeCall, newCall);
+ }
+
+ private boolean sameSourceHoldCase(Call activeCall, Call call) {
+ return supportsHold(activeCall) && areFromSameSource(activeCall, call);
}
@VisibleForTesting
diff --git a/src/com/android/server/telecom/TransactionalServiceWrapper.java b/src/com/android/server/telecom/TransactionalServiceWrapper.java
index fe4b1b7..50ef2e8 100644
--- a/src/com/android/server/telecom/TransactionalServiceWrapper.java
+++ b/src/com/android/server/telecom/TransactionalServiceWrapper.java
@@ -325,7 +325,8 @@
// This request is originating from the VoIP application.
private void handleCallControlNewCallFocusTransactions(Call call, String action,
boolean isAnswer, int potentiallyNewVideoState, ResultReceiver callback) {
- mTransactionManager.addTransaction(createSetActiveTransactions(call),
+ mTransactionManager.addTransaction(
+ createSetActiveTransactions(call, true /* isCallControlRequest */),
new OutcomeReceiver<>() {
@Override
public void onResult(VoipCallTransactionResult result) {
@@ -445,7 +446,8 @@
Call foregroundCallBeforeSwap = mCallsManager.getForegroundCall();
boolean wasActive = foregroundCallBeforeSwap != null && foregroundCallBeforeSwap.isActive();
- SerialTransaction serialTransactions = createSetActiveTransactions(call);
+ SerialTransaction serialTransactions = createSetActiveTransactions(call,
+ false /* isCallControlRequest */);
// 3. get ack from client (that the requested call can go active)
if (isAnswerRequest) {
serialTransactions.appendTransaction(
@@ -654,12 +656,13 @@
mCallsManager.removeCall(call);
}
- private SerialTransaction createSetActiveTransactions(Call call) {
+ private SerialTransaction createSetActiveTransactions(Call call, boolean isCallControlRequest) {
// create list for multiple transactions
List<VoipCallTransaction> transactions = new ArrayList<>();
// potentially hold the current active call in order to set a new call (active/answered)
- transactions.add(new MaybeHoldCallForNewCallTransaction(mCallsManager, call));
+ transactions.add(
+ new MaybeHoldCallForNewCallTransaction(mCallsManager, call, isCallControlRequest));
// And request a new focus call update
transactions.add(new RequestNewActiveCallTransaction(mCallsManager, call));
diff --git a/src/com/android/server/telecom/voip/MaybeHoldCallForNewCallTransaction.java b/src/com/android/server/telecom/voip/MaybeHoldCallForNewCallTransaction.java
index a245c1c..3bed088 100644
--- a/src/com/android/server/telecom/voip/MaybeHoldCallForNewCallTransaction.java
+++ b/src/com/android/server/telecom/voip/MaybeHoldCallForNewCallTransaction.java
@@ -26,16 +26,23 @@
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionStage;
+/**
+ * This VoipCallTransaction is responsible for holding any active call in favor of a new call
+ * request. If the active call cannot be held or disconnected, the transaction will fail.
+ */
public class MaybeHoldCallForNewCallTransaction extends VoipCallTransaction {
private static final String TAG = MaybeHoldCallForNewCallTransaction.class.getSimpleName();
private final CallsManager mCallsManager;
private final Call mCall;
+ private final boolean mIsCallControlRequest;
- public MaybeHoldCallForNewCallTransaction(CallsManager callsManager, Call call) {
+ public MaybeHoldCallForNewCallTransaction(CallsManager callsManager, Call call,
+ boolean isCallControlRequest) {
super(callsManager.getLock());
mCallsManager = callsManager;
mCall = call;
+ mIsCallControlRequest = isCallControlRequest;
}
@Override
@@ -43,7 +50,8 @@
Log.d(TAG, "processTransaction");
CompletableFuture<VoipCallTransactionResult> future = new CompletableFuture<>();
- mCallsManager.transactionHoldPotentialActiveCallForNewCall(mCall, new OutcomeReceiver<>() {
+ mCallsManager.transactionHoldPotentialActiveCallForNewCall(mCall, mIsCallControlRequest,
+ new OutcomeReceiver<>() {
@Override
public void onResult(Boolean result) {
Log.d(TAG, "processTransaction: onResult");
diff --git a/src/com/android/server/telecom/voip/RequestNewActiveCallTransaction.java b/src/com/android/server/telecom/voip/RequestNewActiveCallTransaction.java
index f586cc3..e3aed8e 100644
--- a/src/com/android/server/telecom/voip/RequestNewActiveCallTransaction.java
+++ b/src/com/android/server/telecom/voip/RequestNewActiveCallTransaction.java
@@ -17,7 +17,6 @@
package com.android.server.telecom.voip;
import android.os.OutcomeReceiver;
-import android.telecom.CallAttributes;
import android.telecom.CallException;
import android.util.Log;
@@ -25,6 +24,7 @@
import com.android.server.telecom.CallState;
import com.android.server.telecom.CallsManager;
import com.android.server.telecom.ConnectionServiceFocusManager;
+import com.android.server.telecom.flags.Flags;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionStage;
@@ -69,7 +69,8 @@
return future;
}
- if (mCallsManager.getActiveCall() != null) {
+ if (!Flags.transactionalHoldDisconnectsUnholdable() &&
+ mCallsManager.getActiveCall() != null) {
future.complete(new VoipCallTransactionResult(
CallException.CODE_CALL_CANNOT_BE_SET_TO_ACTIVE,
"Already an active call. Request hold on current active call."));
diff --git a/tests/src/com/android/server/telecom/tests/CallsManagerTest.java b/tests/src/com/android/server/telecom/tests/CallsManagerTest.java
index 295fbff..ae5e6c1 100644
--- a/tests/src/com/android/server/telecom/tests/CallsManagerTest.java
+++ b/tests/src/com/android/server/telecom/tests/CallsManagerTest.java
@@ -33,6 +33,7 @@
import static org.mockito.ArgumentMatchers.anyString;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.ArgumentMatchers.nullable;
+import static org.mockito.Mockito.atLeast;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.doNothing;
import static org.mockito.Mockito.doReturn;
@@ -3037,42 +3038,152 @@
assertFalse(mCallsManager.getCalls().contains(call));
}
+ /**
+ * Verify that
+ * {@link CallsManager#transactionHoldPotentialActiveCallForNewCall(Call, boolean,
+ * OutcomeReceiver)}s OutcomeReceiver returns onResult when there is no active call to place
+ * on hold.
+ */
@MediumTest
@Test
- public void testHoldTransactional() throws Exception {
- CountDownLatch latch = new CountDownLatch(1);
+ public void testHoldWhenActiveCallIsNullOrSame() throws Exception {
Call newCall = addSpyCall();
-
// case 1: no active call, no need to put the call on hold
- when(mConnectionSvrFocusMgr.getCurrentFocusCall()).thenReturn(null);
- mCallsManager.transactionHoldPotentialActiveCallForNewCall(newCall,
- new LatchedOutcomeReceiver(latch, true));
- waitForCountDownLatch(latch);
-
+ assertHoldActiveCallForNewCall(
+ newCall,
+ null /* activeCall */,
+ false /* isCallControlRequest */,
+ true /* expectOnResult */);
// case 2: active call == new call, no need to put the call on hold
- latch = new CountDownLatch(1);
- when(mConnectionSvrFocusMgr.getCurrentFocusCall()).thenReturn(newCall);
- mCallsManager.transactionHoldPotentialActiveCallForNewCall(newCall,
- new LatchedOutcomeReceiver(latch, true));
- waitForCountDownLatch(latch);
+ assertHoldActiveCallForNewCall(
+ newCall,
+ newCall /* activeCall */,
+ false /* isCallControlRequest */,
+ true /* expectOnResult */);
+ }
- // case 3: cannot hold current active call early check
+ /**
+ * Verify that
+ * {@link CallsManager#transactionHoldPotentialActiveCallForNewCall(Call, boolean,
+ * OutcomeReceiver)}s OutcomeReceiver returns onError when there is an active call that
+ * cannot be held, and it's a CallControlRequest.
+ */
+ @MediumTest
+ @Test
+ public void testHoldFailsWithUnholdableCallAndCallControlRequest() throws Exception {
Call cannotHoldCall = addSpyCall(SIM_1_HANDLE, null,
CallState.ACTIVE, 0, 0);
- latch = new CountDownLatch(1);
- when(mConnectionSvrFocusMgr.getCurrentFocusCall()).thenReturn(cannotHoldCall);
- mCallsManager.transactionHoldPotentialActiveCallForNewCall(newCall,
- new LatchedOutcomeReceiver(latch, false));
- waitForCountDownLatch(latch);
+ assertHoldActiveCallForNewCall(
+ addSpyCall(),
+ cannotHoldCall /* activeCall */,
+ true /* isCallControlRequest */,
+ false /* expectOnResult */);
+ }
- // case 4: activeCall != newCall && canHold(activeCall)
+ /**
+ * Verify that
+ * {@link CallsManager#transactionHoldPotentialActiveCallForNewCall(Call, boolean,
+ * OutcomeReceiver)}s OutcomeReceiver returns onResult when there is a holdable call and
+ * it's a CallControlRequest.
+ */
+ @MediumTest
+ @Test
+ public void testHoldSuccessWithHoldableActiveCall() throws Exception {
+ Call newCall = addSpyCall(VOIP_1_HANDLE, CallState.CONNECTING);
Call canHoldCall = addSpyCall(SIM_1_HANDLE, null,
CallState.ACTIVE, Connection.CAPABILITY_HOLD, 0);
- latch = new CountDownLatch(1);
- when(mConnectionSvrFocusMgr.getCurrentFocusCall()).thenReturn(canHoldCall);
- mCallsManager.transactionHoldPotentialActiveCallForNewCall(newCall,
- new LatchedOutcomeReceiver(latch, true));
- waitForCountDownLatch(latch);
+ assertHoldActiveCallForNewCall(
+ newCall,
+ canHoldCall /* activeCall */,
+ true /* isCallControlRequest */,
+ true /* expectOnResult */);
+ }
+
+ /**
+ * Verify that
+ * {@link CallsManager#transactionHoldPotentialActiveCallForNewCall(Call, boolean,
+ * OutcomeReceiver)}s OutcomeReceiver returns onResult when there is an active call that
+ * supports hold, and it's a CallControlRequest.
+ */
+ @MediumTest
+ @Test
+ public void testHoldWhenTheActiveCallSupportsHold() throws Exception {
+ Call newCall = addSpyCall();
+ Call supportsHold = addSpyCall(SIM_1_HANDLE, null,
+ CallState.ACTIVE, Connection.CAPABILITY_SUPPORT_HOLD, 0);
+ assertHoldActiveCallForNewCall(
+ newCall,
+ supportsHold /* activeCall */,
+ true /* isCallControlRequest */,
+ true /* expectOnResult */);
+ }
+
+ /**
+ * Verify that
+ * {@link CallsManager#transactionHoldPotentialActiveCallForNewCall(Call, boolean,
+ * OutcomeReceiver)}s OutcomeReceiver returns onResult when there is an active call that
+ * supports hold + can hold, and it's a CallControlRequest.
+ */
+ @MediumTest
+ @Test
+ public void testHoldWhenTheActiveCallSupportsAndCanHold() throws Exception {
+ Call newCall = addSpyCall();
+ Call supportsHold = addSpyCall(SIM_1_HANDLE, null,
+ CallState.ACTIVE,
+ Connection.CAPABILITY_HOLD | Connection.CAPABILITY_SUPPORT_HOLD,
+ 0);
+ assertHoldActiveCallForNewCall(
+ newCall,
+ supportsHold /* activeCall */,
+ true /* isCallControlRequest */,
+ true /* expectOnResult */);
+ }
+
+ /**
+ * Verify that
+ * {@link CallsManager#transactionHoldPotentialActiveCallForNewCall(Call, boolean,
+ * OutcomeReceiver)}s OutcomeReceiver returns onResult when there is an active call that
+ * supports hold + can hold, and it's a CallControlCallbackRequest.
+ */
+ @MediumTest
+ @Test
+ public void testHoldForCallControlCallbackRequestWithActiveCallThatCanHold() throws Exception {
+ Call newCall = addSpyCall();
+ Call supportsHold = addSpyCall(SIM_1_HANDLE, null,
+ CallState.ACTIVE, Connection.CAPABILITY_HOLD | Connection.CAPABILITY_SUPPORT_HOLD,
+ 0);
+ assertHoldActiveCallForNewCall(
+ newCall,
+ supportsHold /* activeCall */,
+ false /* isCallControlRequest */,
+ true /* expectOnResult */);
+ }
+
+ /**
+ * Verify that
+ * {@link CallsManager#transactionHoldPotentialActiveCallForNewCall(Call, boolean,
+ * OutcomeReceiver)}s OutcomeReceiver returns onResult when there is an active unholdable call,
+ * and it's a CallControlCallbackRequest.
+ */
+ @MediumTest
+ @Test
+ public void testHoldDisconnectsTheActiveCall() throws Exception {
+ Call newCall = addSpyCall(VOIP_1_HANDLE, CallState.CONNECTING);
+ Call activeUnholdableCall = addSpyCall(SIM_1_HANDLE, null,
+ CallState.ACTIVE, 0, 0);
+
+ doAnswer(invocation -> {
+ doReturn(true).when(activeUnholdableCall).isLocallyDisconnecting();
+ return null;
+ }).when(activeUnholdableCall).disconnect();
+
+ assertHoldActiveCallForNewCall(
+ newCall,
+ activeUnholdableCall /* activeCall */,
+ false /* isCallControlRequest */,
+ true /* expectOnResult */);
+
+ verify(activeUnholdableCall, atLeast(1)).disconnect();
}
@SmallTest
@@ -3816,6 +3927,22 @@
when(mPhoneCapability.getMaxActiveVoiceSubscriptions()).thenReturn(num);
}
+ private void assertHoldActiveCallForNewCall(
+ Call newCall,
+ Call activeCall,
+ boolean isCallControlRequest,
+ boolean expectOnResult)
+ throws InterruptedException {
+ CountDownLatch latch = new CountDownLatch(1);
+ when(mFeatureFlags.transactionalHoldDisconnectsUnholdable()).thenReturn(true);
+ when(mConnectionSvrFocusMgr.getCurrentFocusCall()).thenReturn(activeCall);
+ mCallsManager.transactionHoldPotentialActiveCallForNewCall(
+ newCall,
+ isCallControlRequest,
+ new LatchedOutcomeReceiver(latch, expectOnResult));
+ waitForCountDownLatch(latch);
+ }
+
private void waitUntilConditionIsTrueOrTimeout(Condition condition, long timeout,
String description) throws InterruptedException {
final long start = System.currentTimeMillis();
diff --git a/tests/src/com/android/server/telecom/tests/TransactionTests.java b/tests/src/com/android/server/telecom/tests/TransactionTests.java
index 707ed9f..5876474 100644
--- a/tests/src/com/android/server/telecom/tests/TransactionTests.java
+++ b/tests/src/com/android/server/telecom/tests/TransactionTests.java
@@ -217,14 +217,14 @@
public void testTransactionalHoldActiveCallForNewCall() throws Exception {
// GIVEN
MaybeHoldCallForNewCallTransaction transaction =
- new MaybeHoldCallForNewCallTransaction(mCallsManager, mMockCall1);
+ new MaybeHoldCallForNewCallTransaction(mCallsManager, mMockCall1, false);
// WHEN
transaction.processTransaction(null);
// THEN
verify(mCallsManager, times(1))
- .transactionHoldPotentialActiveCallForNewCall(eq(mMockCall1),
+ .transactionHoldPotentialActiveCallForNewCall(eq(mMockCall1), eq(false),
isA(OutcomeReceiver.class));
}