Fix Bug in ImsManager#addRegistrationListener

Listeners were not being unregistered from
the ImsService. The listeners were restructured
to instead be held in ImsManager, instead of the
ImsService.

Bug: 38304804
Test: Run Rogers VoWiFi activation
Merged-In: I2a0d4b2ed7d232834be7c74c318a60fa11e64389
Change-Id: I2a0d4b2ed7d232834be7c74c318a60fa11e64389

1 files changed, 79 insertions, 69 deletions

diff --git a/src/java/com/android/ims/ImsManager.java b/src/java/com/android/ims/ImsManager.java
index 214753fa..7c1c1822 100644
--- a/src/java/com/android/ims/ImsManager.java
+++ b/src/java/com/android/ims/ImsManager.java
@@ -204,7 +204,15 @@ public class ImsManager {
 
     // Keep track of the ImsRegistrationListenerProxys that have been created so that we can
     // remove them from the ImsService.
-    private Set<ImsRegistrationListenerProxy> mRegistrationListeners = new HashSet<>();
+    private final Set<ImsConnectionStateListener> mRegistrationListeners = new HashSet<>();
+
+    private final ImsRegistrationListenerProxy mRegistrationListenerProxy =
+            new ImsRegistrationListenerProxy();
+
+    // When true, we have registered the mRegistrationListenerProxy with the ImsService. Don't do
+    // it again.
+    private boolean mHasRegisteredForProxy = false;
+    private final Object mHasRegisteredLock = new Object();
 
     // SystemProperties used as cache
     private static final String VOLTE_PROVISIONED_PROP = "net.lte.ims.volte.provisioned";
@@ -1585,7 +1593,11 @@ public class ImsManager {
 
         try {
             result = mImsServiceProxy.startSession(incomingCallPendingIntent,
-                    createRegistrationListenerProxy(serviceClass, listener));
+                    mRegistrationListenerProxy);
+            synchronized (mHasRegisteredLock) {
+                mHasRegisteredForProxy = true;
+                addRegistrationListener(listener);
+            }
         } catch (RemoteException e) {
             throw new ImsException("open()", e,
                     ImsReasonInfo.CODE_LOCAL_IMS_SERVICE_DOWN);
@@ -1609,23 +1621,43 @@ public class ImsManager {
      * @param listener To listen to IMS registration events; It cannot be null
      * @throws NullPointerException if {@code listener} is null
      * @throws ImsException if calling the IMS service results in an error
+     *
+     * @deprecated Use {@link #addRegistrationListener(ImsConnectionStateListener)} instead.
      */
     public void addRegistrationListener(int serviceClass, ImsConnectionStateListener listener)
             throws ImsException {
+        addRegistrationListener(listener);
+    }
+
+    /**
+     * Adds registration listener to the IMS service.
+     *
+     * @param listener To listen to IMS registration events; It cannot be null
+     * @throws NullPointerException if {@code listener} is null
+     * @throws ImsException if calling the IMS service results in an error
+     */
+    public void addRegistrationListener(ImsConnectionStateListener listener)
+            throws ImsException {
         checkAndThrowExceptionIfServiceUnavailable();
 
         if (listener == null) {
             throw new NullPointerException("listener can't be null");
         }
-
-        try {
-            ImsRegistrationListenerProxy p = createRegistrationListenerProxy(serviceClass,
-                    listener);
-            mRegistrationListeners.add(p);
-            mImsServiceProxy.addRegistrationListener(p);
-        } catch (RemoteException e) {
-            throw new ImsException("addRegistrationListener()", e,
-                    ImsReasonInfo.CODE_LOCAL_IMS_SERVICE_DOWN);
+        // We only want this Proxy registered once. It can either happen here or in open().
+        synchronized (mHasRegisteredLock) {
+            if (!mHasRegisteredForProxy) {
+                try {
+                    mImsServiceProxy.addRegistrationListener(mRegistrationListenerProxy);
+                    // Only record if there isn't a RemoteException.
+                    mHasRegisteredForProxy = true;
+                } catch (RemoteException e) {
+                    throw new ImsException("addRegistrationListener()", e,
+                            ImsReasonInfo.CODE_LOCAL_IMS_SERVICE_DOWN);
+                }
+            }
+        }
+        synchronized (mRegistrationListeners) {
+            mRegistrationListeners.add(listener);
         }
     }
 
@@ -1639,23 +1671,12 @@ public class ImsManager {
      */
     public void removeRegistrationListener(ImsConnectionStateListener listener)
             throws ImsException {
-        checkAndThrowExceptionIfServiceUnavailable();
-
         if (listener == null) {
             throw new NullPointerException("listener can't be null");
         }
 
-        try {
-            Optional<ImsRegistrationListenerProxy> optionalProxy = mRegistrationListeners.stream()
-                    .filter(l -> listener.equals(l.mListener)).findFirst();
-            if(optionalProxy.isPresent()) {
-                ImsRegistrationListenerProxy p = optionalProxy.get();
-                mRegistrationListeners.remove(p);
-                mImsServiceProxy.removeRegistrationListener(p);
-            }
-        } catch (RemoteException e) {
-            throw new ImsException("removeRegistrationListener()", e,
-                    ImsReasonInfo.CODE_LOCAL_IMS_SERVICE_DOWN);
+        synchronized (mRegistrationListeners) {
+            mRegistrationListeners.remove(listener);
         }
     }
 
@@ -2120,6 +2141,10 @@ public class ImsManager {
             Rlog.i(TAG, "Creating ImsService using ImsResolver");
             mImsServiceProxy = getServiceProxy();
         }
+        // We have created a new ImsService connection, signal for re-registration
+        synchronized (mHasRegisteredLock) {
+            mHasRegisteredForProxy = false;
+        }
     }
 
     // Deprecated method of binding with the ImsService defined in the ServiceManager.
@@ -2177,13 +2202,6 @@ public class ImsManager {
         }
     }
 
-    private ImsRegistrationListenerProxy createRegistrationListenerProxy(int serviceClass,
-            ImsConnectionStateListener listener) {
-        ImsRegistrationListenerProxy proxy =
-                new ImsRegistrationListenerProxy(serviceClass, listener);
-        return proxy;
-    }
-
     private static void log(String s) {
         Rlog.d(TAG, s);
     }
@@ -2298,18 +2316,6 @@ public class ImsManager {
      * Adapter class for {@link IImsRegistrationListener}.
      */
     private class ImsRegistrationListenerProxy extends IImsRegistrationListener.Stub {
-        private int mServiceClass;
-        private ImsConnectionStateListener mListener;
-
-        public ImsRegistrationListenerProxy(int serviceClass,
-                ImsConnectionStateListener listener) {
-            mServiceClass = serviceClass;
-            mListener = listener;
-        }
-
-        public boolean isSameProxy(int serviceClass) {
-            return (mServiceClass == serviceClass);
-        }
 
         @Deprecated
         public void registrationConnected() {
@@ -2317,8 +2323,9 @@ public class ImsManager {
                 log("registrationConnected ::");
             }
 
-            if (mListener != null) {
-                mListener.onImsConnected(ServiceState.RIL_RADIO_TECHNOLOGY_UNKNOWN);
+            synchronized (mRegistrationListeners) {
+                mRegistrationListeners.forEach(l -> l.onImsConnected(
+                        ServiceState.RIL_RADIO_TECHNOLOGY_UNKNOWN));
             }
         }
 
@@ -2328,8 +2335,9 @@ public class ImsManager {
                 log("registrationProgressing ::");
             }
 
-            if (mListener != null) {
-                mListener.onImsProgressing(ServiceState.RIL_RADIO_TECHNOLOGY_UNKNOWN);
+            synchronized (mRegistrationListeners) {
+                mRegistrationListeners.forEach(l -> l.onImsProgressing(
+                        ServiceState.RIL_RADIO_TECHNOLOGY_UNKNOWN));
             }
         }
 
@@ -2341,8 +2349,8 @@ public class ImsManager {
                 log("registrationConnectedWithRadioTech :: imsRadioTech=" + imsRadioTech);
             }
 
-            if (mListener != null) {
-                mListener.onImsConnected(imsRadioTech);
+            synchronized (mRegistrationListeners) {
+                mRegistrationListeners.forEach(l -> l.onImsConnected(imsRadioTech));
             }
         }
 
@@ -2354,8 +2362,8 @@ public class ImsManager {
                 log("registrationProgressingWithRadioTech :: imsRadioTech=" + imsRadioTech);
             }
 
-            if (mListener != null) {
-                mListener.onImsProgressing(imsRadioTech);
+            synchronized (mRegistrationListeners) {
+                mRegistrationListeners.forEach(l -> l.onImsProgressing(imsRadioTech));
             }
         }
 
@@ -2366,9 +2374,8 @@ public class ImsManager {
             }
 
             addToRecentDisconnectReasons(imsReasonInfo);
-
-            if (mListener != null) {
-                mListener.onImsDisconnected(imsReasonInfo);
+            synchronized (mRegistrationListeners) {
+                mRegistrationListeners.forEach(l -> l.onImsDisconnected(imsReasonInfo));
             }
         }
 
@@ -2378,8 +2385,8 @@ public class ImsManager {
                 log("registrationResumed ::");
             }
 
-            if (mListener != null) {
-                mListener.onImsResumed();
+            synchronized (mRegistrationListeners) {
+                mRegistrationListeners.forEach(ImsConnectionStateListener::onImsResumed);
             }
         }
 
@@ -2389,8 +2396,8 @@ public class ImsManager {
                 log("registrationSuspended ::");
             }
 
-            if (mListener != null) {
-                mListener.onImsSuspended();
+            synchronized (mRegistrationListeners) {
+                mRegistrationListeners.forEach(ImsConnectionStateListener::onImsSuspended);
             }
         }
 
@@ -2399,8 +2406,9 @@ public class ImsManager {
             log("registrationServiceCapabilityChanged :: serviceClass=" +
                     serviceClass + ", event=" + event);
 
-            if (mListener != null) {
-                mListener.onImsConnected(ServiceState.RIL_RADIO_TECHNOLOGY_UNKNOWN);
+            synchronized (mRegistrationListeners) {
+                mRegistrationListeners.forEach(l -> l.onImsConnected(
+                        ServiceState.RIL_RADIO_TECHNOLOGY_UNKNOWN));
             }
         }
 
@@ -2409,9 +2417,10 @@ public class ImsManager {
                 int[] enabledFeatures, int[] disabledFeatures) {
             log("registrationFeatureCapabilityChanged :: serviceClass=" +
                     serviceClass);
-            if (mListener != null) {
-                mListener.onFeatureCapabilityChanged(serviceClass,
-                        enabledFeatures, disabledFeatures);
+
+            synchronized (mRegistrationListeners) {
+                mRegistrationListeners.forEach(l -> l.onFeatureCapabilityChanged(serviceClass,
+                        enabledFeatures, disabledFeatures));
             }
         }
 
@@ -2419,8 +2428,8 @@ public class ImsManager {
         public void voiceMessageCountUpdate(int count) {
             log("voiceMessageCountUpdate :: count=" + count);
 
-            if (mListener != null) {
-                mListener.onVoiceMessageCountChanged(count);
+            synchronized (mRegistrationListeners) {
+                mRegistrationListeners.forEach(l -> l.onVoiceMessageCountChanged(count));
             }
         }
 
@@ -2428,8 +2437,8 @@ public class ImsManager {
         public void registrationAssociatedUriChanged(Uri[] uris) {
             if (DBG) log("registrationAssociatedUriChanged ::");
 
-            if (mListener != null) {
-                mListener.registrationAssociatedUriChanged(uris);
+            synchronized (mRegistrationListeners) {
+                mRegistrationListeners.forEach(l -> l.registrationAssociatedUriChanged(uris));
             }
         }
 
@@ -2438,8 +2447,9 @@ public class ImsManager {
             if (DBG) log("registrationChangeFailed :: targetAccessTech=" + targetAccessTech +
                     ", imsReasonInfo=" + imsReasonInfo);
 
-            if (mListener != null) {
-                mListener.onRegistrationChangeFailed(targetAccessTech, imsReasonInfo);
+            synchronized (mRegistrationListeners) {
+                mRegistrationListeners.forEach(l -> l.onRegistrationChangeFailed(targetAccessTech,
+                        imsReasonInfo));
             }
         }
     }