From d2d8d45b8915ef316b00ebbf0fa037107d0c0e4b Mon Sep 17 00:00:00 2001 From: markchien Date: Wed, 26 Feb 2020 20:54:55 +0800 Subject: [PATCH] Fix crash and duplicated ethernet tethering request This change fix two things: 1. Handle ethernet callback in internal thread to avoid crash. IpServer should be created from tethering thread, otherwise mIpNeighborMonitor of IpServer would throw IllegalStateException("start() called from off-thread") 2. Ethernet tethering request may be duplicated if multiple startTethering is called but no stopTethering Bug: 130840861 Bug: 148824036 Test: ON/OFF ethernet tehtering manually atest TetheringTests Change-Id: I7c5127e96d80d077735010d2e62c7227805ccb10 --- api/system-current.txt | 2 +- api/test-current.txt | 2 +- core/java/android/net/EthernetManager.java | 10 ++++---- .../connectivity/tethering/Tethering.java | 20 ++++++++-------- .../connectivity/tethering/TetheringTest.java | 23 +++++++++++++++++++ 5 files changed, 42 insertions(+), 15 deletions(-) diff --git a/api/system-current.txt b/api/system-current.txt index 91c23432e2ed1..9d660a3669bc7 100755 --- a/api/system-current.txt +++ b/api/system-current.txt @@ -4454,7 +4454,7 @@ package android.net { } public class EthernetManager { - method @NonNull public android.net.EthernetManager.TetheredInterfaceRequest requestTetheredInterface(@NonNull android.net.EthernetManager.TetheredInterfaceCallback); + method @NonNull public android.net.EthernetManager.TetheredInterfaceRequest requestTetheredInterface(@NonNull java.util.concurrent.Executor, @NonNull android.net.EthernetManager.TetheredInterfaceCallback); } public static interface EthernetManager.TetheredInterfaceCallback { diff --git a/api/test-current.txt b/api/test-current.txt index d0c13724b363d..c2553ed35ef70 100644 --- a/api/test-current.txt +++ b/api/test-current.txt @@ -1440,7 +1440,7 @@ package android.net { } public class EthernetManager { - method @NonNull public android.net.EthernetManager.TetheredInterfaceRequest requestTetheredInterface(@NonNull android.net.EthernetManager.TetheredInterfaceCallback); + method @NonNull public android.net.EthernetManager.TetheredInterfaceRequest requestTetheredInterface(@NonNull java.util.concurrent.Executor, @NonNull android.net.EthernetManager.TetheredInterfaceCallback); } public static interface EthernetManager.TetheredInterfaceCallback { diff --git a/core/java/android/net/EthernetManager.java b/core/java/android/net/EthernetManager.java index a3899b705c1b5..139f5bebcd899 100644 --- a/core/java/android/net/EthernetManager.java +++ b/core/java/android/net/EthernetManager.java @@ -28,6 +28,7 @@ import android.os.RemoteException; import java.util.ArrayList; import java.util.Objects; +import java.util.concurrent.Executor; /** * A class representing the IP configuration of the Ethernet network. @@ -248,18 +249,19 @@ public class EthernetManager { * @param callback A callback to be called once the request has been fulfilled. */ @NonNull - public TetheredInterfaceRequest requestTetheredInterface( - @NonNull TetheredInterfaceCallback callback) { + public TetheredInterfaceRequest requestTetheredInterface(@NonNull final Executor executor, + @NonNull final TetheredInterfaceCallback callback) { Objects.requireNonNull(callback, "Callback must be non-null"); + Objects.requireNonNull(executor, "Executor must be non-null"); final ITetheredInterfaceCallback cbInternal = new ITetheredInterfaceCallback.Stub() { @Override public void onAvailable(String iface) { - callback.onAvailable(iface); + executor.execute(() -> callback.onAvailable(iface)); } @Override public void onUnavailable() { - callback.onUnavailable(); + executor.execute(() -> callback.onUnavailable()); } }; diff --git a/packages/Tethering/src/com/android/server/connectivity/tethering/Tethering.java b/packages/Tethering/src/com/android/server/connectivity/tethering/Tethering.java index 84ca656d64ba8..0958e8a1a407c 100644 --- a/packages/Tethering/src/com/android/server/connectivity/tethering/Tethering.java +++ b/packages/Tethering/src/com/android/server/connectivity/tethering/Tethering.java @@ -220,6 +220,7 @@ public class Tethering { private final UserRestrictionActionListener mTetheringRestriction; private final ActiveDataSubIdListener mActiveDataSubIdListener; private final ConnectedClientsTracker mConnectedClientsTracker; + private final TetheringThreadExecutor mExecutor; private int mActiveDataSubId = INVALID_SUBSCRIPTION_ID; // All the usage of mTetheringEventCallback should run in the same thread. private ITetheringEventCallback mTetheringEventCallback = null; @@ -296,8 +297,8 @@ public class Tethering { final UserManager userManager = (UserManager) mContext.getSystemService( Context.USER_SERVICE); mTetheringRestriction = new UserRestrictionActionListener(userManager, this); - final TetheringThreadExecutor executor = new TetheringThreadExecutor(mHandler); - mActiveDataSubIdListener = new ActiveDataSubIdListener(executor); + mExecutor = new TetheringThreadExecutor(mHandler); + mActiveDataSubIdListener = new ActiveDataSubIdListener(mExecutor); // Load tethering configuration. updateConfiguration(); @@ -315,9 +316,7 @@ public class Tethering { final WifiManager wifiManager = getWifiManager(); if (wifiManager != null) { - wifiManager.registerSoftApCallback( - mHandler::post /* executor */, - new TetheringSoftApCallback()); + wifiManager.registerSoftApCallback(mExecutor, new TetheringSoftApCallback()); } } @@ -606,14 +605,17 @@ public class Tethering { Context.ETHERNET_SERVICE); synchronized (mPublicSync) { if (enable) { + if (mEthernetCallback != null) return TETHER_ERROR_NO_ERROR; + mEthernetCallback = new EthernetCallback(); - mEthernetIfaceRequest = em.requestTetheredInterface(mEthernetCallback); + mEthernetIfaceRequest = em.requestTetheredInterface(mExecutor, mEthernetCallback); } else { - if (mConfiguredEthernetIface != null) { - stopEthernetTetheringLocked(); + stopEthernetTetheringLocked(); + if (mEthernetCallback != null) { mEthernetIfaceRequest.release(); + mEthernetCallback = null; + mEthernetIfaceRequest = null; } - mEthernetCallback = null; } } return TETHER_ERROR_NO_ERROR; diff --git a/packages/Tethering/tests/unit/src/com/android/server/connectivity/tethering/TetheringTest.java b/packages/Tethering/tests/unit/src/com/android/server/connectivity/tethering/TetheringTest.java index d14c62a981fdf..4581b56f13dcb 100644 --- a/packages/Tethering/tests/unit/src/com/android/server/connectivity/tethering/TetheringTest.java +++ b/packages/Tethering/tests/unit/src/com/android/server/connectivity/tethering/TetheringTest.java @@ -28,6 +28,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_ETHERNET; import static android.net.TetheringManager.TETHERING_NCM; import static android.net.TetheringManager.TETHERING_USB; import static android.net.TetheringManager.TETHERING_WIFI; @@ -75,6 +76,8 @@ import android.content.pm.ApplicationInfo; import android.content.res.Resources; import android.hardware.usb.UsbManager; import android.net.ConnectivityManager; +import android.net.EthernetManager; +import android.net.EthernetManager.TetheredInterfaceRequest; import android.net.INetd; import android.net.ITetheringEventCallback; import android.net.InetAddresses; @@ -180,6 +183,7 @@ public class TetheringTest { @Mock private UserManager mUserManager; @Mock private NetworkRequest mNetworkRequest; @Mock private ConnectivityManager mCm; + @Mock private EthernetManager mEm; private final MockIpServerDependencies mIpServerDependencies = spy(new MockIpServerDependencies()); @@ -232,6 +236,7 @@ public class TetheringTest { if (Context.USER_SERVICE.equals(name)) return mUserManager; if (Context.NETWORK_STATS_SERVICE.equals(name)) return mStatsManager; if (Context.CONNECTIVITY_SERVICE.equals(name)) return mCm; + if (Context.ETHERNET_SERVICE.equals(name)) return mEm; return super.getSystemService(name); } @@ -1316,6 +1321,24 @@ public class TetheringTest { assertEquals(fakeSubId, newConfig.activeDataSubId); } + @Test + public void testNoDuplicatedEthernetRequest() throws Exception { + final TetheredInterfaceRequest mockRequest = mock(TetheredInterfaceRequest.class); + when(mEm.requestTetheredInterface(any(), any())).thenReturn(mockRequest); + mTethering.startTethering(createTetheringRquestParcel(TETHERING_ETHERNET), null); + mLooper.dispatchAll(); + verify(mEm, times(1)).requestTetheredInterface(any(), any()); + mTethering.startTethering(createTetheringRquestParcel(TETHERING_ETHERNET), null); + mLooper.dispatchAll(); + verifyNoMoreInteractions(mEm); + mTethering.stopTethering(TETHERING_ETHERNET); + mLooper.dispatchAll(); + verify(mockRequest, times(1)).release(); + mTethering.stopTethering(TETHERING_ETHERNET); + mLooper.dispatchAll(); + verifyNoMoreInteractions(mEm); + } + private void workingWifiP2pGroupOwner( boolean emulateInterfaceStatusChanged) throws Exception { if (emulateInterfaceStatusChanged) {