commit bb7f284dcde6c1739a2f23a00636beefe8ec5153
author Yan Yan <evitayan@google.com> Wed Feb 02 11:23:32 2022 -0800
committer Yan Yan <evitayan@google.com> Fri Mar 25 16:30:50 2022 -0700
tree 58cbf82995a94acef5bb7b0f0c217d8bc7046f8b
parent a981da90b3af36b4a3be1c982cb0772d5fbc00e2

Fire onIpSecTransformDeleted before onClosed

This commit fixes the IKE library implementation to fire
onIpSecTransformDeleted before onClosed or onClosedWithException
so that the implementation can be aligned with the API doc
of ChildSessionCallback. This commit allows callers to monitor
the "close" callback of Child Session to know the associated
IPsec transforms are closed.

Bug: 159252747
Test: atest FrameworksIkeTests(new tests), CtsIkeTestCases
Change-Id: I0759d1d8424a40e04e8f8e1d91ee9f0548d7ffd1

src/java/com/android/internal/net/ipsec/ike/ChildSessionStateMachine.java

diff --git a/src/java/com/android/internal/net/ipsec/ike/ChildSessionStateMachine.java b/src/java/com/android/internal/net/ipsec/ike/ChildSessionStateMachine.java
index 9153ceb..f36aaed 100644
--- a/src/java/com/android/internal/net/ipsec/ike/ChildSessionStateMachine.java
+++ b/src/java/com/android/internal/net/ipsec/ike/ChildSessionStateMachine.java
@@ -738,14 +738,14 @@
 
     private void handleChildFatalError(Exception error) {
         IkeException ikeException = wrapAsIkeException(error);
-        executeUserCallback(
-                () -> {
-                    mUserCallback.onClosedWithException(ikeException);
-                });
         loge("Child Session fatal error", ikeException);
 
         // Clean up all SaRecords and quit
         closeAllSaRecords(false /*expectSaClosed*/);
+        executeUserCallback(
+                () -> {
+                    mUserCallback.onClosedWithException(ikeException);
+                });
         quitSessionNow();
     }
 
@@ -762,13 +762,11 @@
         public boolean processStateMessage(Message message) {
             switch (message.what) {
                 case CMD_KILL_SESSION:
+                    closeAllSaRecords(false /*expectSaClosed*/);
                     executeUserCallback(
                             () -> {
                                 mUserCallback.onClosed();
                             });
-
-                    closeAllSaRecords(false /*expectSaClosed*/);
-
                     quitSessionNow();
                     return HANDLED;
                 default:
@@ -1263,8 +1261,8 @@
                     new OnIpSecSaPairDeletedRunnable(mCurrentChildSaRecord);
             executeUserCallback(
                     () -> {
-                        mUserCallback.onClosed();
                         delRunnable.run();
+                        mUserCallback.onClosed();
                     });
 
             mChildSmCallback.onChildSaDeleted(mCurrentChildSaRecord.getRemoteSpi());

tests/iketests/src/java/com/android/internal/net/ipsec/ike/ChildSessionStateMachineTest.java

diff --git a/tests/iketests/src/java/com/android/internal/net/ipsec/ike/ChildSessionStateMachineTest.java b/tests/iketests/src/java/com/android/internal/net/ipsec/ike/ChildSessionStateMachineTest.java
index 69aeac3..f02ee45 100644
--- a/tests/iketests/src/java/com/android/internal/net/ipsec/ike/ChildSessionStateMachineTest.java
+++ b/tests/iketests/src/java/com/android/internal/net/ipsec/ike/ChildSessionStateMachineTest.java
@@ -59,8 +59,10 @@
 import static org.mockito.Matchers.anyString;
 import static org.mockito.Matchers.argThat;
 import static org.mockito.Matchers.eq;
+import static org.mockito.Mockito.atLeastOnce;
 import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doReturn;
+import static org.mockito.Mockito.inOrder;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.reset;
@@ -134,6 +136,7 @@
 import org.junit.Ignore;
 import org.junit.Test;
 import org.mockito.ArgumentCaptor;
+import org.mockito.InOrder;
 
 import java.net.Inet4Address;
 import java.net.InetAddress;
@@ -831,9 +834,26 @@
     }
 
     private void verifyNotifyUsersDeleteSession(Executor spyExecutor) {
-        verify(spyExecutor).execute(any(Runnable.class));
-        verify(mMockChildSessionCallback).onClosed();
+        verifyNotifyUsersDeleteSession(spyExecutor, null);
+    }
+
+    private void verifyNotifyUsersDeleteSession(
+            Executor spyExecutor, Class<? extends IkeException> exceptionClass) {
+        verify(spyExecutor, atLeastOnce()).execute(any(Runnable.class));
         verifyNotifyUserDeleteChildSa(mSpyCurrentChildSaRecord);
+
+        InOrder orderVerifier = inOrder(mMockChildSessionCallback);
+        orderVerifier
+                .verify(mMockChildSessionCallback, times(2))
+                .onIpSecTransformDeleted(any(), anyInt());
+
+        if (exceptionClass == null) {
+            orderVerifier.verify(mMockChildSessionCallback).onClosed();
+        } else {
+            orderVerifier
+                    .verify(mMockChildSessionCallback)
+                    .onClosedWithException(any(exceptionClass));
+        }
     }
 
     @Test
@@ -1383,8 +1403,7 @@
         mLooper.dispatchAll();
 
         // Verify user was notified and state machine has quit.
-        verifyHandleFatalErrorAndQuit(InvalidSyntaxException.class);
-        verifyNotifyUserDeleteChildSa(mSpyCurrentChildSaRecord);
+        verifyNotifyUsersDeleteSession(mSpyUserCbExecutor, InvalidSyntaxException.class);
 
         // Verify no SPI for provisional Child was registered.
         verify(mMockChildSessionSmCallback, never())
@@ -1425,8 +1444,7 @@
         mLooper.dispatchAll();
 
         // Verify user was notified and state machine has quit.
-        verifyHandleFatalErrorAndQuit(IkeInternalException.class);
-        verifyNotifyUserDeleteChildSa(mSpyCurrentChildSaRecord);
+        verifyNotifyUsersDeleteSession(mSpyUserCbExecutor, IkeInternalException.class);
 
         // Verify SPI for provisional Child was registered and unregistered.
         verify(mMockChildSessionSmCallback)