From be9a6d2b6c063f619e075888c3e3017ec210e530 Mon Sep 17 00:00:00 2001 From: Aaron Huang Date: Mon, 8 Feb 2021 10:43:40 +0000 Subject: [PATCH 1/2] Revert^2 "Refactor setCurrentProxyScriptUrl to a void method" setCurrentProxyScriptUrl is used by ProxyTracker which is included in connectivity service mainline module. To make this method @SystemApi in follow-up patch, change this from a boolean method to a void method. Moving the send or not send broadcast logic to caller because it can know whether it needs to send a broadcast or not. The original change was reverted because it broke DeviceOwnerTest and HostsideVpnTests. The new patch fixes no broadcast sent bug. The logic was reversed when a PAC file URL is empty so the broadcast did not send when proxy change. Bug: 179225084 Bug: 177035719 Test: FrameworksNetTests CtsHostsideNetworkTests:HostsideVpnTests#testSetProxy CtsHostsideNetworkTests:HostsideVpnTests#testBindToNetworkWithProxy CtsHostsideNetworkTests:HostsideVpnTests#testNoProxy CtsDevicePolicyManagerTestCases:DeviceOwnerTest#testProxyPacProxyTest CtsDevicePolicyManagerTestCases:DeviceOwnerTest#testProxyStaticProxyTest Change-Id: I029c33913264bfee336a559c4bd048ddd027322b --- .../com/android/server/connectivity/ProxyTracker.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/services/core/java/com/android/server/connectivity/ProxyTracker.java b/services/core/java/com/android/server/connectivity/ProxyTracker.java index d83ff837d9..154055a425 100644 --- a/services/core/java/com/android/server/connectivity/ProxyTracker.java +++ b/services/core/java/com/android/server/connectivity/ProxyTracker.java @@ -226,9 +226,9 @@ public class ProxyTracker { final ProxyInfo defaultProxy = getDefaultProxy(); final ProxyInfo proxyInfo = null != defaultProxy ? defaultProxy : ProxyInfo.buildDirectProxy("", 0, Collections.emptyList()); + mPacProxyInstaller.setCurrentProxyScriptUrl(proxyInfo); - if (mPacProxyInstaller.setCurrentProxyScriptUrl(proxyInfo) - == PacProxyInstaller.DONT_SEND_BROADCAST) { + if (!shouldSendBroadcast(proxyInfo)) { return; } if (DBG) Log.d(TAG, "sending Proxy Broadcast for " + proxyInfo); @@ -244,6 +244,10 @@ public class ProxyTracker { } } + private boolean shouldSendBroadcast(ProxyInfo proxy) { + return Uri.EMPTY.equals(proxy.getPacFileUrl()) || proxy.getPort() > 0; + } + /** * Sets the global proxy in memory. Also writes the values to the global settings of the device. * From f9fa0b95a82402be72a43af501454729420f5af0 Mon Sep 17 00:00:00 2001 From: Aaron Huang Date: Mon, 18 Jan 2021 15:28:01 +0800 Subject: [PATCH 2/2] Make PacProxyService be a system service PacProxyInstaller class is running a thread all the time and is listening to intent ACTION_PAC_REFRESH so it would be better to make it be a system service with a manager class PacProxyManager which is obtained with getSystemService(PacProxyManager.class). Besides, rename PacProxyInstaller to PacProxyService will be easier to know it's the service for PacProxyManager. ConnectivityService is going to be a mainline module and it needs constructor of PacProxyService to be SystemApi. However, in current design, it needs to pass a handler and an int arguments to the constructor which would be difficult to maintain if just expose the constructor directly. So, define a listener for the event that the current PAC proxy has been installed so that the handler and the int arguments can be removed from the constructor. Bug: 177035719 Test: FrameworksNetTests Change-Id: I2abff75ec59a17628ef006aad348c53fadbed076 --- framework/src/android/net/ProxyInfo.java | 2 +- .../android/server/ConnectivityService.java | 3 +- .../server/connectivity/ProxyTracker.java | 39 ++++++++++++++----- 3 files changed, 32 insertions(+), 12 deletions(-) diff --git a/framework/src/android/net/ProxyInfo.java b/framework/src/android/net/ProxyInfo.java index 229db0d717..745e20f154 100644 --- a/framework/src/android/net/ProxyInfo.java +++ b/framework/src/android/net/ProxyInfo.java @@ -129,7 +129,7 @@ public class ProxyInfo implements Parcelable { } /** - * Only used in PacProxyInstaller after Local Proxy is bound. + * Only used in PacProxyService after Local Proxy is bound. * @hide */ public ProxyInfo(@NonNull Uri pacFileUrl, int localProxyPort) { diff --git a/services/core/java/com/android/server/ConnectivityService.java b/services/core/java/com/android/server/ConnectivityService.java index 07b473caa9..4e271e5795 100644 --- a/services/core/java/com/android/server/ConnectivityService.java +++ b/services/core/java/com/android/server/ConnectivityService.java @@ -4420,7 +4420,8 @@ public class ConnectivityService extends IConnectivityManager.Stub break; } case EVENT_PROXY_HAS_CHANGED: { - handleApplyDefaultProxy((ProxyInfo)msg.obj); + final Pair arg = (Pair) msg.obj; + handleApplyDefaultProxy(arg.second); break; } case EVENT_REGISTER_NETWORK_PROVIDER: { diff --git a/services/core/java/com/android/server/connectivity/ProxyTracker.java b/services/core/java/com/android/server/connectivity/ProxyTracker.java index 154055a425..8b9c836787 100644 --- a/services/core/java/com/android/server/connectivity/ProxyTracker.java +++ b/services/core/java/com/android/server/connectivity/ProxyTracker.java @@ -27,15 +27,19 @@ import android.annotation.Nullable; import android.content.ContentResolver; import android.content.Context; import android.content.Intent; +import android.net.Network; +import android.net.PacProxyManager; import android.net.Proxy; import android.net.ProxyInfo; import android.net.Uri; import android.os.Binder; import android.os.Handler; +import android.os.HandlerExecutor; import android.os.UserHandle; import android.provider.Settings; import android.text.TextUtils; import android.util.Log; +import android.util.Pair; import com.android.internal.annotations.GuardedBy; import com.android.net.module.util.ProxyUtils; @@ -67,7 +71,7 @@ public class ProxyTracker { // is not set. Individual networks have their own settings that override this. This member // is set through setDefaultProxy, which is called when the default network changes proxies // in its LinkProperties, or when ConnectivityService switches to a new default network, or - // when PacProxyInstaller resolves the proxy. + // when PacProxyService resolves the proxy. @Nullable @GuardedBy("mProxyLock") private volatile ProxyInfo mDefaultProxy = null; @@ -77,16 +81,31 @@ public class ProxyTracker { private final Handler mConnectivityServiceHandler; - // The object responsible for Proxy Auto Configuration (PAC). - @NonNull - private final PacProxyInstaller mPacProxyInstaller; + private final PacProxyManager mPacProxyManager; + + private class PacProxyInstalledListener implements PacProxyManager.PacProxyInstalledListener { + private final int mEvent; + + PacProxyInstalledListener(int event) { + mEvent = event; + } + + public void onPacProxyInstalled(@Nullable Network network, @NonNull ProxyInfo proxy) { + mConnectivityServiceHandler + .sendMessage(mConnectivityServiceHandler + .obtainMessage(mEvent, new Pair<>(network, proxy))); + } + } public ProxyTracker(@NonNull final Context context, @NonNull final Handler connectivityServiceInternalHandler, final int pacChangedEvent) { mContext = context; mConnectivityServiceHandler = connectivityServiceInternalHandler; - mPacProxyInstaller = new PacProxyInstaller( - context, connectivityServiceInternalHandler, pacChangedEvent); + mPacProxyManager = context.getSystemService(PacProxyManager.class); + + PacProxyInstalledListener listener = new PacProxyInstalledListener(pacChangedEvent); + mPacProxyManager.addPacProxyInstalledListener( + new HandlerExecutor(mConnectivityServiceHandler), listener); } // Convert empty ProxyInfo's to null as null-checks are used to determine if proxies are present @@ -182,7 +201,7 @@ public class ProxyTracker { if (!TextUtils.isEmpty(pacFileUrl)) { mConnectivityServiceHandler.post( - () -> mPacProxyInstaller.setCurrentProxyScriptUrl(proxyProperties)); + () -> mPacProxyManager.setCurrentProxyScriptUrl(proxyProperties)); } } } @@ -226,7 +245,7 @@ public class ProxyTracker { final ProxyInfo defaultProxy = getDefaultProxy(); final ProxyInfo proxyInfo = null != defaultProxy ? defaultProxy : ProxyInfo.buildDirectProxy("", 0, Collections.emptyList()); - mPacProxyInstaller.setCurrentProxyScriptUrl(proxyInfo); + mPacProxyManager.setCurrentProxyScriptUrl(proxyInfo); if (!shouldSendBroadcast(proxyInfo)) { return; @@ -312,10 +331,10 @@ public class ProxyTracker { return; } - // This call could be coming from the PacProxyInstaller, containing the port of the + // This call could be coming from the PacProxyService, containing the port of the // local proxy. If this new proxy matches the global proxy then copy this proxy to the // global (to get the correct local port), and send a broadcast. - // TODO: Switch PacProxyInstaller to have its own message to send back rather than + // TODO: Switch PacProxyService to have its own message to send back rather than // reusing EVENT_HAS_CHANGED_PROXY and this call to handleApplyDefaultProxy. if ((mGlobalProxy != null) && (proxyInfo != null) && (!Uri.EMPTY.equals(proxyInfo.getPacFileUrl()))