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

2 files changed, 47 insertions, 37 deletions

diff --git a/services/core/java/com/android/server/connectivity/PacProxyInstaller.java b/services/core/java/com/android/server/connectivity/PacProxyInstaller.java
index aadaf4d9584f..5dc8c1a00eaf 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 @@ public class PacProxyInstaller {
     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 @@ public class PacProxyInstaller {
     private volatile boolean mHasSentBroadcast;
     private volatile boolean mHasDownloaded;
 
-    private Handler mConnectivityHandler;
+    private final Handler mConnectivityHandler;
     private final int mProxyMessage;
 
     /**
@@ -102,6 +99,13 @@ public class PacProxyInstaller {
     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 class PacProxyInstaller {
         }
     }
 
-    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 @@ public class PacProxyInstaller {
      * 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 @@ public class PacProxyInstaller {
         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 class PacProxyInstaller {
                             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 @@ public class PacProxyInstaller {
         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 d83ff837d9be..154055a4250b 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.
      *