From 1e020903cc3e611c94dd013f0e4d74c6226bc6e3 Mon Sep 17 00:00:00 2001 From: markchien Date: Fri, 3 Dec 2021 11:38:38 +0800 Subject: [PATCH] Reuse BluetoothPan object and use it under tethering handler thread 1. Instead of create BluetoothPan every time when tethering need to use it, store it with mBluetoothPan and resue it. 2. Call BluetoothPan function under tethering handler thread. Bug: 190438212 Test: atest TetheringTests Change-Id: I40adece59960ec44a02dc438d6bd95483a0788af --- .../networkstack/tethering/Tethering.java | 110 +++++++++++++----- .../networkstack/tethering/TetheringTest.java | 102 ++++++++++++---- 2 files changed, 164 insertions(+), 48 deletions(-) diff --git a/Tethering/src/com/android/networkstack/tethering/Tethering.java b/Tethering/src/com/android/networkstack/tethering/Tethering.java index 78f2afce4e..55c24d3830 100644 --- a/Tethering/src/com/android/networkstack/tethering/Tethering.java +++ b/Tethering/src/com/android/networkstack/tethering/Tethering.java @@ -123,6 +123,7 @@ import android.telephony.TelephonyManager; import android.text.TextUtils; import android.util.ArrayMap; import android.util.Log; +import android.util.Pair; import android.util.SparseArray; import androidx.annotation.NonNull; @@ -267,6 +268,9 @@ public class Tethering { private String mConfiguredEthernetIface; private EthernetCallback mEthernetCallback; private SettingsObserver mSettingsObserver; + private BluetoothPan mBluetoothPan; + private PanServiceListener mBluetoothPanListener; + private ArrayList> mPendingPanRequests; public Tethering(TetheringDependencies deps) { mLog.mark("Tethering.constructed"); @@ -276,6 +280,11 @@ public class Tethering { mLooper = mDeps.getTetheringLooper(); mNotificationUpdater = mDeps.getNotificationUpdater(mContext, mLooper); + // This is intended to ensrure that if something calls startTethering(bluetooth) just after + // bluetooth is enabled. Before onServiceConnected is called, store the calls into this + // list and handle them as soon as onServiceConnected is called. + mPendingPanRequests = new ArrayList<>(); + mTetherStates = new ArrayMap<>(); mConnectedClientsTracker = new ConnectedClientsTracker(); @@ -701,35 +710,82 @@ public class Tethering { return; } - adapter.getProfileProxy(mContext, new ServiceListener() { - @Override - public void onServiceDisconnected(int profile) { } + if (mBluetoothPanListener != null && mBluetoothPanListener.isConnected()) { + // The PAN service is connected. Enable or disable bluetooth tethering. + // When bluetooth tethering is enabled, any time a PAN client pairs with this + // host, bluetooth will bring up a bt-pan interface and notify tethering to + // enable IP serving. + setBluetoothTetheringSettings(mBluetoothPan, enable, listener); + return; + } - @Override - public void onServiceConnected(int profile, BluetoothProfile proxy) { - // Clear identify is fine because caller already pass tethering permission at - // ConnectivityService#startTethering()(or stopTethering) before the control comes - // here. Bluetooth will check tethering permission again that there is - // Context#getOpPackageName() under BluetoothPan#setBluetoothTethering() to get - // caller's package name for permission check. - // Calling BluetoothPan#setBluetoothTethering() here means the package name always - // be system server. If calling identity is not cleared, that package's uid might - // not match calling uid and end up in permission denied. - final long identityToken = Binder.clearCallingIdentity(); - try { - ((BluetoothPan) proxy).setBluetoothTethering(enable); - } finally { - Binder.restoreCallingIdentity(identityToken); + // The reference of IIntResultListener should only exist when application want to start + // tethering but tethering is not bound to pan service yet. Even if the calling process + // dies, the referenice of IIntResultListener would still keep in mPendingPanRequests. Once + // tethering bound to pan service (onServiceConnected) or bluetooth just crash + // (onServiceDisconnected), all the references from mPendingPanRequests would be cleared. + mPendingPanRequests.add(new Pair(enable, listener)); + + // Bluetooth tethering is not a popular feature. To avoid bind to bluetooth pan service all + // the time but user never use bluetooth tethering. mBluetoothPanListener is created first + // time someone calls a bluetooth tethering method (even if it's just to disable tethering + // when it's already disabled) and never unset after that. + if (mBluetoothPanListener == null) { + mBluetoothPanListener = new PanServiceListener(); + adapter.getProfileProxy(mContext, mBluetoothPanListener, BluetoothProfile.PAN); + } + } + + private class PanServiceListener implements ServiceListener { + private boolean mIsConnected = false; + + @Override + public void onServiceConnected(int profile, BluetoothProfile proxy) { + // Posting this to handling onServiceConnected in tethering handler thread may have + // race condition that bluetooth service may disconnected when tethering thread + // actaully handle onServiceconnected. If this race happen, calling + // BluetoothPan#setBluetoothTethering would silently fail. It is fine because pan + // service is unreachable and both bluetooth and bluetooth tethering settings are off. + mHandler.post(() -> { + mBluetoothPan = (BluetoothPan) proxy; + mIsConnected = true; + + for (Pair request : mPendingPanRequests) { + setBluetoothTetheringSettings(mBluetoothPan, request.first, request.second); } - // TODO: Enabling bluetooth tethering can fail asynchronously here. - // We should figure out a way to bubble up that failure instead of sending success. - final int result = (((BluetoothPan) proxy).isTetheringOn() == enable) - ? TETHER_ERROR_NO_ERROR - : TETHER_ERROR_INTERNAL_ERROR; - sendTetherResult(listener, result, TETHERING_BLUETOOTH); - adapter.closeProfileProxy(BluetoothProfile.PAN, proxy); - } - }, BluetoothProfile.PAN); + mPendingPanRequests.clear(); + }); + } + + @Override + public void onServiceDisconnected(int profile) { + mHandler.post(() -> { + // onServiceDisconnected means Bluetooth is off (or crashed) and is not + // reachable before next onServiceConnected. + mIsConnected = false; + + for (Pair request : mPendingPanRequests) { + sendTetherResult(request.second, TETHER_ERROR_SERVICE_UNAVAIL, + TETHERING_BLUETOOTH); + } + mPendingPanRequests.clear(); + }); + } + + public boolean isConnected() { + return mIsConnected; + } + } + + private void setBluetoothTetheringSettings(@NonNull final BluetoothPan bluetoothPan, + final boolean enable, final IIntResultListener listener) { + bluetoothPan.setBluetoothTethering(enable); + + // Enabling bluetooth tethering settings can silently fail. Send internal error if the + // result is not expected. + final int result = bluetoothPan.isTetheringOn() == enable + ? TETHER_ERROR_NO_ERROR : TETHER_ERROR_INTERNAL_ERROR; + sendTetherResult(listener, result, TETHERING_BLUETOOTH); } private int setEthernetTethering(final boolean enable) { 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 f45768fd5d..40d133a2a2 100644 --- a/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java +++ b/Tethering/tests/unit/src/com/android/networkstack/tethering/TetheringTest.java @@ -2558,10 +2558,10 @@ public class TetheringTest { @Test public void testBluetoothTethering() throws Exception { final ResultListener result = new ResultListener(TETHER_ERROR_NO_ERROR); - when(mBluetoothAdapter.isEnabled()).thenReturn(true); + mockBluetoothSettings(true /* bluetoothOn */, true /* tetheringOn */); mTethering.startTethering(createTetheringRequestParcel(TETHERING_BLUETOOTH), result); mLooper.dispatchAll(); - verifySetBluetoothTethering(true); + verifySetBluetoothTethering(true /* enable */, true /* bindToPanService */); result.assertHasResult(); mTethering.interfaceAdded(TEST_BT_IFNAME); @@ -2574,6 +2574,64 @@ public class TetheringTest { mLooper.dispatchAll(); tetherResult.assertHasResult(); + verifyNetdCommandForBtSetup(); + + // Turning tethering on a second time does not bind to the PAN service again, since it's + // already bound. + mockBluetoothSettings(true /* bluetoothOn */, true /* tetheringOn */); + final ResultListener secondResult = new ResultListener(TETHER_ERROR_NO_ERROR); + mTethering.startTethering(createTetheringRequestParcel(TETHERING_BLUETOOTH), secondResult); + mLooper.dispatchAll(); + verifySetBluetoothTethering(true /* enable */, false /* bindToPanService */); + secondResult.assertHasResult(); + + mockBluetoothSettings(true /* bluetoothOn */, false /* tetheringOn */); + 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 /* enable */, false /* bindToPanService */); + + verifyNetdCommandForBtTearDown(); + } + + @Test + public void testBluetoothServiceDisconnects() throws Exception { + final ResultListener result = new ResultListener(TETHER_ERROR_NO_ERROR); + mockBluetoothSettings(true /* bluetoothOn */, true /* tetheringOn */); + mTethering.startTethering(createTetheringRequestParcel(TETHERING_BLUETOOTH), result); + mLooper.dispatchAll(); + ServiceListener panListener = verifySetBluetoothTethering(true /* enable */, + true /* bindToPanService */); + 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, IpServer.STATE_TETHERED, tetherResult); + mLooper.dispatchAll(); + tetherResult.assertHasResult(); + + verifyNetdCommandForBtSetup(); + + panListener.onServiceDisconnected(BluetoothProfile.PAN); + mTethering.interfaceStatusChanged(TEST_BT_IFNAME, false); + mLooper.dispatchAll(); + + verifyNetdCommandForBtTearDown(); + } + + private void mockBluetoothSettings(boolean bluetoothOn, boolean tetheringOn) { + when(mBluetoothAdapter.isEnabled()).thenReturn(bluetoothOn); + when(mBluetoothPan.isTetheringOn()).thenReturn(tetheringOn); + } + + private void verifyNetdCommandForBtSetup() throws Exception { 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), @@ -2584,39 +2642,41 @@ public class TetheringTest { 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); - + private void verifyNetdCommandForBtTearDown() throws Exception { 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); + // If bindToPanService is true, this function would return ServiceListener which could notify + // PanService is connected or disconnected. + private ServiceListener verifySetBluetoothTethering(final boolean enable, + final boolean bindToPanService) { + ServiceListener listener = null; 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); + if (bindToPanService) { + final ArgumentCaptor listenerCaptor = + ArgumentCaptor.forClass(ServiceListener.class); + verify(mBluetoothAdapter).getProfileProxy(eq(mServiceContext), listenerCaptor.capture(), + eq(BluetoothProfile.PAN)); + listener = listenerCaptor.getValue(); + listener.onServiceConnected(BluetoothProfile.PAN, mBluetoothPan); + mLooper.dispatchAll(); + } else { + verify(mBluetoothAdapter, never()).getProfileProxy(eq(mServiceContext), any(), + anyInt()); + } verify(mBluetoothPan).setBluetoothTethering(enable); verify(mBluetoothPan).isTetheringOn(); - verify(mBluetoothAdapter).closeProfileProxy(eq(BluetoothProfile.PAN), eq(mBluetoothPan)); verifyNoMoreInteractions(mBluetoothAdapter, mBluetoothPan); reset(mBluetoothAdapter, mBluetoothPan); + + return listener; } private void runDualStackUsbTethering(final String expectedIface) throws Exception {