From ce522835eace4fe2cb1d374ffe10afd308d6b473 Mon Sep 17 00:00:00 2001
From: Daichi Hirono <hirono@google.com>
Date: Fri, 17 Mar 2017 09:12:12 +0900
Subject: [PATCH] Stop holding mProxyLock while downloading PAC script data.

Holding PacManager#mProxyLock for long time eventually causes ANR when
launching new applications.

1. ActivityThread#handleBindApplication blocks until

   ActivityThread#handleBindApplication
   -> ConnectivityService#getProxyForNetwork()
   -> ConnectivityService#getDefaultProxy()

  obtains the lock of ConnectivityService#mProxyLock

2. ConnectivityService#mProxyLock can be held by
   ConnectivityService#setGlobalProxy() running on another thread until

   ConnectivityService#setGlobalProxy()
   -> ConnectivityService#sendProxyBroadcast()
   -> PacManager#setCurrentProxyScriptUrl()

  obtains the lock of PacManager#mProxyLock

3. Before the CL, PacManager#mProxyLock could be held by
   mPacDownloader#run() on mNetThread until downloading a PAC script
   completed.

The CL fixes the step 3 so that mPacDownloader#run does not keep the
lock of PacManager#mProxyLock. It eventually fixes long blocking at
ActivityThread#handleBindApplication and ANR when launching an
application.

Bug: 36317236
Test: Build succeeded
Merged-In: I864e41e1142178681f8f1b4fb7750d37e3ab2076
Change-Id: I864e41e1142178681f8f1b4fb7750d37e3ab2076
---
 .../server/connectivity/PacManager.java       | 34 +++++++++++--------
 1 file changed, 19 insertions(+), 15 deletions(-)

diff --git a/services/core/java/com/android/server/connectivity/PacManager.java b/services/core/java/com/android/server/connectivity/PacManager.java
index 58c76ec7674a9..46f76b1a1aec4 100644
--- a/services/core/java/com/android/server/connectivity/PacManager.java
+++ b/services/core/java/com/android/server/connectivity/PacManager.java
@@ -15,6 +15,7 @@
  */
 package com.android.server.connectivity;
 
+import android.annotation.WorkerThread;
 import android.app.AlarmManager;
 import android.app.PendingIntent;
 import android.content.BroadcastReceiver;
@@ -73,7 +74,7 @@ public class PacManager {
     public static final String KEY_PROXY = "keyProxy";
     private String mCurrentPac;
     @GuardedBy("mProxyLock")
-    private Uri mPacUrl = Uri.EMPTY;
+    private volatile Uri mPacUrl = Uri.EMPTY;
 
     private AlarmManager mAlarmManager;
     @GuardedBy("mProxyLock")
@@ -86,29 +87,34 @@ public class PacManager {
     private int mCurrentDelay;
     private int mLastPort;
 
-    private boolean mHasSentBroadcast;
-    private boolean mHasDownloaded;
+    private volatile boolean mHasSentBroadcast;
+    private volatile boolean mHasDownloaded;
 
     private Handler mConnectivityHandler;
     private int mProxyMessage;
 
     /**
-     * Used for locking when setting mProxyService and all references to mPacUrl or mCurrentPac.
+     * Used for locking when setting mProxyService and all references to mCurrentPac.
      */
     private final Object mProxyLock = new Object();
 
+    /**
+     * Runnable to download PAC script.
+     * The behavior relies on the assamption it always run on mNetThread to guarantee that the
+     * latest data fetched from mPacUrl is stored in mProxyService.
+     */
     private Runnable mPacDownloader = new Runnable() {
         @Override
+        @WorkerThread
         public void run() {
             String file;
-            synchronized (mProxyLock) {
-                if (Uri.EMPTY.equals(mPacUrl)) return;
-                try {
-                    file = get(mPacUrl);
-                } catch (IOException ioe) {
-                    file = null;
-                    Log.w(TAG, "Failed to load PAC file: " + ioe);
-                }
+            final Uri pacUrl = mPacUrl;
+            if (Uri.EMPTY.equals(pacUrl)) return;
+            try {
+                file = get(pacUrl);
+            } catch (IOException ioe) {
+                file = null;
+                Log.w(TAG, "Failed to load PAC file: " + ioe);
             }
             if (file != null) {
                 synchronized (mProxyLock) {
@@ -171,9 +177,7 @@ public class PacManager {
                 // Allow to send broadcast, nothing to do.
                 return false;
             }
-            synchronized (mProxyLock) {
-                mPacUrl = proxy.getPacFileUrl();
-            }
+            mPacUrl = proxy.getPacFileUrl();
             mCurrentDelay = DELAY_1;
             mHasSentBroadcast = false;
             mHasDownloaded = false;