From d08b3a68cc39738b5881d6509c5b950b6a3bee18 Mon Sep 17 00:00:00 2001
From: "Philip P. Moltmann" <moltmann@google.com>
Date: Fri, 10 Jul 2020 11:41:53 -0700
Subject: [PATCH] Fix locking in ServiceListener

- Use Android framework locking style
- Make sure each and ever access is protected

Test: Ran the service
Fixes: 159418766
Change-Id: I0d884f5aa1644f694f1c4f9cc9f817ebb8cbe110
---
 .../plugin/hp/ServiceListener.java            | 157 ++++++++++--------
 1 file changed, 89 insertions(+), 68 deletions(-)

diff --git a/packages/PrintRecommendationService/src/com/android/printservice/recommendation/plugin/hp/ServiceListener.java b/packages/PrintRecommendationService/src/com/android/printservice/recommendation/plugin/hp/ServiceListener.java
index 9535ef06f888b..348c971e17cce 100644
--- a/packages/PrintRecommendationService/src/com/android/printservice/recommendation/plugin/hp/ServiceListener.java
+++ b/packages/PrintRecommendationService/src/com/android/printservice/recommendation/plugin/hp/ServiceListener.java
@@ -22,6 +22,8 @@ import android.net.nsd.NsdManager;
 import android.net.nsd.NsdServiceInfo;
 import android.text.TextUtils;
 
+import androidx.annotation.GuardedBy;
+
 import com.android.printservice.recommendation.R;
 import com.android.printservice.recommendation.util.DiscoveryListenerMultiplexer;
 import com.android.printservice.recommendation.util.PrinterHashMap;
@@ -40,7 +42,12 @@ public class ServiceListener implements ServiceResolveQueue.ResolveCallback {
     private final String[] mServiceType;
     private final Observer mObserver;
     private final ServiceResolveQueue mResolveQueue;
+    private final Object mLock = new Object();
+
+    @GuardedBy("mLock")
     private List<NsdManager.DiscoveryListener> mListeners = new ArrayList<>();
+
+    @GuardedBy("mLock")
     public HashMap<String, PrinterHashMap> mVendorHashMap = new HashMap<>();
 
     public interface Observer {
@@ -73,108 +80,120 @@ public class ServiceListener implements ServiceResolveQueue.ResolveCallback {
         printerFound(nsdServiceInfo);
     }
 
-    private synchronized void printerFound(NsdServiceInfo nsdServiceInfo) {
+    private void printerFound(NsdServiceInfo nsdServiceInfo) {
         if (nsdServiceInfo == null) return;
         if (TextUtils.isEmpty(PrinterHashMap.getKey(nsdServiceInfo))) return;
         String vendor = MDnsUtils.getVendor(nsdServiceInfo);
         if (vendor == null) vendor = "";
-        for(Map.Entry<String,VendorInfo> entry : mVendorInfoHashMap.entrySet()) {
-            for(String vendorValues : entry.getValue().mDNSValues) {
-                if (vendor.equalsIgnoreCase(vendorValues)) {
-                    vendor = entry.getValue().mVendorID;
-                    break;
-                }
-            }
-            // intentional pointer check
-            //noinspection StringEquality
-            if ((vendor != entry.getValue().mVendorID) &&
-                    MDnsUtils.isVendorPrinter(nsdServiceInfo, entry.getValue().mDNSValues)) {
-                vendor = entry.getValue().mVendorID;
-            }
-            // intentional pointer check
-            //noinspection StringEquality
-            if (vendor == entry.getValue().mVendorID) break;
-        }
 
-        if (TextUtils.isEmpty(vendor)) {
-            return;
-        }
-
-        if (!mObserver.matchesCriteria(vendor, nsdServiceInfo))
-            return;
         boolean mapsChanged;
+        synchronized (mLock) {
+            for (Map.Entry<String, VendorInfo> entry : mVendorInfoHashMap.entrySet()) {
+                for (String vendorValues : entry.getValue().mDNSValues) {
+                    if (vendor.equalsIgnoreCase(vendorValues)) {
+                        vendor = entry.getValue().mVendorID;
+                        break;
+                    }
+                }
+                // intentional pointer check
+                //noinspection StringEquality
+                if ((vendor != entry.getValue().mVendorID) &&
+                        MDnsUtils.isVendorPrinter(nsdServiceInfo, entry.getValue().mDNSValues)) {
+                    vendor = entry.getValue().mVendorID;
+                }
+                // intentional pointer check
+                //noinspection StringEquality
+                if (vendor == entry.getValue().mVendorID) break;
+            }
 
-        PrinterHashMap vendorHash = mVendorHashMap.get(vendor);
-        if (vendorHash == null) {
-            vendorHash = new PrinterHashMap();
+            if (TextUtils.isEmpty(vendor)) {
+                return;
+            }
+
+            if (!mObserver.matchesCriteria(vendor, nsdServiceInfo))
+                return;
+
+            PrinterHashMap vendorHash = mVendorHashMap.get(vendor);
+            if (vendorHash == null) {
+                vendorHash = new PrinterHashMap();
+            }
+            mapsChanged = (vendorHash.addPrinter(nsdServiceInfo) == null);
+            mVendorHashMap.put(vendor, vendorHash);
         }
-        mapsChanged = (vendorHash.addPrinter(nsdServiceInfo) == null);
-        mVendorHashMap.put(vendor, vendorHash);
 
         if (mapsChanged) {
             mObserver.dataSetChanged();
         }
     }
 
-    private synchronized void printerRemoved(NsdServiceInfo nsdServiceInfo) {
+    private void printerRemoved(NsdServiceInfo nsdServiceInfo) {
         boolean wasRemoved = false;
-        Set<String> vendors = mVendorHashMap.keySet();
-        for(String vendor : vendors) {
-            PrinterHashMap map = mVendorHashMap.get(vendor);
-            wasRemoved |= (map.removePrinter(nsdServiceInfo) != null);
-            if (map.isEmpty()) wasRemoved |= (mVendorHashMap.remove(vendor) != null);
+
+        synchronized (mLock) {
+            Set<String> vendors = mVendorHashMap.keySet();
+            for (String vendor : vendors) {
+                PrinterHashMap map = mVendorHashMap.get(vendor);
+                wasRemoved |= (map.removePrinter(nsdServiceInfo) != null);
+                if (map.isEmpty()) wasRemoved |= (mVendorHashMap.remove(vendor) != null);
+            }
         }
+
         if (wasRemoved) {
             mObserver.dataSetChanged();
         }
     }
 
     public void start() {
-        stop();
-        for(final String service :mServiceType) {
-            NsdManager.DiscoveryListener listener = new NsdManager.DiscoveryListener() {
-                @Override
-                public void onStartDiscoveryFailed(String s, int i) {
+        synchronized (mLock) {
+            stop();
 
-                }
+            for (final String service : mServiceType) {
+                NsdManager.DiscoveryListener listener = new NsdManager.DiscoveryListener() {
+                    @Override
+                    public void onStartDiscoveryFailed(String s, int i) {
 
-                @Override
-                public void onStopDiscoveryFailed(String s, int i) {
+                    }
 
-                }
+                    @Override
+                    public void onStopDiscoveryFailed(String s, int i) {
 
-                @Override
-                public void onDiscoveryStarted(String s) {
+                    }
 
-                }
+                    @Override
+                    public void onDiscoveryStarted(String s) {
 
-                @Override
-                public void onDiscoveryStopped(String s) {
+                    }
 
-                }
+                    @Override
+                    public void onDiscoveryStopped(String s) {
 
-                @Override
-                public void onServiceFound(NsdServiceInfo nsdServiceInfo) {
-                    mResolveQueue.queueRequest(nsdServiceInfo, ServiceListener.this);
-                }
+                    }
 
-                @Override
-                public void onServiceLost(NsdServiceInfo nsdServiceInfo) {
-                    mResolveQueue.removeRequest(nsdServiceInfo, ServiceListener.this);
-                    printerRemoved(nsdServiceInfo);
-                }
-            };
-            DiscoveryListenerMultiplexer.addListener(mNSDManager, service, listener);
-            mListeners.add(listener);
+                    @Override
+                    public void onServiceFound(NsdServiceInfo nsdServiceInfo) {
+                        mResolveQueue.queueRequest(nsdServiceInfo, ServiceListener.this);
+                    }
+
+                    @Override
+                    public void onServiceLost(NsdServiceInfo nsdServiceInfo) {
+                        mResolveQueue.removeRequest(nsdServiceInfo, ServiceListener.this);
+                        printerRemoved(nsdServiceInfo);
+                    }
+                };
+                DiscoveryListenerMultiplexer.addListener(mNSDManager, service, listener);
+                mListeners.add(listener);
+            }
         }
     }
 
     public void stop() {
-        for(NsdManager.DiscoveryListener listener : mListeners) {
-            DiscoveryListenerMultiplexer.removeListener(mNSDManager, listener);
+        synchronized (mLock) {
+            for (NsdManager.DiscoveryListener listener : mListeners) {
+                DiscoveryListenerMultiplexer.removeListener(mNSDManager, listener);
+            }
+            mVendorHashMap.clear();
+            mListeners.clear();
         }
-        mVendorHashMap.clear();
-        mListeners.clear();
     }
 
     /**
@@ -183,8 +202,10 @@ public class ServiceListener implements ServiceResolveQueue.ResolveCallback {
     public ArrayList<InetAddress> getPrinters() {
         ArrayList<InetAddress> printerAddressess = new ArrayList<>();
 
-        for (PrinterHashMap oneVendorPrinters : mVendorHashMap.values()) {
-            printerAddressess.addAll(oneVendorPrinters.getPrinterAddresses());
+        synchronized (mLock) {
+            for (PrinterHashMap oneVendorPrinters : mVendorHashMap.values()) {
+                printerAddressess.addAll(oneVendorPrinters.getPrinterAddresses());
+            }
         }
 
         return printerAddressess;