diff --git a/service/src/com/android/server/ConnectivityService.java b/service/src/com/android/server/ConnectivityService.java index 85507f6d16..c2da84d324 100755 --- a/service/src/com/android/server/ConnectivityService.java +++ b/service/src/com/android/server/ConnectivityService.java @@ -11353,7 +11353,7 @@ public class ConnectivityService extends IConnectivityManager.Stub public void onInterfaceLinkStateChanged(@NonNull String iface, boolean up) { mHandler.post(() -> { for (NetworkAgentInfo nai : mNetworkAgentInfos) { - nai.clatd.interfaceLinkStateChanged(iface, up); + nai.clatd.handleInterfaceLinkStateChanged(iface, up); } }); } @@ -11362,7 +11362,7 @@ public class ConnectivityService extends IConnectivityManager.Stub public void onInterfaceRemoved(@NonNull String iface) { mHandler.post(() -> { for (NetworkAgentInfo nai : mNetworkAgentInfos) { - nai.clatd.interfaceRemoved(iface); + nai.clatd.handleInterfaceRemoved(iface); } }); } diff --git a/service/src/com/android/server/connectivity/Nat464Xlat.java b/service/src/com/android/server/connectivity/Nat464Xlat.java index f9e07fdbd3..065922dcd8 100644 --- a/service/src/com/android/server/connectivity/Nat464Xlat.java +++ b/service/src/com/android/server/connectivity/Nat464Xlat.java @@ -483,8 +483,9 @@ public class Nat464Xlat { /** * Adds stacked link on base link and transitions to RUNNING state. + * Must be called on the handler thread. */ - private void handleInterfaceLinkStateChanged(String iface, boolean up) { + public void handleInterfaceLinkStateChanged(String iface, boolean up) { // TODO: if we call start(), then stop(), then start() again, and the // interfaceLinkStateChanged notification for the first start is delayed past the first // stop, then the code becomes out of sync with system state and will behave incorrectly. @@ -499,6 +500,7 @@ public class Nat464Xlat { // Once this code is converted to StateMachine, it will be possible to use deferMessage to // ensure it stays in STARTING state until the interfaceLinkStateChanged notification fires, // and possibly use a timeout (or provide some guarantees at the lower layer) to address #1. + ensureRunningOnHandlerThread(); if (!isStarting() || !up || !Objects.equals(mIface, iface)) { return; } @@ -519,8 +521,10 @@ public class Nat464Xlat { /** * Removes stacked link on base link and transitions to IDLE state. + * Must be called on the handler thread. */ - private void handleInterfaceRemoved(String iface) { + public void handleInterfaceRemoved(String iface) { + ensureRunningOnHandlerThread(); if (!Objects.equals(mIface, iface)) { return; } @@ -536,14 +540,6 @@ public class Nat464Xlat { stop(); } - public void interfaceLinkStateChanged(String iface, boolean up) { - mNetwork.handler().post(() -> { handleInterfaceLinkStateChanged(iface, up); }); - } - - public void interfaceRemoved(String iface) { - mNetwork.handler().post(() -> handleInterfaceRemoved(iface)); - } - /** * Translate the input v4 address to v6 clat address. */ diff --git a/tests/unit/java/com/android/server/ConnectivityServiceTest.java b/tests/unit/java/com/android/server/ConnectivityServiceTest.java index 2fccdcb9a4..c8cbce1ef7 100755 --- a/tests/unit/java/com/android/server/ConnectivityServiceTest.java +++ b/tests/unit/java/com/android/server/ConnectivityServiceTest.java @@ -10772,6 +10772,8 @@ public class ConnectivityServiceTest { final RouteInfo ipv4Subnet = new RouteInfo(myIpv4, null, MOBILE_IFNAME); final RouteInfo stackedDefault = new RouteInfo((IpPrefix) null, myIpv4.getAddress(), CLAT_MOBILE_IFNAME); + final BaseNetdUnsolicitedEventListener netdUnsolicitedListener = + getRegisteredNetdUnsolicitedEventListener(); final NetworkRequest networkRequest = new NetworkRequest.Builder() .addTransportType(TRANSPORT_CELLULAR) @@ -10839,7 +10841,6 @@ public class ConnectivityServiceTest { assertRoutesRemoved(cellNetId, ipv4Subnet); // When NAT64 prefix discovery succeeds, LinkProperties are updated and clatd is started. - Nat464Xlat clat = getNat464Xlat(mCellAgent); assertNull(mCm.getLinkProperties(mCellAgent.getNetwork()).getNat64Prefix()); mService.mResolverUnsolEventCallback.onNat64PrefixEvent( makeNat64PrefixEvent(cellNetId, PREFIX_OPERATION_ADDED, kNat64PrefixString, 96)); @@ -10850,7 +10851,8 @@ public class ConnectivityServiceTest { verifyClatdStart(null /* inOrder */, MOBILE_IFNAME, cellNetId, kNat64Prefix.toString()); // Clat iface comes up. Expect stacked link to be added. - clat.interfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true); + netdUnsolicitedListener.onInterfaceLinkStateChanged( + CLAT_MOBILE_IFNAME, true); networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent); List stackedLps = mCm.getLinkProperties(mCellAgent.getNetwork()) .getStackedLinks(); @@ -10896,7 +10898,7 @@ public class ConnectivityServiceTest { kOtherNat64Prefix.toString()); networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent, cb -> cb.getLp().getNat64Prefix().equals(kOtherNat64Prefix)); - clat.interfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true); + netdUnsolicitedListener.onInterfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true); networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent, cb -> cb.getLp().getStackedLinks().size() == 1); assertRoutesAdded(cellNetId, stackedDefault); @@ -10924,7 +10926,7 @@ public class ConnectivityServiceTest { assertRoutesRemoved(cellNetId, stackedDefault); // The interface removed callback happens but has no effect after stop is called. - clat.interfaceRemoved(CLAT_MOBILE_IFNAME); + netdUnsolicitedListener.onInterfaceRemoved(CLAT_MOBILE_IFNAME); networkCallback.assertNoCallback(); verify(mMockNetd, times(1)).networkRemoveInterface(cellNetId, CLAT_MOBILE_IFNAME); @@ -10961,7 +10963,7 @@ public class ConnectivityServiceTest { verifyClatdStart(null /* inOrder */, MOBILE_IFNAME, cellNetId, kNat64Prefix.toString()); // Clat iface comes up. Expect stacked link to be added. - clat.interfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true); + netdUnsolicitedListener.onInterfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true); networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent, cb -> cb.getLp().getStackedLinks().size() == 1 && cb.getLp().getNat64Prefix() != null); @@ -11029,8 +11031,7 @@ public class ConnectivityServiceTest { // Clatd is started and clat iface comes up. Expect stacked link to be added. verifyClatdStart(null /* inOrder */, MOBILE_IFNAME, cellNetId, kNat64Prefix.toString()); - clat = getNat464Xlat(mCellAgent); - clat.interfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true /* up */); + netdUnsolicitedListener.onInterfaceLinkStateChanged(CLAT_MOBILE_IFNAME, true /* up */); networkCallback.expect(LINK_PROPERTIES_CHANGED, mCellAgent, cb -> cb.getLp().getStackedLinks().size() == 1 && cb.getLp().getNat64Prefix().equals(kNat64Prefix)); diff --git a/tests/unit/java/com/android/server/connectivity/Nat464XlatTest.java b/tests/unit/java/com/android/server/connectivity/Nat464XlatTest.java index 58c0114761..2fe871389a 100644 --- a/tests/unit/java/com/android/server/connectivity/Nat464XlatTest.java +++ b/tests/unit/java/com/android/server/connectivity/Nat464XlatTest.java @@ -86,7 +86,6 @@ public class Nat464XlatTest { @Mock ClatCoordinator mClatCoordinator; TestLooper mLooper; - Handler mHandler; NetworkAgentConfig mAgentConfig = new NetworkAgentConfig(); Nat464Xlat makeNat464Xlat(boolean isCellular464XlatEnabled) { @@ -96,6 +95,14 @@ public class Nat464XlatTest { } }; + // The test looper needs to be created here on the test case thread and not in setUp, + // because setUp and test cases are run in different threads. Creating the test looper in + // setUp would make Looper.getThread() return the setUp thread, which does not match the + // test case thread that is actually used to process the messages. + mLooper = new TestLooper(); + final Handler handler = new Handler(mLooper.getLooper()); + doReturn(handler).when(mNai).handler(); + return new Nat464Xlat(mNai, mNetd, mDnsResolver, deps) { @Override protected int getNetId() { return NETID; @@ -117,9 +124,6 @@ public class Nat464XlatTest { @Before public void setUp() throws Exception { - mLooper = new TestLooper(); - mHandler = new Handler(mLooper.getLooper()); - MockitoAnnotations.initMocks(this); mNai.linkProperties = new LinkProperties(); @@ -130,7 +134,6 @@ public class Nat464XlatTest { markNetworkConnected(); when(mNai.connService()).thenReturn(mConnectivity); when(mNai.netAgentConfig()).thenReturn(mAgentConfig); - when(mNai.handler()).thenReturn(mHandler); final InterfaceConfigurationParcel mConfig = new InterfaceConfigurationParcel(); when(mNetd.interfaceGetCfg(eq(STACKED_IFACE))).thenReturn(mConfig); mConfig.ipv4Addr = ADDR.getAddress().getHostAddress(); @@ -272,8 +275,7 @@ public class Nat464XlatTest { verifyClatdStart(null /* inOrder */); // Stacked interface up notification arrives. - nat.interfaceLinkStateChanged(STACKED_IFACE, true); - mLooper.dispatchNext(); + nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true); verify(mNetd).interfaceGetCfg(eq(STACKED_IFACE)); verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture()); @@ -294,8 +296,7 @@ public class Nat464XlatTest { // Verify the generated v6 is reset when clat is stopped. assertNull(nat.mIPv6Address); // Stacked interface removed notification arrives and is ignored. - nat.interfaceRemoved(STACKED_IFACE); - mLooper.dispatchNext(); + nat.handleInterfaceRemoved(STACKED_IFACE); verifyNoMoreInteractions(mNetd, mConnectivity); } @@ -324,8 +325,7 @@ public class Nat464XlatTest { verifyClatdStart(inOrder); // Stacked interface up notification arrives. - nat.interfaceLinkStateChanged(STACKED_IFACE, true); - mLooper.dispatchNext(); + nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true); inOrder.verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture()); assertFalse(c.getValue().getStackedLinks().isEmpty()); @@ -344,10 +344,8 @@ public class Nat464XlatTest { if (interfaceRemovedFirst) { // Stacked interface removed notification arrives and is ignored. - nat.interfaceRemoved(STACKED_IFACE); - mLooper.dispatchNext(); - nat.interfaceLinkStateChanged(STACKED_IFACE, false); - mLooper.dispatchNext(); + nat.handleInterfaceRemoved(STACKED_IFACE); + nat.handleInterfaceLinkStateChanged(STACKED_IFACE, false); } assertTrue(c.getValue().getStackedLinks().isEmpty()); @@ -361,15 +359,12 @@ public class Nat464XlatTest { if (!interfaceRemovedFirst) { // Stacked interface removed notification arrives and is ignored. - nat.interfaceRemoved(STACKED_IFACE); - mLooper.dispatchNext(); - nat.interfaceLinkStateChanged(STACKED_IFACE, false); - mLooper.dispatchNext(); + nat.handleInterfaceRemoved(STACKED_IFACE); + nat.handleInterfaceLinkStateChanged(STACKED_IFACE, false); } // Stacked interface up notification arrives. - nat.interfaceLinkStateChanged(STACKED_IFACE, true); - mLooper.dispatchNext(); + nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true); inOrder.verify(mConnectivity).handleUpdateLinkProperties(eq(mNai), c.capture()); assertFalse(c.getValue().getStackedLinks().isEmpty()); @@ -411,8 +406,7 @@ public class Nat464XlatTest { verifyClatdStart(null /* inOrder */); // Stacked interface up notification arrives. - nat.interfaceLinkStateChanged(STACKED_IFACE, true); - mLooper.dispatchNext(); + nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true); verify(mNetd).interfaceGetCfg(eq(STACKED_IFACE)); verify(mConnectivity, times(1)).handleUpdateLinkProperties(eq(mNai), c.capture()); @@ -421,8 +415,7 @@ public class Nat464XlatTest { assertRunning(nat); // Stacked interface removed notification arrives (clatd crashed, ...). - nat.interfaceRemoved(STACKED_IFACE); - mLooper.dispatchNext(); + nat.handleInterfaceRemoved(STACKED_IFACE); verifyClatdStop(null /* inOrder */); verify(mConnectivity, times(2)).handleUpdateLinkProperties(eq(mNai), c.capture()); @@ -457,12 +450,10 @@ public class Nat464XlatTest { assertIdle(nat); // In-flight interface up notification arrives: no-op - nat.interfaceLinkStateChanged(STACKED_IFACE, true); - mLooper.dispatchNext(); + nat.handleInterfaceLinkStateChanged(STACKED_IFACE, true); // Interface removed notification arrives after stopClatd() takes effect: no-op. - nat.interfaceRemoved(STACKED_IFACE); - mLooper.dispatchNext(); + nat.handleInterfaceRemoved(STACKED_IFACE); assertIdle(nat);