Merge "Add removeListener, update tests, hide binder"

5 files changed, 228 insertions, 33 deletions

diff --git a/core/java/android/app/timezonedetector/ITimeZoneDetectorService.aidl b/core/java/android/app/timezonedetector/ITimeZoneDetectorService.aidl
index d8675f39d452..6e93af6a053b 100644
--- a/core/java/android/app/timezonedetector/ITimeZoneDetectorService.aidl
+++ b/core/java/android/app/timezonedetector/ITimeZoneDetectorService.aidl
@@ -41,6 +41,7 @@ interface ITimeZoneDetectorService {
   TimeZoneConfiguration getConfiguration();
   boolean updateConfiguration(in TimeZoneConfiguration configuration);
   void addConfigurationListener(ITimeZoneConfigurationListener listener);
+  void removeConfigurationListener(ITimeZoneConfigurationListener listener);
 
   boolean suggestManualTimeZone(in ManualTimeZoneSuggestion timeZoneSuggestion);
   void suggestTelephonyTimeZone(in TelephonyTimeZoneSuggestion timeZoneSuggestion);
diff --git a/core/java/android/app/timezonedetector/TimeZoneDetector.java b/core/java/android/app/timezonedetector/TimeZoneDetector.java
index 621ef526aacf..7885613bfb59 100644
--- a/core/java/android/app/timezonedetector/TimeZoneDetector.java
+++ b/core/java/android/app/timezonedetector/TimeZoneDetector.java
@@ -74,11 +74,26 @@ public interface TimeZoneDetector {
     boolean updateConfiguration(@NonNull TimeZoneConfiguration configuration);
 
     /**
+     * An interface that can be used to listen for changes to the time zone detector configuration.
+     */
+    interface TimeZoneConfigurationListener {
+        /** Called when the configuration changes. There are no guarantees about the thread used. */
+        void onChange(@NonNull TimeZoneConfiguration configuration);
+    }
+
+    /**
      * Registers a listener that will be informed when the configuration changes. The complete
      * configuration is passed to the listener, not just the properties that have changed.
      */
     @RequiresPermission(android.Manifest.permission.WRITE_SECURE_SETTINGS)
-    void addConfigurationListener(@NonNull ITimeZoneConfigurationListener listener);
+    void addConfigurationListener(@NonNull TimeZoneConfigurationListener listener);
+
+    /**
+     * Removes a listener previously passed to
+     * {@link #addConfigurationListener(ITimeZoneConfigurationListener)}
+     */
+    @RequiresPermission(android.Manifest.permission.WRITE_SECURE_SETTINGS)
+    void removeConfigurationListener(@NonNull TimeZoneConfigurationListener listener);
 
     /**
      * A shared utility method to create a {@link ManualTimeZoneSuggestion}.
diff --git a/core/java/android/app/timezonedetector/TimeZoneDetectorImpl.java b/core/java/android/app/timezonedetector/TimeZoneDetectorImpl.java
index 978cb218fbba..6bd365fad6f6 100644
--- a/core/java/android/app/timezonedetector/TimeZoneDetectorImpl.java
+++ b/core/java/android/app/timezonedetector/TimeZoneDetectorImpl.java
@@ -21,6 +21,7 @@ import android.content.Context;
 import android.os.RemoteException;
 import android.os.ServiceManager;
 import android.os.ServiceManager.ServiceNotFoundException;
+import android.util.ArraySet;
 import android.util.Log;
 
 /**
@@ -34,13 +35,16 @@ public final class TimeZoneDetectorImpl implements TimeZoneDetector {
 
     private final ITimeZoneDetectorService mITimeZoneDetectorService;
 
+    private ITimeZoneConfigurationListener mConfigurationReceiver;
+    private ArraySet<TimeZoneConfigurationListener> mConfigurationListeners;
+
     public TimeZoneDetectorImpl() throws ServiceNotFoundException {
         mITimeZoneDetectorService = ITimeZoneDetectorService.Stub.asInterface(
                 ServiceManager.getServiceOrThrow(Context.TIME_ZONE_DETECTOR_SERVICE));
     }
 
     @Override
-   @NonNull
+    @NonNull
     public TimeZoneCapabilities getCapabilities() {
         if (DEBUG) {
             Log.d(TAG, "getCapabilities called");
@@ -78,14 +82,70 @@ public final class TimeZoneDetectorImpl implements TimeZoneDetector {
     }
 
     @Override
-    public void addConfigurationListener(@NonNull ITimeZoneConfigurationListener listener) {
+    public void addConfigurationListener(@NonNull TimeZoneConfigurationListener listener) {
         if (DEBUG) {
             Log.d(TAG, "addConfigurationListener called: " + listener);
         }
-        try {
-            mITimeZoneDetectorService.addConfigurationListener(listener);
-        } catch (RemoteException e) {
-            throw e.rethrowFromSystemServer();
+        synchronized (this) {
+            if (mConfigurationListeners.contains(listener)) {
+                return;
+            }
+            if (mConfigurationReceiver == null) {
+                ITimeZoneConfigurationListener iListener =
+                        new ITimeZoneConfigurationListener.Stub() {
+                    @Override
+                    public void onChange(@NonNull TimeZoneConfiguration configuration) {
+                        notifyConfigurationListeners(configuration);
+                    }
+                };
+                mConfigurationReceiver = iListener;
+            }
+            if (mConfigurationListeners == null) {
+                mConfigurationListeners = new ArraySet<>();
+            }
+
+            boolean wasEmpty = mConfigurationListeners.isEmpty();
+            mConfigurationListeners.add(listener);
+            if (wasEmpty) {
+                try {
+                    mITimeZoneDetectorService.addConfigurationListener(mConfigurationReceiver);
+                } catch (RemoteException e) {
+                    throw e.rethrowFromSystemServer();
+                }
+            }
+        }
+    }
+
+    private void notifyConfigurationListeners(@NonNull TimeZoneConfiguration configuration) {
+        ArraySet<TimeZoneConfigurationListener> configurationListeners;
+        synchronized (this) {
+            configurationListeners = new ArraySet<>(mConfigurationListeners);
+        }
+        int size = configurationListeners.size();
+        for (int i = 0; i < size; i++) {
+            configurationListeners.valueAt(i).onChange(configuration);
+        }
+    }
+
+    @Override
+    public void removeConfigurationListener(@NonNull TimeZoneConfigurationListener listener) {
+        if (DEBUG) {
+            Log.d(TAG, "removeConfigurationListener called: " + listener);
+        }
+
+        synchronized (this) {
+            if (mConfigurationListeners == null) {
+                return;
+            }
+            boolean wasEmpty = mConfigurationListeners.isEmpty();
+            mConfigurationListeners.remove(listener);
+            if (mConfigurationListeners.isEmpty() && !wasEmpty) {
+                try {
+                    mITimeZoneDetectorService.removeConfigurationListener(mConfigurationReceiver);
+                } catch (RemoteException e) {
+                    throw e.rethrowFromSystemServer();
+                }
+            }
         }
     }
 
diff --git a/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorService.java b/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorService.java
index d9415ce81636..3ec61fdda917 100644
--- a/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorService.java
+++ b/services/core/java/com/android/server/timezonedetector/TimeZoneDetectorService.java
@@ -49,6 +49,7 @@ import com.android.server.timezonedetector.TimeZoneDetectorStrategy.StrategyList
 import java.io.FileDescriptor;
 import java.io.PrintWriter;
 import java.util.ArrayList;
+import java.util.Iterator;
 import java.util.Objects;
 
 /**
@@ -188,20 +189,14 @@ public final class TimeZoneDetectorService extends ITimeZoneDetectorService.Stub
         int userId = UserHandle.getCallingUserId();
 
         ConfigListenerInfo listenerInfo = new ConfigListenerInfo(userId, listener);
-        final IBinder.DeathRecipient deathRecipient = new IBinder.DeathRecipient() {
-            @Override
-            public void binderDied() {
-                synchronized (mConfigurationListeners) {
-                    Slog.i(TAG, "Configuration listener died: " + listenerInfo);
-                    mConfigurationListeners.remove(listenerInfo);
-                }
-            }
-        };
 
         synchronized (mConfigurationListeners) {
+            if (mConfigurationListeners.contains(listenerInfo)) {
+                return;
+            }
             try {
-                // Remove the record of the listener if the client process dies.
-                listener.asBinder().linkToDeath(deathRecipient, 0 /* flags */);
+                // Ensure the reference to the listener is removed if the client process dies.
+                listenerInfo.linkToDeath();
 
                 // Only add the listener if we can linkToDeath().
                 mConfigurationListeners.add(listenerInfo);
@@ -211,6 +206,31 @@ public final class TimeZoneDetectorService extends ITimeZoneDetectorService.Stub
         }
     }
 
+    @Override
+    public void removeConfigurationListener(@NonNull ITimeZoneConfigurationListener listener) {
+        enforceManageTimeZoneDetectorConfigurationPermission();
+        Objects.requireNonNull(listener);
+        int userId = UserHandle.getCallingUserId();
+
+        synchronized (mConfigurationListeners) {
+            ConfigListenerInfo toRemove = new ConfigListenerInfo(userId, listener);
+            Iterator<ConfigListenerInfo> listenerIterator = mConfigurationListeners.iterator();
+            while (listenerIterator.hasNext()) {
+                ConfigListenerInfo currentListenerInfo = listenerIterator.next();
+                if (currentListenerInfo.equals(toRemove)) {
+                    listenerIterator.remove();
+
+                    // Stop listening for the client process to die.
+                    try {
+                        currentListenerInfo.unlinkToDeath();
+                    } catch (RemoteException e) {
+                        Slog.e(TAG, "Unable to unlinkToDeath() for listener=" + listener, e);
+                    }
+                }
+            }
+        }
+    }
+
     void handleConfigurationChanged() {
         // Note: we could trigger an async time zone detection operation here via a call to
         // handleAutoTimeZoneDetectionChanged(), but that is triggered in response to the underlying
@@ -319,7 +339,7 @@ public final class TimeZoneDetectorService extends ITimeZoneDetectorService.Stub
                 this, in, out, err, args, callback, resultReceiver);
     }
 
-    private static class ConfigListenerInfo {
+    private class ConfigListenerInfo implements IBinder.DeathRecipient {
         private final @UserIdInt int mUserId;
         private final ITimeZoneConfigurationListener mListener;
 
@@ -337,6 +357,40 @@ public final class TimeZoneDetectorService extends ITimeZoneDetectorService.Stub
             return mListener;
         }
 
+        void linkToDeath() throws RemoteException {
+            mListener.asBinder().linkToDeath(this, 0 /* flags */);
+        }
+
+        void unlinkToDeath() throws RemoteException {
+            mListener.asBinder().unlinkToDeath(this, 0 /* flags */);
+        }
+
+        @Override
+        public void binderDied() {
+            synchronized (mConfigurationListeners) {
+                Slog.i(TAG, "Configuration listener client died: " + this);
+                mConfigurationListeners.remove(this);
+            }
+        }
+
+        @Override
+        public boolean equals(Object o) {
+            if (this == o) {
+                return true;
+            }
+            if (o == null || getClass() != o.getClass()) {
+                return false;
+            }
+            ConfigListenerInfo that = (ConfigListenerInfo) o;
+            return mUserId == that.mUserId
+                    && mListener.equals(that.mListener);
+        }
+
+        @Override
+        public int hashCode() {
+            return Objects.hash(mUserId, mListener);
+        }
+
         @Override
         public String toString() {
             return "ConfigListenerInfo{"
diff --git a/services/tests/servicestests/src/com/android/server/timezonedetector/TimeZoneDetectorServiceTest.java b/services/tests/servicestests/src/com/android/server/timezonedetector/TimeZoneDetectorServiceTest.java
index 971d2e28b14e..153634548c17 100644
--- a/services/tests/servicestests/src/com/android/server/timezonedetector/TimeZoneDetectorServiceTest.java
+++ b/services/tests/servicestests/src/com/android/server/timezonedetector/TimeZoneDetectorServiceTest.java
@@ -21,12 +21,16 @@ import static android.app.timezonedetector.TimeZoneCapabilities.CAPABILITY_POSSE
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.fail;
 import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;
 import static org.mockito.ArgumentMatchers.anyString;
 import static org.mockito.ArgumentMatchers.eq;
 import static org.mockito.Mockito.doNothing;
 import static org.mockito.Mockito.doThrow;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.verify;
+import static org.mockito.Mockito.verifyNoMoreInteractions;
 import static org.mockito.Mockito.when;
 
 import android.app.timezonedetector.ITimeZoneConfigurationListener;
@@ -158,10 +162,24 @@ public class TimeZoneDetectorServiceTest {
         }
     }
 
+    @Test(expected = SecurityException.class)
+    public void testRemoveConfigurationListener_withoutPermission() {
+        doThrow(new SecurityException("Mock"))
+                .when(mMockContext).enforceCallingPermission(anyString(), any());
+
+        ITimeZoneConfigurationListener mockListener = mock(ITimeZoneConfigurationListener.class);
+        try {
+            mTimeZoneDetectorService.removeConfigurationListener(mockListener);
+            fail();
+        } finally {
+            verify(mMockContext).enforceCallingPermission(
+                    eq(android.Manifest.permission.WRITE_SECURE_SETTINGS),
+                    anyString());
+        }
+    }
+
     @Test
     public void testConfigurationChangeListenerRegistrationAndCallbacks() throws Exception {
-        doNothing().when(mMockContext).enforceCallingPermission(anyString(), any());
-
         TimeZoneConfiguration autoDetectDisabledConfiguration =
                 createTimeZoneConfiguration(false /* autoDetectionEnabled */);
 
@@ -169,22 +187,69 @@ public class TimeZoneDetectorServiceTest {
 
         IBinder mockListenerBinder = mock(IBinder.class);
         ITimeZoneConfigurationListener mockListener = mock(ITimeZoneConfigurationListener.class);
-        when(mockListener.asBinder()).thenReturn(mockListenerBinder);
 
-        mTimeZoneDetectorService.addConfigurationListener(mockListener);
+        {
+            doNothing().when(mMockContext).enforceCallingPermission(anyString(), any());
+            when(mockListener.asBinder()).thenReturn(mockListenerBinder);
 
-        verify(mMockContext).enforceCallingPermission(
-                eq(android.Manifest.permission.WRITE_SECURE_SETTINGS),
-                anyString());
-        verify(mockListenerBinder).linkToDeath(any(), eq(0));
+            mTimeZoneDetectorService.addConfigurationListener(mockListener);
 
-        // Simulate the configuration being changed and verify the mockListener was notified.
-        TimeZoneConfiguration autoDetectEnabledConfiguration =
-                createTimeZoneConfiguration(true /* autoDetectionEnabled */);
-        mFakeTimeZoneDetectorStrategy.updateConfiguration(
-                ARBITRARY_USER_ID, autoDetectEnabledConfiguration);
+            verify(mMockContext).enforceCallingPermission(
+                    eq(android.Manifest.permission.WRITE_SECURE_SETTINGS),
+                    anyString());
+            verify(mockListener).asBinder();
+            verify(mockListenerBinder).linkToDeath(any(), anyInt());
+            verifyNoMoreInteractions(mockListenerBinder, mockListener, mMockContext);
+            reset(mockListenerBinder, mockListener, mMockContext);
+        }
+
+        {
+            doNothing().when(mMockContext).enforceCallingPermission(anyString(), any());
+
+            // Simulate the configuration being changed and verify the mockListener was notified.
+            TimeZoneConfiguration autoDetectEnabledConfiguration =
+                    createTimeZoneConfiguration(true /* autoDetectionEnabled */);
+
+            mTimeZoneDetectorService.updateConfiguration(autoDetectEnabledConfiguration);
+
+            verify(mMockContext).enforceCallingPermission(
+                    eq(android.Manifest.permission.WRITE_SECURE_SETTINGS),
+                    anyString());
+            verify(mockListener).onChange(autoDetectEnabledConfiguration);
+            verifyNoMoreInteractions(mockListenerBinder, mockListener, mMockContext);
+            reset(mockListenerBinder, mockListener, mMockContext);
+        }
+
+        {
+            doNothing().when(mMockContext).enforceCallingPermission(anyString(), any());
+            when(mockListener.asBinder()).thenReturn(mockListenerBinder);
+            when(mockListenerBinder.unlinkToDeath(any(), anyInt())).thenReturn(true);
+
+            // Now remove the listener, change the config again, and verify the listener is not
+            // called.
+            mTimeZoneDetectorService.removeConfigurationListener(mockListener);
 
-        verify(mockListener).onChange(autoDetectEnabledConfiguration);
+            verify(mMockContext).enforceCallingPermission(
+                    eq(android.Manifest.permission.WRITE_SECURE_SETTINGS),
+                    anyString());
+            verify(mockListener).asBinder();
+            verify(mockListenerBinder).unlinkToDeath(any(), eq(0));
+            verifyNoMoreInteractions(mockListenerBinder, mockListener, mMockContext);
+            reset(mockListenerBinder, mockListener, mMockContext);
+        }
+
+        {
+            doNothing().when(mMockContext).enforceCallingPermission(anyString(), any());
+
+            mTimeZoneDetectorService.updateConfiguration(autoDetectDisabledConfiguration);
+
+            verify(mMockContext).enforceCallingPermission(
+                    eq(android.Manifest.permission.WRITE_SECURE_SETTINGS),
+                    anyString());
+            verify(mockListener, never()).onChange(any());
+            verifyNoMoreInteractions(mockListenerBinder, mockListener, mMockContext);
+            reset(mockListenerBinder, mockListener, mMockContext);
+        }
     }
 
     @Test(expected = SecurityException.class)