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
diff --git a/services/core/java/com/android/server/connectivity/PacProxyInstaller.java b/services/core/java/com/android/server/connectivity/PacProxyInstaller.java
index aadaf4d..5dc8c1a 100644
--- a/services/core/java/com/android/server/connectivity/PacProxyInstaller.java
+++ b/services/core/java/com/android/server/connectivity/PacProxyInstaller.java
@@ -16,6 +16,7 @@
package com.android.server.connectivity;
+import android.annotation.NonNull;
import android.annotation.WorkerThread;
import android.app.AlarmManager;
import android.app.PendingIntent;
@@ -71,10 +72,6 @@
private static final int DELAY_LONG = 4;
private static final long MAX_PAC_SIZE = 20 * 1000 * 1000;
- // Return values for #setCurrentProxyScriptUrl
- public static final boolean DONT_SEND_BROADCAST = false;
- public static final boolean DO_SEND_BROADCAST = true;
-
private String mCurrentPac;
@GuardedBy("mProxyLock")
private volatile Uri mPacUrl = Uri.EMPTY;
@@ -93,7 +90,7 @@
private volatile boolean mHasSentBroadcast;
private volatile boolean mHasDownloaded;
- private Handler mConnectivityHandler;
+ private final Handler mConnectivityHandler;
private final int mProxyMessage;
/**
@@ -102,6 +99,13 @@
private final Object mProxyLock = new Object();
/**
+ * Lock ensuring consistency between the values of mHasSentBroadcast, mHasDownloaded, the
+ * last URL and port, and the broadcast message being sent with the correct arguments.
+ * TODO : this should probably protect all instances of these variables
+ */
+ private final Object mBroadcastStateLock = new Object();
+
+ /**
* Runnable to download PAC script.
* The behavior relies on the assumption it always runs on mNetThread to guarantee that the
* latest data fetched from mPacUrl is stored in mProxyService.
@@ -146,7 +150,7 @@
}
}
- public PacProxyInstaller(Context context, Handler handler, int proxyMessage) {
+ public PacProxyInstaller(@NonNull Context context, @NonNull Handler handler, int proxyMessage) {
mContext = context;
mLastPort = -1;
final HandlerThread netThread = new HandlerThread("android.pacproxyinstaller",
@@ -176,31 +180,27 @@
* PacProxyInstaller will trigger a new broadcast when it is ready.
*
* @param proxy Proxy information that is about to be broadcast.
- * @return Returns whether the broadcast should be sent : either DO_ or DONT_SEND_BROADCAST
*/
- public synchronized boolean setCurrentProxyScriptUrl(ProxyInfo proxy) {
- if (!Uri.EMPTY.equals(proxy.getPacFileUrl())) {
- if (proxy.getPacFileUrl().equals(mPacUrl) && (proxy.getPort() > 0)) {
- // Allow to send broadcast, nothing to do.
- return DO_SEND_BROADCAST;
- }
- mPacUrl = proxy.getPacFileUrl();
- mCurrentDelay = DELAY_1;
- mHasSentBroadcast = false;
- mHasDownloaded = false;
- getAlarmManager().cancel(mPacRefreshIntent);
- bind();
- return DONT_SEND_BROADCAST;
- } else {
- getAlarmManager().cancel(mPacRefreshIntent);
- synchronized (mProxyLock) {
- mPacUrl = Uri.EMPTY;
- mCurrentPac = null;
- if (mProxyService != null) {
- unbind();
+ public void setCurrentProxyScriptUrl(@NonNull ProxyInfo proxy) {
+ synchronized (mBroadcastStateLock) {
+ if (!Uri.EMPTY.equals(proxy.getPacFileUrl())) {
+ if (proxy.getPacFileUrl().equals(mPacUrl) && (proxy.getPort() > 0)) return;
+ mPacUrl = proxy.getPacFileUrl();
+ mCurrentDelay = DELAY_1;
+ mHasSentBroadcast = false;
+ mHasDownloaded = false;
+ getAlarmManager().cancel(mPacRefreshIntent);
+ bind();
+ } else {
+ getAlarmManager().cancel(mPacRefreshIntent);
+ synchronized (mProxyLock) {
+ mPacUrl = Uri.EMPTY;
+ mCurrentPac = null;
+ if (mProxyService != null) {
+ unbind();
+ }
}
}
- return DO_SEND_BROADCAST;
}
}
@@ -275,6 +275,7 @@
getAlarmManager().set(AlarmManager.ELAPSED_REALTIME, timeTillTrigger, mPacRefreshIntent);
}
+ @GuardedBy("mProxyLock")
private void setCurrentProxyScript(String script) {
if (mProxyService == null) {
Log.e(TAG, "setCurrentProxyScript: no proxy service");
@@ -347,6 +348,9 @@
public void setProxyPort(int port) {
if (mLastPort != -1) {
// Always need to send if port changed
+ // TODO: Here lacks synchronization because this write cannot
+ // guarantee that it's visible from sendProxyIfNeeded() when
+ // it's called by a Runnable which is post by mNetThread.
mHasSentBroadcast = false;
}
mLastPort = port;
@@ -386,13 +390,15 @@
mConnectivityHandler.sendMessage(mConnectivityHandler.obtainMessage(mProxyMessage, proxy));
}
- private synchronized void sendProxyIfNeeded() {
- if (!mHasDownloaded || (mLastPort == -1)) {
- return;
- }
- if (!mHasSentBroadcast) {
- sendPacBroadcast(ProxyInfo.buildPacProxy(mPacUrl, mLastPort));
- mHasSentBroadcast = true;
+ private void sendProxyIfNeeded() {
+ synchronized (mBroadcastStateLock) {
+ if (!mHasDownloaded || (mLastPort == -1)) {
+ return;
+ }
+ if (!mHasSentBroadcast) {
+ sendPacBroadcast(ProxyInfo.buildPacProxy(mPacUrl, mLastPort));
+ mHasSentBroadcast = true;
+ }
}
}
}
diff --git a/services/core/java/com/android/server/connectivity/ProxyTracker.java b/services/core/java/com/android/server/connectivity/ProxyTracker.java
index d83ff83..154055a 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 @@
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 @@
}
}
+ 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.
*
