From c233b1807e4b7e0b6d8f14fb1f66346d50858a9d Mon Sep 17 00:00:00 2001 From: markchien Date: Tue, 29 Dec 2020 00:08:51 +0800 Subject: [PATCH] Put tether/untether calls into handler queue BT tethering is the only user of #tether and #untether API. Enable BT tethering may fail by race between interface status change and #tether call. In starting BT tethering flow, tethering may receive pan interface DOWN and UP then PanService call #tether to request for processing its pan interface. Tethering put interface status change event into handler queue but it process #tether and #unether calls right away without order them by queue. It makes tethering handle binder call #tether before pan interface DOWN even DOWN event arrived eailer. This cause BT tethering bring down by interface DOWN event unexpectedly. Although this still can't fix all the race cases of tethering, but at lesat this could fix the race mentioned above. Bug: 173310882 Test: manual ON/OFF bt tethering atest TetheringTests Change-Id: I2411378aa36ad4371cca12423bb65542cb8df7a1 --- .../networkstack/tethering/Tethering.java | 16 +++- .../tethering/TetheringService.java | 8 +- .../tethering/TetheringServiceTest.java | 8 +- .../networkstack/tethering/TetheringTest.java | 84 +++++++++++++++++-- 4 files changed, 95 insertions(+), 21 deletions(-) diff --git a/Tethering/src/com/android/networkstack/tethering/Tethering.java b/Tethering/src/com/android/networkstack/tethering/Tethering.java index 62ae88c24b..0cf6fcdeae 100644 --- a/Tethering/src/com/android/networkstack/tethering/Tethering.java +++ b/Tethering/src/com/android/networkstack/tethering/Tethering.java @@ -747,8 +747,12 @@ public class Tethering { } } - int tether(String iface) { - return tether(iface, IpServer.STATE_TETHERED); + void tether(String iface, final IIntResultListener listener) { + mHandler.post(() -> { + try { + listener.onResult(tether(iface, IpServer.STATE_TETHERED)); + } catch (RemoteException e) { } + }); } private int tether(String iface, int requestedState) { @@ -781,6 +785,14 @@ public class Tethering { } } + void untether(String iface, final IIntResultListener listener) { + mHandler.post(() -> { + try { + listener.onResult(untether(iface)); + } catch (RemoteException e) { } + }); + } + int untether(String iface) { if (DBG) Log.d(TAG, "Untethering " + iface); synchronized (mPublicSync) { diff --git a/Tethering/src/com/android/networkstack/tethering/TetheringService.java b/Tethering/src/com/android/networkstack/tethering/TetheringService.java index 613328d1c1..400b31ee06 100644 --- a/Tethering/src/com/android/networkstack/tethering/TetheringService.java +++ b/Tethering/src/com/android/networkstack/tethering/TetheringService.java @@ -106,9 +106,7 @@ public class TetheringService extends Service { IIntResultListener listener) { if (checkAndNotifyCommonError(callerPkg, callingAttributionTag, listener)) return; - try { - listener.onResult(mTethering.tether(iface)); - } catch (RemoteException e) { } + mTethering.tether(iface, listener); } @Override @@ -116,9 +114,7 @@ public class TetheringService extends Service { IIntResultListener listener) { if (checkAndNotifyCommonError(callerPkg, callingAttributionTag, listener)) return; - try { - listener.onResult(mTethering.untether(iface)); - } catch (RemoteException e) { } + mTethering.untether(iface, listener); } @Override diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringServiceTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringServiceTest.java index 7bba67b05f..be98f6012f 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringServiceTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringServiceTest.java @@ -156,11 +156,9 @@ public final class TetheringServiceTest { } private void runTether(final TestTetheringResult result) throws Exception { - when(mTethering.tether(TEST_IFACE_NAME)).thenReturn(TETHER_ERROR_NO_ERROR); mTetheringConnector.tether(TEST_IFACE_NAME, TEST_CALLER_PKG, TEST_ATTRIBUTION_TAG, result); verify(mTethering).isTetheringSupported(); - verify(mTethering).tether(TEST_IFACE_NAME); - result.assertResult(TETHER_ERROR_NO_ERROR); + verify(mTethering).tether(eq(TEST_IFACE_NAME), eq(result)); } @Test @@ -186,12 +184,10 @@ public final class TetheringServiceTest { } private void runUnTether(final TestTetheringResult result) throws Exception { - when(mTethering.untether(TEST_IFACE_NAME)).thenReturn(TETHER_ERROR_NO_ERROR); mTetheringConnector.untether(TEST_IFACE_NAME, TEST_CALLER_PKG, TEST_ATTRIBUTION_TAG, result); verify(mTethering).isTetheringSupported(); - verify(mTethering).untether(TEST_IFACE_NAME); - result.assertResult(TETHER_ERROR_NO_ERROR); + verify(mTethering).untether(eq(TEST_IFACE_NAME), eq(result)); } @Test diff --git a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java index f4b3749132..53c35f8f79 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java @@ -33,6 +33,7 @@ import static android.net.TetheringManager.ACTION_TETHER_STATE_CHANGED; import static android.net.TetheringManager.EXTRA_ACTIVE_LOCAL_ONLY; import static android.net.TetheringManager.EXTRA_ACTIVE_TETHER; import static android.net.TetheringManager.EXTRA_AVAILABLE_TETHER; +import static android.net.TetheringManager.TETHERING_BLUETOOTH; import static android.net.TetheringManager.TETHERING_ETHERNET; import static android.net.TetheringManager.TETHERING_NCM; import static android.net.TetheringManager.TETHERING_USB; @@ -85,6 +86,9 @@ import static org.mockito.Mockito.when; import android.app.usage.NetworkStatsManager; import android.bluetooth.BluetoothAdapter; +import android.bluetooth.BluetoothPan; +import android.bluetooth.BluetoothProfile; +import android.bluetooth.BluetoothProfile.ServiceListener; import android.content.BroadcastReceiver; import android.content.ContentResolver; import android.content.Context; @@ -226,6 +230,8 @@ public class TetheringTest { @Mock private TetheringNotificationUpdater mNotificationUpdater; @Mock private BpfCoordinator mBpfCoordinator; @Mock private PackageManager mPackageManager; + @Mock private BluetoothAdapter mBluetoothAdapter; + @Mock private BluetoothPan mBluetoothPan; private final MockIpServerDependencies mIpServerDependencies = spy(new MockIpServerDependencies()); @@ -320,7 +326,8 @@ public class TetheringTest { || ifName.equals(TEST_MOBILE_IFNAME) || ifName.equals(TEST_P2P_IFNAME) || ifName.equals(TEST_NCM_IFNAME) - || ifName.equals(TEST_ETH_IFNAME)); + || ifName.equals(TEST_ETH_IFNAME) + || ifName.equals(TEST_BT_IFNAME)); final String[] ifaces = new String[] { TEST_USB_IFNAME, TEST_WLAN_IFNAME, TEST_MOBILE_IFNAME, TEST_P2P_IFNAME, TEST_NCM_IFNAME, TEST_ETH_IFNAME}; @@ -452,8 +459,7 @@ public class TetheringTest { @Override public BluetoothAdapter getBluetoothAdapter() { - // TODO: add test for bluetooth tethering. - return null; + return mBluetoothAdapter; } @Override @@ -550,7 +556,7 @@ public class TetheringTest { when(mNetd.interfaceGetList()) .thenReturn(new String[] { TEST_MOBILE_IFNAME, TEST_WLAN_IFNAME, TEST_USB_IFNAME, TEST_P2P_IFNAME, - TEST_NCM_IFNAME, TEST_ETH_IFNAME}); + TEST_NCM_IFNAME, TEST_ETH_IFNAME, TEST_BT_IFNAME}); when(mResources.getString(R.string.config_wifi_tether_enable)).thenReturn(""); mInterfaceConfiguration = new InterfaceConfigurationParcel(); mInterfaceConfiguration.flags = new String[0]; @@ -597,11 +603,11 @@ public class TetheringTest { when(mResources.getStringArray(R.array.config_tether_usb_regexs)) .thenReturn(new String[] { "test_rndis\\d" }); when(mResources.getStringArray(R.array.config_tether_wifi_regexs)) - .thenReturn(new String[]{ "test_wlan\\d" }); + .thenReturn(new String[] { "test_wlan\\d" }); when(mResources.getStringArray(R.array.config_tether_wifi_p2p_regexs)) - .thenReturn(new String[]{ "test_p2p-p2p\\d-.*" }); + .thenReturn(new String[] { "test_p2p-p2p\\d-.*" }); when(mResources.getStringArray(R.array.config_tether_bluetooth_regexs)) - .thenReturn(new String[0]); + .thenReturn(new String[] { "test_pan\\d" }); when(mResources.getStringArray(R.array.config_tether_ncm_regexs)) .thenReturn(new String[] { "test_ncm\\d" }); when(mResources.getIntArray(R.array.config_tether_upstream_types)).thenReturn(new int[0]); @@ -2190,6 +2196,70 @@ public class TetheringTest { return lease; } + @Test + public void testBluetoothTethering() throws Exception { + final ResultListener result = new ResultListener(TETHER_ERROR_NO_ERROR); + when(mBluetoothAdapter.isEnabled()).thenReturn(true); + mTethering.startTethering(createTetheringRequestParcel(TETHERING_BLUETOOTH), result); + mLooper.dispatchAll(); + verifySetBluetoothTethering(true); + result.assertHasResult(); + + mTethering.interfaceAdded(TEST_BT_IFNAME); + mLooper.dispatchAll(); + + mTethering.interfaceStatusChanged(TEST_BT_IFNAME, false); + mTethering.interfaceStatusChanged(TEST_BT_IFNAME, true); + final ResultListener tetherResult = new ResultListener(TETHER_ERROR_NO_ERROR); + mTethering.tether(TEST_BT_IFNAME, tetherResult); + mLooper.dispatchAll(); + tetherResult.assertHasResult(); + + verify(mNetd).tetherInterfaceAdd(TEST_BT_IFNAME); + verify(mNetd).networkAddInterface(INetd.LOCAL_NET_ID, TEST_BT_IFNAME); + verify(mNetd, times(2)).networkAddRoute(eq(INetd.LOCAL_NET_ID), eq(TEST_BT_IFNAME), + anyString(), anyString()); + verify(mNetd).ipfwdEnableForwarding(TETHERING_NAME); + verify(mNetd).tetherStartWithConfiguration(any()); + verify(mNetd, times(2)).networkAddRoute(eq(INetd.LOCAL_NET_ID), eq(TEST_BT_IFNAME), + anyString(), anyString()); + verifyNoMoreInteractions(mNetd); + reset(mNetd); + + when(mBluetoothAdapter.isEnabled()).thenReturn(true); + mTethering.stopTethering(TETHERING_BLUETOOTH); + mLooper.dispatchAll(); + final ResultListener untetherResult = new ResultListener(TETHER_ERROR_NO_ERROR); + mTethering.untether(TEST_BT_IFNAME, untetherResult); + mLooper.dispatchAll(); + untetherResult.assertHasResult(); + verifySetBluetoothTethering(false); + + verify(mNetd).tetherApplyDnsInterfaces(); + verify(mNetd).tetherInterfaceRemove(TEST_BT_IFNAME); + verify(mNetd).networkRemoveInterface(INetd.LOCAL_NET_ID, TEST_BT_IFNAME); + verify(mNetd).interfaceSetCfg(any(InterfaceConfigurationParcel.class)); + verify(mNetd).tetherStop(); + verify(mNetd).ipfwdDisableForwarding(TETHERING_NAME); + verifyNoMoreInteractions(mNetd); + } + + private void verifySetBluetoothTethering(final boolean enable) { + final ArgumentCaptor listenerCaptor = + ArgumentCaptor.forClass(ServiceListener.class); + verify(mBluetoothAdapter).isEnabled(); + verify(mBluetoothAdapter).getProfileProxy(eq(mServiceContext), listenerCaptor.capture(), + eq(BluetoothProfile.PAN)); + final ServiceListener listener = listenerCaptor.getValue(); + when(mBluetoothPan.isTetheringOn()).thenReturn(enable); + listener.onServiceConnected(BluetoothProfile.PAN, mBluetoothPan); + verify(mBluetoothPan).setBluetoothTethering(enable); + verify(mBluetoothPan).isTetheringOn(); + verify(mBluetoothAdapter).closeProfileProxy(eq(BluetoothProfile.PAN), eq(mBluetoothPan)); + verifyNoMoreInteractions(mBluetoothAdapter, mBluetoothPan); + reset(mBluetoothAdapter, mBluetoothPan); + } + // TODO: Test that a request for hotspot mode doesn't interfere with an // already operating tethering mode interface. }