Time service / strategy config refactoring

Change the TimeDetectorService / TimeDetectorStrategy relationship so
that the service goes via the strategy to access config. The strategy
also interacts with config via the ServiceConfigAccessor rather than
hiding it behind the Environment interface. This matches the
implementation in the equivalent time zone detector code.

This refactoring revealed a bug the equals() method of
ConfigurationInternal, which has been fixed.

Test: atest services/tests/servicestests/src/com/android/server/timedetector/TimeDetectorInternalImplTest.java
Test: atest services/tests/servicestests/src/com/android/server/timedetector/TimeDetectorStrategyImplTest.java
Test: Treehugger
Bug: 262280071
Change-Id: I4f95ca03178baa5a5fe647f6fef40a6f1394a6cf

services/core/java/com/android/server/timedetector/ConfigurationInternal.java

@@ -244,6 +244,8 @@ public final class ConfigurationInternal {
                 && mAutoDetectionEnabledSetting == that.mAutoDetectionEnabledSetting
                 && mUserId == that.mUserId && mUserConfigAllowed == that.mUserConfigAllowed
                 && mSystemClockUpdateThresholdMillis == that.mSystemClockUpdateThresholdMillis
+                && mSystemClockConfidenceThresholdMillis
+                == that.mSystemClockConfidenceThresholdMillis
                 && mAutoSuggestionLowerBound.equals(that.mAutoSuggestionLowerBound)
                 && mManualSuggestionLowerBound.equals(that.mManualSuggestionLowerBound)
                 && mSuggestionUpperBound.equals(that.mSuggestionUpperBound)
@@ -253,7 +255,8 @@ public final class ConfigurationInternal {
     @Override
     public int hashCode() {
         int result = Objects.hash(mAutoDetectionSupported, mAutoDetectionEnabledSetting, mUserId,
-                mUserConfigAllowed, mSystemClockUpdateThresholdMillis, mAutoSuggestionLowerBound,
+                mUserConfigAllowed, mSystemClockUpdateThresholdMillis,
+                mSystemClockConfidenceThresholdMillis, mAutoSuggestionLowerBound,
                 mManualSuggestionLowerBound, mSuggestionUpperBound);
         result = 31 * result + Arrays.hashCode(mOriginPriorities);
         return result;

services/core/java/com/android/server/timedetector/EnvironmentImpl.java

@@ -22,15 +22,16 @@ import android.content.Context;
 import android.os.Handler;
 import android.os.PowerManager;
 import android.os.SystemClock;
+import android.util.IndentingPrintWriter;
 import android.util.Slog;
 
 import com.android.server.AlarmManagerInternal;
 import com.android.server.LocalServices;
 import com.android.server.SystemClockTime;
 import com.android.server.SystemClockTime.TimeConfidence;
-import com.android.server.timezonedetector.StateChangeListener;
 
-import java.io.PrintWriter;
+import java.time.Duration;
+import java.time.Instant;
 import java.util.Objects;
 
 /**
@@ -41,14 +42,11 @@ final class EnvironmentImpl implements TimeDetectorStrategyImpl.Environment {
     private static final String LOG_TAG = TimeDetectorService.TAG;
 
     @NonNull private final Handler mHandler;
-    @NonNull private final ServiceConfigAccessor mServiceConfigAccessor;
     @NonNull private final PowerManager.WakeLock mWakeLock;
     @NonNull private final AlarmManagerInternal mAlarmManagerInternal;
 
-    EnvironmentImpl(@NonNull Context context, @NonNull Handler handler,
-            @NonNull ServiceConfigAccessor serviceConfigAccessor) {
+    EnvironmentImpl(@NonNull Context context, @NonNull Handler handler) {
         mHandler = Objects.requireNonNull(handler);
-        mServiceConfigAccessor = Objects.requireNonNull(serviceConfigAccessor);
 
         PowerManager powerManager = context.getSystemService(PowerManager.class);
         mWakeLock = Objects.requireNonNull(
@@ -58,19 +56,6 @@ final class EnvironmentImpl implements TimeDetectorStrategyImpl.Environment {
                 LocalServices.getService(AlarmManagerInternal.class));
     }
 
-    @Override
-    public void setConfigurationInternalChangeListener(
-            @NonNull StateChangeListener listener) {
-        StateChangeListener stateChangeListener =
-                () -> mHandler.post(listener::onChange);
-        mServiceConfigAccessor.addConfigurationInternalChangeListener(stateChangeListener);
-    }
-
-    @Override
-    public ConfigurationInternal getCurrentUserConfigurationInternal() {
-        return mServiceConfigAccessor.getCurrentUserConfigurationInternal();
-    }
-
     @Override
     public void acquireWakeLock() {
         if (mWakeLock.isHeld()) {
@@ -126,8 +111,19 @@ final class EnvironmentImpl implements TimeDetectorStrategyImpl.Environment {
     }
 
     @Override
-    public void dumpDebugLog(@NonNull PrintWriter printWriter) {
-        SystemClockTime.dump(printWriter);
+    public void dumpDebugLog(@NonNull IndentingPrintWriter pw) {
+        long elapsedRealtimeMillis = elapsedRealtimeMillis();
+        pw.printf("elapsedRealtimeMillis()=%s (%s)\n",
+                Duration.ofMillis(elapsedRealtimeMillis), elapsedRealtimeMillis);
+        long systemClockMillis = systemClockMillis();
+        pw.printf("systemClockMillis()=%s (%s)\n",
+                Instant.ofEpochMilli(systemClockMillis), systemClockMillis);
+        pw.println("systemClockConfidence()=" + systemClockConfidence());
+
+        pw.println("SystemClockTime debug log:");
+        pw.increaseIndent();
+        SystemClockTime.dump(pw);
+        pw.decreaseIndent();
     }
 
     @Override

services/core/java/com/android/server/timedetector/TimeDetectorService.java

@@ -96,8 +96,8 @@ public final class TimeDetectorService extends ITimeDetectorService.Stub
 
             CallerIdentityInjector callerIdentityInjector = CallerIdentityInjector.REAL;
             TimeDetectorService service = new TimeDetectorService(
-                    context, handler, callerIdentityInjector, serviceConfigAccessor,
-                    timeDetectorStrategy, NtpTrustedTime.getInstance(context));
+                    context, handler, callerIdentityInjector, timeDetectorStrategy,
+                    NtpTrustedTime.getInstance(context));
 
             // Publish the binder service so it can be accessed from other (appropriately
             // permissioned) processes.
@@ -108,7 +108,6 @@ public final class TimeDetectorService extends ITimeDetectorService.Stub
     @NonNull private final Handler mHandler;
     @NonNull private final Context mContext;
     @NonNull private final CallerIdentityInjector mCallerIdentityInjector;
-    @NonNull private final ServiceConfigAccessor mServiceConfigAccessor;
     @NonNull private final TimeDetectorStrategy mTimeDetectorStrategy;
     @NonNull private final NtpTrustedTime mNtpTrustedTime;
 
@@ -123,20 +122,18 @@ public final class TimeDetectorService extends ITimeDetectorService.Stub
     @VisibleForTesting
     public TimeDetectorService(@NonNull Context context, @NonNull Handler handler,
             @NonNull CallerIdentityInjector callerIdentityInjector,
-            @NonNull ServiceConfigAccessor serviceConfigAccessor,
             @NonNull TimeDetectorStrategy timeDetectorStrategy,
             @NonNull NtpTrustedTime ntpTrustedTime) {
         mContext = Objects.requireNonNull(context);
         mHandler = Objects.requireNonNull(handler);
         mCallerIdentityInjector = Objects.requireNonNull(callerIdentityInjector);
-        mServiceConfigAccessor = Objects.requireNonNull(serviceConfigAccessor);
         mTimeDetectorStrategy = Objects.requireNonNull(timeDetectorStrategy);
         mNtpTrustedTime = Objects.requireNonNull(ntpTrustedTime);
 
-        // Wire up a change listener so that ITimeZoneDetectorListeners can be notified when
-        // the configuration changes for any reason.
-        mServiceConfigAccessor.addConfigurationInternalChangeListener(
-                () -> mHandler.post(this::handleConfigurationInternalChangedOnHandlerThread));
+        // Wire up a change listener so that ITimeDetectorListeners can be notified when the
+        // detector state changes for any reason.
+        mTimeDetectorStrategy.addChangeListener(
+                () -> mHandler.post(this::handleChangeOnHandlerThread));
     }
 
     @Override
@@ -151,10 +148,8 @@ public final class TimeDetectorService extends ITimeDetectorService.Stub
 
         final long token = mCallerIdentityInjector.clearCallingIdentity();
         try {
-            ConfigurationInternal configurationInternal =
-                    mServiceConfigAccessor.getConfigurationInternal(userId);
-            final boolean bypassUserPolicyCheck = false;
-            return configurationInternal.createCapabilitiesAndConfig(bypassUserPolicyCheck);
+            final boolean bypassUserPolicyChecks = false;
+            return mTimeDetectorStrategy.getCapabilitiesAndConfig(userId, bypassUserPolicyChecks);
         } finally {
             mCallerIdentityInjector.restoreCallingIdentity(token);
         }
@@ -180,9 +175,9 @@ public final class TimeDetectorService extends ITimeDetectorService.Stub
 
         final long token = mCallerIdentityInjector.clearCallingIdentity();
         try {
-            final boolean bypassUserPolicyCheck = false;
-            return mServiceConfigAccessor.updateConfiguration(
-                    resolvedUserId, configuration, bypassUserPolicyCheck);
+            final boolean bypassUserPolicyChecks = false;
+            return mTimeDetectorStrategy.updateConfiguration(
+                    resolvedUserId, configuration, bypassUserPolicyChecks);
         } finally {
             mCallerIdentityInjector.restoreCallingIdentity(token);
         }
@@ -262,7 +257,7 @@ public final class TimeDetectorService extends ITimeDetectorService.Stub
         }
     }
 
-    private void handleConfigurationInternalChangedOnHandlerThread() {
+    private void handleChangeOnHandlerThread() {
         // Configuration has changed, but each user may have a different view of the configuration.
         // It's possible that this will cause unnecessary notifications but that shouldn't be a
         // problem.

services/core/java/com/android/server/timedetector/TimeDetectorStrategy.java

@@ -21,6 +21,8 @@ import android.annotation.NonNull;
 import android.annotation.Nullable;
 import android.annotation.UserIdInt;
 import android.app.time.ExternalTimeSuggestion;
+import android.app.time.TimeCapabilitiesAndConfig;
+import android.app.time.TimeConfiguration;
 import android.app.time.TimeState;
 import android.app.time.UnixEpochTime;
 import android.app.timedetector.ManualTimeSuggestion;
@@ -87,6 +89,48 @@ public interface TimeDetectorStrategy extends Dumpable {
      */
     boolean confirmTime(@NonNull UnixEpochTime confirmationTime);
 
+    /**
+     * Adds a listener that will be triggered when something changes that could affect the result
+     * of the {@link #getCapabilitiesAndConfig} call for the <em>current user only</em>. This
+     * includes the current user changing. This is exposed so that (indirect) users like SettingsUI
+     * can monitor for changes to data derived from {@link TimeCapabilitiesAndConfig} and update
+     * the UI accordingly.
+     */
+    void addChangeListener(@NonNull StateChangeListener listener);
+
+    /**
+     * Returns a {@link TimeCapabilitiesAndConfig} object for the specified user.
+     *
+     * <p>The strategy is dependent on device state like current user, settings and device config.
+     * These updates are usually handled asynchronously, so callers should expect some delay between
+     * a change being made directly to services like settings and the strategy becoming aware of
+     * them. Changes made via {@link #updateConfiguration} will be visible immediately.
+     *
+     * @param userId the user ID to retrieve the information for
+     * @param bypassUserPolicyChecks {@code true} for device policy manager use cases where device
+     *   policy restrictions that should apply to actual users can be ignored
+     */
+    TimeCapabilitiesAndConfig getCapabilitiesAndConfig(
+            @UserIdInt int userId, boolean bypassUserPolicyChecks);
+
+    /**
+     * Updates the configuration properties that control a device's time behavior.
+     *
+     * <p>This method returns {@code true} if the configuration was changed, {@code false}
+     * otherwise.
+     *
+     * <p>See {@link #getCapabilitiesAndConfig} for guarantees about visibility of updates to
+     * subsequent calls.
+     *
+     * @param userId the current user ID, supplied to make sure that the asynchronous process
+     *   that happens when users switch is completed when the call is made
+     * @param configuration the configuration changes
+     * @param bypassUserPolicyChecks {@code true} for device policy manager use cases where device
+     *   policy restrictions that should apply to actual users can be ignored
+     */
+    boolean updateConfiguration(@UserIdInt int userId,
+            @NonNull TimeConfiguration configuration, boolean bypassUserPolicyChecks);
+
     /** Processes the suggested time from telephony sources. */
     void suggestTelephonyTime(@NonNull TelephonyTimeSuggestion suggestion);
 

services/core/java/com/android/server/timedetector/TimeDetectorStrategyImpl.java

@@ -30,6 +30,7 @@ import android.annotation.UserIdInt;
 import android.app.time.ExternalTimeSuggestion;
 import android.app.time.TimeCapabilities;
 import android.app.time.TimeCapabilitiesAndConfig;
+import android.app.time.TimeConfiguration;
 import android.app.time.TimeState;
 import android.app.time.UnixEpochTime;
 import android.app.timedetector.ManualTimeSuggestion;
@@ -48,10 +49,10 @@ import com.android.server.timezonedetector.ArrayMapWithHistory;
 import com.android.server.timezonedetector.ReferenceWithHistory;
 import com.android.server.timezonedetector.StateChangeListener;
 
-import java.io.PrintWriter;
-import java.time.Duration;
 import java.time.Instant;
+import java.util.ArrayList;
 import java.util.Arrays;
+import java.util.List;
 import java.util.Objects;
 
 /**
@@ -94,6 +95,12 @@ public final class TimeDetectorStrategyImpl implements TimeDetectorStrategy {
     @NonNull
     private final Environment mEnvironment;
 
+    @NonNull
+    private final ServiceConfigAccessor mServiceConfigAccessor;
+
+    @GuardedBy("this")
+    @NonNull private final List<StateChangeListener> mStateChangeListeners = new ArrayList<>();
+
     @GuardedBy("this")
     @NonNull
     private ConfigurationInternal mCurrentConfigurationInternal;
@@ -139,16 +146,6 @@ public final class TimeDetectorStrategyImpl implements TimeDetectorStrategy {
      */
     public interface Environment {
 
-        /**
-         * Sets a {@link StateChangeListener} that will be invoked when there are any changes that
-         * could affect the content of {@link ConfigurationInternal}.
-         * This is invoked during system server setup.
-         */
-        void setConfigurationInternalChangeListener(@NonNull StateChangeListener listener);
-
-        /** Returns the {@link ConfigurationInternal} for the current user. */
-        @NonNull ConfigurationInternal getCurrentUserConfigurationInternal();
-
         /** Acquire a suitable wake lock. Must be followed by {@link #releaseWakeLock()} */
         void acquireWakeLock();
 
@@ -174,16 +171,15 @@ public final class TimeDetectorStrategyImpl implements TimeDetectorStrategy {
         /** Release the wake lock acquired by a call to {@link #acquireWakeLock()}. */
         void releaseWakeLock();
 
-
         /**
          * Adds a standalone entry to the time debug log.
          */
         void addDebugLogEntry(@NonNull String logMsg);
 
         /**
-         * Dumps the time debug log to the supplied {@link PrintWriter}.
+         * Dumps the time debug log to the supplied {@link IndentingPrintWriter}.
          */
-        void dumpDebugLog(PrintWriter printWriter);
+        void dumpDebugLog(IndentingPrintWriter ipw);
 
         /**
          * Requests that the supplied runnable is invoked asynchronously.
@@ -195,19 +191,23 @@ public final class TimeDetectorStrategyImpl implements TimeDetectorStrategy {
             @NonNull Context context, @NonNull Handler handler,
             @NonNull ServiceConfigAccessor serviceConfigAccessor) {
 
-        TimeDetectorStrategyImpl.Environment environment =
-                new EnvironmentImpl(context, handler, serviceConfigAccessor);
-        return new TimeDetectorStrategyImpl(environment);
+        TimeDetectorStrategyImpl.Environment environment = new EnvironmentImpl(context, handler);
+        return new TimeDetectorStrategyImpl(environment, serviceConfigAccessor);
     }
 
     @VisibleForTesting
-    TimeDetectorStrategyImpl(@NonNull Environment environment) {
+    TimeDetectorStrategyImpl(@NonNull Environment environment,
+            @NonNull ServiceConfigAccessor serviceConfigAccessor) {
         mEnvironment = Objects.requireNonNull(environment);
+        mServiceConfigAccessor = Objects.requireNonNull(serviceConfigAccessor);
 
         synchronized (this) {
-            mEnvironment.setConfigurationInternalChangeListener(
-                    this::handleConfigurationInternalChanged);
-            mCurrentConfigurationInternal = mEnvironment.getCurrentUserConfigurationInternal();
+            // Listen for config and user changes and get an initial snapshot of configuration.
+            StateChangeListener stateChangeListener = this::handleConfigurationInternalMaybeChanged;
+            mServiceConfigAccessor.addConfigurationInternalChangeListener(stateChangeListener);
+
+            // Initialize mCurrentConfigurationInternal with a starting value.
+            updateCurrentConfigurationInternalIfRequired("TimeDetectorStrategyImpl:");
         }
     }
 
@@ -421,6 +421,57 @@ public final class TimeDetectorStrategyImpl implements TimeDetectorStrategy {
         }
     }
 
+    @GuardedBy("this")
+    private void notifyStateChangeListenersAsynchronously() {
+        for (StateChangeListener listener : mStateChangeListeners) {
+            // This is queuing asynchronous notification, so no need to surrender the "this" lock.
+            mEnvironment.runAsync(listener::onChange);
+        }
+    }
+
+    @Override
+    public synchronized void addChangeListener(@NonNull StateChangeListener listener) {
+        mStateChangeListeners.add(listener);
+    }
+
+    @Override
+    public synchronized TimeCapabilitiesAndConfig getCapabilitiesAndConfig(@UserIdInt int userId,
+            boolean bypassUserPolicyChecks) {
+        ConfigurationInternal configurationInternal;
+        if (mCurrentConfigurationInternal.getUserId() == userId) {
+            // Use the cached snapshot we have.
+            configurationInternal = mCurrentConfigurationInternal;
+        } else {
+            // This is not a common case: It would be unusual to want the configuration for a user
+            // other than the "current" user, but it is supported because it is trivial to do so.
+            // Unlike the current user config, there's no cached copy to worry about so read it
+            // directly from mServiceConfigAccessor.
+            configurationInternal = mServiceConfigAccessor.getConfigurationInternal(userId);
+        }
+        return configurationInternal.createCapabilitiesAndConfig(bypassUserPolicyChecks);
+    }
+
+    @Override
+    public synchronized boolean updateConfiguration(@UserIdInt int userId,
+            @NonNull TimeConfiguration configuration, boolean bypassUserPolicyChecks) {
+        // Write-through
+        boolean updateSuccessful = mServiceConfigAccessor.updateConfiguration(
+                userId, configuration, bypassUserPolicyChecks);
+
+        // The update above will trigger config update listeners asynchronously if they are needed,
+        // but that could mean an immediate call to getCapabilitiesAndConfig() for the current user
+        // wouldn't see the update. So, handle the cache update and notifications here. When the
+        // async update listener triggers it will find everything already up to date and do nothing.
+        if (updateSuccessful) {
+            String logMsg = "updateConfiguration:"
+                    + " userId=" + userId
+                    + ", configuration=" + configuration
+                    + ", bypassUserPolicyChecks=" + bypassUserPolicyChecks;
+            updateCurrentConfigurationInternalIfRequired(logMsg);
+        }
+        return updateSuccessful;
+    }
+
     @Override
     public synchronized void suggestTelephonyTime(@NonNull TelephonyTimeSuggestion suggestion) {
         // Empty time suggestion means that telephony network connectivity has been lost.
@@ -448,26 +499,49 @@ public final class TimeDetectorStrategyImpl implements TimeDetectorStrategy {
         doAutoTimeDetection(reason);
     }
 
-    private synchronized void handleConfigurationInternalChanged() {
-        ConfigurationInternal currentUserConfig =
-                mEnvironment.getCurrentUserConfigurationInternal();
-        String logMsg = "handleConfigurationInternalChanged:"
-                + " oldConfiguration=" + mCurrentConfigurationInternal
-                + ", newConfiguration=" + currentUserConfig;
-        addDebugLogEntry(logMsg);
-        mCurrentConfigurationInternal = currentUserConfig;
+    /**
+     * Handles a configuration change notification.
+     */
+    private synchronized void handleConfigurationInternalMaybeChanged() {
+        String logMsg = "handleConfigurationInternalMaybeChanged:";
+        updateCurrentConfigurationInternalIfRequired(logMsg);
+    }
 
-        boolean autoDetectionEnabled =
-                mCurrentConfigurationInternal.getAutoDetectionEnabledBehavior();
-        // When automatic time detection is enabled we update the system clock instantly if we can.
-        // Conversely, when automatic time detection is disabled we leave the clock as it is.
-        if (autoDetectionEnabled) {
-            String reason = "Auto time zone detection config changed.";
-            doAutoTimeDetection(reason);
-        } else {
-            // CLOCK_PARANOIA: We are losing "control" of the system clock so we cannot predict what
-            // it should be in future.
-            mLastAutoSystemClockTimeSet = null;
+    @GuardedBy("this")
+    private void updateCurrentConfigurationInternalIfRequired(@NonNull String logMsg) {
+        ConfigurationInternal newCurrentConfigurationInternal =
+                mServiceConfigAccessor.getCurrentUserConfigurationInternal();
+        // mCurrentConfigurationInternal is null the first time this method is called.
+        ConfigurationInternal oldCurrentConfigurationInternal = mCurrentConfigurationInternal;
+
+        // If the configuration actually changed, update the cached copy synchronously and do
+        // other necessary house-keeping / (async) listener notifications.
+        if (!newCurrentConfigurationInternal.equals(oldCurrentConfigurationInternal)) {
+            mCurrentConfigurationInternal = newCurrentConfigurationInternal;
+
+            logMsg = new StringBuilder(logMsg)
+                    .append(" [oldConfiguration=").append(oldCurrentConfigurationInternal)
+                    .append(", newConfiguration=").append(newCurrentConfigurationInternal)
+                    .append("]")
+                    .toString();
+            addDebugLogEntry(logMsg);
+
+            // The configuration and maybe the status changed so notify listeners.
+            notifyStateChangeListenersAsynchronously();
+
+            boolean autoDetectionEnabled =
+                    mCurrentConfigurationInternal.getAutoDetectionEnabledBehavior();
+            // When automatic time detection is enabled we update the system clock instantly if we
+            // can. Conversely, when automatic time detection is disabled we leave the clock as it
+            // is.
+            if (autoDetectionEnabled) {
+                String reason = "Auto time zone detection config changed.";
+                doAutoTimeDetection(reason);
+            } else {
+                // CLOCK_PARANOIA: We are losing "control" of the system clock so we cannot predict
+                // what it should be in future.
+                mLastAutoSystemClockTimeSet = null;
+            }
         }
     }
 
@@ -489,13 +563,11 @@ public final class TimeDetectorStrategyImpl implements TimeDetectorStrategy {
         ipw.println("[Capabilities="
                 + mCurrentConfigurationInternal.createCapabilitiesAndConfig(bypassUserPolicyChecks)
                 + "]");
-        long elapsedRealtimeMillis = mEnvironment.elapsedRealtimeMillis();
-        ipw.printf("mEnvironment.elapsedRealtimeMillis()=%s (%s)\n",
-                Duration.ofMillis(elapsedRealtimeMillis), elapsedRealtimeMillis);
-        long systemClockMillis = mEnvironment.systemClockMillis();
-        ipw.printf("mEnvironment.systemClockMillis()=%s (%s)\n",
-                Instant.ofEpochMilli(systemClockMillis), systemClockMillis);
-        ipw.println("mEnvironment.systemClockConfidence()=" + mEnvironment.systemClockConfidence());
+
+        ipw.println("mEnvironment:");
+        ipw.increaseIndent();
+        mEnvironment.dumpDebugLog(ipw);
+        ipw.decreaseIndent();
 
         ipw.println("Time change log:");
         ipw.increaseIndent(); // level 2
@@ -525,6 +597,11 @@ public final class TimeDetectorStrategyImpl implements TimeDetectorStrategy {
         ipw.decreaseIndent(); // level 1
     }
 
+    @VisibleForTesting
+    public synchronized ConfigurationInternal getCachedCapabilitiesAndConfigForTests() {
+        return mCurrentConfigurationInternal;
+    }
+
     @GuardedBy("this")
     private boolean storeTelephonySuggestion(@NonNull TelephonyTimeSuggestion suggestion) {
         UnixEpochTime newUnixEpochTime = suggestion.getUnixEpochTime();

services/tests/servicestests/src/com/android/server/timedetector/FakeServiceConfigAccessor.java

@@ -35,7 +35,7 @@ public class FakeServiceConfigAccessor implements ServiceConfigAccessor {
 
     private final List<StateChangeListener> mConfigurationInternalChangeListeners =
             new ArrayList<>();
-    private ConfigurationInternal mConfigurationInternal;
+    private ConfigurationInternal mCurrentUserConfigurationInternal;
 
     @Override
     public void addConfigurationInternalChangeListener(StateChangeListener listener) {
@@ -49,21 +49,23 @@ public class FakeServiceConfigAccessor implements ServiceConfigAccessor {
 
     @Override
     public ConfigurationInternal getCurrentUserConfigurationInternal() {
-        return mConfigurationInternal;
+        return mCurrentUserConfigurationInternal;
     }
 
     @Override
     public boolean updateConfiguration(
-            @UserIdInt int userID, @NonNull TimeConfiguration requestedChanges,
+            @UserIdInt int userId, @NonNull TimeConfiguration requestedChanges,
             boolean bypassUserPolicyChecks) {
-        assertNotNull(mConfigurationInternal);
+        assertNotNull(mCurrentUserConfigurationInternal);
         assertNotNull(requestedChanges);
 
+        ConfigurationInternal toUpdate = getConfigurationInternal(userId);
+
         // Simulate the real strategy's behavior: the new configuration will be updated to be the
         // old configuration merged with the new if the user has the capability to up the settings.
         // Then, if the configuration changed, the change listener is invoked.
         TimeCapabilitiesAndConfig capabilitiesAndConfig =
-                mConfigurationInternal.createCapabilitiesAndConfig(bypassUserPolicyChecks);
+                toUpdate.createCapabilitiesAndConfig(bypassUserPolicyChecks);
         TimeCapabilities capabilities = capabilitiesAndConfig.getCapabilities();
         TimeConfiguration configuration = capabilitiesAndConfig.getConfiguration();
         TimeConfiguration newConfiguration =
@@ -73,28 +75,36 @@ public class FakeServiceConfigAccessor implements ServiceConfigAccessor {
         }
 
         if (!newConfiguration.equals(capabilitiesAndConfig.getConfiguration())) {
-            mConfigurationInternal = mConfigurationInternal.merge(newConfiguration);
+            mCurrentUserConfigurationInternal = toUpdate.merge(newConfiguration);
 
             // Note: Unlike the real strategy, the listeners are invoked synchronously.
-            simulateConfigurationChangeForTests();
+            notifyConfigurationChange();
         }
         return true;
     }
 
-    void initializeConfiguration(ConfigurationInternal configurationInternal) {
-        mConfigurationInternal = configurationInternal;
+
+    void initializeCurrentUserConfiguration(ConfigurationInternal configurationInternal) {
+        mCurrentUserConfigurationInternal = configurationInternal;
     }
 
-    void simulateConfigurationChangeForTests() {
-        for (StateChangeListener listener : mConfigurationInternalChangeListeners) {
-            listener.onChange();
-        }
+    void simulateCurrentUserConfigurationInternalChange(
+            ConfigurationInternal configurationInternal) {
+        mCurrentUserConfigurationInternal = configurationInternal;
+        // Note: Unlike the real strategy, the listeners are invoked synchronously.
+        notifyConfigurationChange();
     }
 
     @Override
     public ConfigurationInternal getConfigurationInternal(int userId) {
         assertEquals("Multi-user testing not supported currently",
-                userId, mConfigurationInternal.getUserId());
-        return mConfigurationInternal;
+                userId, mCurrentUserConfigurationInternal.getUserId());
+        return mCurrentUserConfigurationInternal;
+    }
+
+    private void notifyConfigurationChange() {
+        for (StateChangeListener listener : mConfigurationInternalChangeListeners) {
+            listener.onChange();
+        }
     }
 }

services/tests/servicestests/src/com/android/server/timedetector/FakeTimeDetectorStrategy.java

@@ -18,6 +18,8 @@ package com.android.server.timedetector;
 
 import android.annotation.UserIdInt;
 import android.app.time.ExternalTimeSuggestion;
+import android.app.time.TimeCapabilitiesAndConfig;
+import android.app.time.TimeConfiguration;
 import android.app.time.TimeState;
 import android.app.time.UnixEpochTime;
 import android.app.timedetector.ManualTimeSuggestion;
@@ -31,10 +33,20 @@ import com.android.server.timezonedetector.StateChangeListener;
  * in tests.
  */
 public class FakeTimeDetectorStrategy implements TimeDetectorStrategy {
+    private final FakeServiceConfigAccessor mFakeServiceConfigAccessor;
+
     // State
     private TimeState mTimeState;
     private NetworkTimeSuggestion mLatestNetworkTimeSuggestion;
 
+    FakeTimeDetectorStrategy() {
+        mFakeServiceConfigAccessor = new FakeServiceConfigAccessor();
+    }
+
+    void initializeConfiguration(ConfigurationInternal configuration) {
+        mFakeServiceConfigAccessor.initializeCurrentUserConfiguration(configuration);
+    }
+
     @Override
     public TimeState getTimeState() {
         return mTimeState;
@@ -50,6 +62,26 @@ public class FakeTimeDetectorStrategy implements TimeDetectorStrategy {
         return false;
     }
 
+    @Override
+    public void addChangeListener(StateChangeListener listener) {
+        mFakeServiceConfigAccessor.addConfigurationInternalChangeListener(listener);
+    }
+
+    @Override
+    public TimeCapabilitiesAndConfig getCapabilitiesAndConfig(int userId,
+            boolean bypassUserPolicyChecks) {
+        ConfigurationInternal configurationInternal =
+                mFakeServiceConfigAccessor.getConfigurationInternal(userId);
+        return configurationInternal.createCapabilitiesAndConfig(bypassUserPolicyChecks);
+    }
+
+    @Override
+    public boolean updateConfiguration(int userId, TimeConfiguration configuration,
+            boolean bypassUserPolicyChecks) {
+        return mFakeServiceConfigAccessor.updateConfiguration(
+                userId, configuration, bypassUserPolicyChecks);
+    }
+
     @Override
     public void suggestTelephonyTime(TelephonyTimeSuggestion suggestion) {
     }

services/tests/servicestests/src/com/android/server/timedetector/TimeDetectorInternalImplTest.java

@@ -89,7 +89,7 @@ public class TimeDetectorInternalImplTest {
     public void testGetCapabilitiesAndConfigForDpm() throws Exception {
         final boolean autoDetectionEnabled = true;
         ConfigurationInternal testConfig = createConfigurationInternal(autoDetectionEnabled);
-        mFakeServiceConfigAccessorSpy.initializeConfiguration(testConfig);
+        mFakeServiceConfigAccessorSpy.initializeCurrentUserConfiguration(testConfig);
 
         TimeCapabilitiesAndConfig actualCapabilitiesAndConfig =
                 mTimeDetectorInternal.getCapabilitiesAndConfigForDpm();
@@ -108,7 +108,8 @@ public class TimeDetectorInternalImplTest {
         final boolean autoDetectionEnabled = false;
         ConfigurationInternal initialConfigurationInternal =
                 createConfigurationInternal(autoDetectionEnabled);
-        mFakeServiceConfigAccessorSpy.initializeConfiguration(initialConfigurationInternal);
+        mFakeServiceConfigAccessorSpy.initializeCurrentUserConfiguration(
+                initialConfigurationInternal);
 
         TimeConfiguration timeConfiguration = new TimeConfiguration.Builder()
                 .setAutoDetectionEnabled(true)

services/tests/servicestests/src/com/android/server/timedetector/TimeDetectorServiceTest.java

@@ -83,7 +83,6 @@ public class TimeDetectorServiceTest {
     private HandlerThread mHandlerThread;
     private TestHandler mTestHandler;
     private TestCallerIdentityInjector mTestCallerIdentityInjector;
-    private FakeServiceConfigAccessor mFakeServiceConfigAccessorSpy;
     private FakeTimeDetectorStrategy mFakeTimeDetectorStrategySpy;
 
     private NtpTrustedTime mMockNtpTrustedTime;
@@ -101,13 +100,12 @@ public class TimeDetectorServiceTest {
         mTestCallerIdentityInjector = new TestCallerIdentityInjector();
         mTestCallerIdentityInjector.initializeCallingUserId(ARBITRARY_USER_ID);
 
-        mFakeServiceConfigAccessorSpy = spy(new FakeServiceConfigAccessor());
         mFakeTimeDetectorStrategySpy = spy(new FakeTimeDetectorStrategy());
         mMockNtpTrustedTime = mock(NtpTrustedTime.class);
 
         mTimeDetectorService = new TimeDetectorService(
                 mMockContext, mTestHandler, mTestCallerIdentityInjector,
-                mFakeServiceConfigAccessorSpy, mFakeTimeDetectorStrategySpy, mMockNtpTrustedTime);
+                mFakeTimeDetectorStrategySpy, mMockNtpTrustedTime);
     }
 
     @After
@@ -132,14 +130,14 @@ public class TimeDetectorServiceTest {
 
         ConfigurationInternal configuration =
                 createConfigurationInternal(true /* autoDetectionEnabled*/);
-        mFakeServiceConfigAccessorSpy.initializeConfiguration(configuration);
+        mFakeTimeDetectorStrategySpy.initializeConfiguration(configuration);
 
         TimeCapabilitiesAndConfig actualCapabilitiesAndConfig =
                 mTimeDetectorService.getCapabilitiesAndConfig();
         verify(mMockContext).enforceCallingPermission(
                 eq(android.Manifest.permission.MANAGE_TIME_AND_ZONE_DETECTION), anyString());
         int expectedUserId = mTestCallerIdentityInjector.getCallingUserId();
-        verify(mFakeServiceConfigAccessorSpy).getConfigurationInternal(expectedUserId);
+        verify(mFakeTimeDetectorStrategySpy).getCapabilitiesAndConfig(expectedUserId, false);
 
         boolean bypassUserPolicyChecks = false;
         TimeCapabilitiesAndConfig expectedCapabilitiesAndConfig =
@@ -174,7 +172,7 @@ public class TimeDetectorServiceTest {
     public void testListenerRegistrationAndCallbacks() throws Exception {
         ConfigurationInternal initialConfiguration =
                 createConfigurationInternal(false /* autoDetectionEnabled */);
-        mFakeServiceConfigAccessorSpy.initializeConfiguration(initialConfiguration);
+        mFakeTimeDetectorStrategySpy.initializeConfiguration(initialConfiguration);
 
         IBinder mockListenerBinder = mock(IBinder.class);
         ITimeDetectorListener mockListener = mock(ITimeDetectorListener.class);

services/tests/servicestests/src/com/android/server/timedetector/TimeDetectorStrategyImplTest.java

@@ -29,14 +29,22 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import static org.mockito.Mockito.never;
+import static org.mockito.Mockito.reset;
+import static org.mockito.Mockito.spy;
+import static org.mockito.Mockito.times;
+import static org.mockito.Mockito.verify;
 
 import android.annotation.UserIdInt;
 import android.app.time.ExternalTimeSuggestion;
+import android.app.time.TimeCapabilitiesAndConfig;
+import android.app.time.TimeConfiguration;
 import android.app.time.TimeState;
 import android.app.time.UnixEpochTime;
 import android.app.timedetector.ManualTimeSuggestion;
 import android.app.timedetector.TelephonyTimeSuggestion;
 import android.os.TimestampedValue;
+import android.util.IndentingPrintWriter;
 
 import com.android.server.SystemClockTime.TimeConfidence;
 import com.android.server.timedetector.TimeDetectorStrategy.Origin;
@@ -47,14 +55,12 @@ import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 
-import java.io.PrintWriter;
 import java.time.Duration;
 import java.time.Instant;
 import java.time.LocalDateTime;
 import java.time.ZoneOffset;
 import java.util.ArrayList;
 import java.util.List;
-import java.util.Objects;
 
 import junitparams.JUnitParamsRunner;
 import junitparams.Parameters;
@@ -120,13 +126,108 @@ public class TimeDetectorStrategyImplTest {
                     .build();
 
     private FakeEnvironment mFakeEnvironment;
+    private FakeServiceConfigAccessor mFakeServiceConfigAccessorSpy;
+    private TimeDetectorStrategyImpl mTimeDetectorStrategy;
 
     @Before
     public void setUp() {
         mFakeEnvironment = new FakeEnvironment();
-        mFakeEnvironment.initializeConfig(CONFIG_AUTO_DISABLED);
         mFakeEnvironment.initializeFakeClocks(
                 ARBITRARY_CLOCK_INITIALIZATION_INFO, TIME_CONFIDENCE_LOW);
+
+        mFakeServiceConfigAccessorSpy = spy(new FakeServiceConfigAccessor());
+        mFakeServiceConfigAccessorSpy.initializeCurrentUserConfiguration(CONFIG_AUTO_DISABLED);
+
+        mTimeDetectorStrategy = new TimeDetectorStrategyImpl(
+                mFakeEnvironment, mFakeServiceConfigAccessorSpy);
+    }
+
+    @Test
+    public void testChangeListenerBehavior() throws Exception {
+        TestStateChangeListener stateChangeListener = new TestStateChangeListener();
+        mTimeDetectorStrategy.addChangeListener(stateChangeListener);
+
+        boolean bypassUserPolicyChecks = false;
+
+        // Report a config change, but not one that actually changes anything.
+        {
+            mFakeServiceConfigAccessorSpy.simulateCurrentUserConfigurationInternalChange(
+                    CONFIG_AUTO_DISABLED);
+            assertStateChangeNotificationsSent(stateChangeListener, 0);
+            assertEquals(CONFIG_AUTO_DISABLED,
+                    mTimeDetectorStrategy.getCachedCapabilitiesAndConfigForTests());
+        }
+
+        // Report a config change that actually changes something.
+        {
+            mFakeServiceConfigAccessorSpy.simulateCurrentUserConfigurationInternalChange(
+                    CONFIG_AUTO_ENABLED);
+            assertStateChangeNotificationsSent(stateChangeListener, 1);
+            assertEquals(CONFIG_AUTO_ENABLED,
+                    mTimeDetectorStrategy.getCachedCapabilitiesAndConfigForTests());
+        }
+
+        // Perform a (current user) update via the strategy.
+        {
+            TimeConfiguration requestedChanges =
+                    new TimeConfiguration.Builder().setAutoDetectionEnabled(false).build();
+            mTimeDetectorStrategy.updateConfiguration(
+                    ARBITRARY_USER_ID, requestedChanges, bypassUserPolicyChecks);
+            assertStateChangeNotificationsSent(stateChangeListener, 1);
+        }
+    }
+
+    // Current user behavior: the strategy caches and returns the latest configuration.
+    @Test
+    public void testReadAndWriteConfiguration() throws Exception {
+        ConfigurationInternal currentUserConfig = CONFIG_AUTO_ENABLED;
+        mFakeServiceConfigAccessorSpy.simulateCurrentUserConfigurationInternalChange(
+                currentUserConfig);
+
+        final boolean bypassUserPolicyChecks = false;
+
+        ConfigurationInternal cachedConfigurationInternal =
+                mTimeDetectorStrategy.getCachedCapabilitiesAndConfigForTests();
+        assertEquals(currentUserConfig, cachedConfigurationInternal);
+
+        // Confirm getCapabilitiesAndConfig() does not call through to the ServiceConfigAccessor.
+        {
+            reset(mFakeServiceConfigAccessorSpy);
+            TimeCapabilitiesAndConfig actualCapabilitiesAndConfig =
+                    mTimeDetectorStrategy.getCapabilitiesAndConfig(
+                            currentUserConfig.getUserId(), bypassUserPolicyChecks);
+            verify(mFakeServiceConfigAccessorSpy, never()).getConfigurationInternal(
+                    currentUserConfig.getUserId());
+
+            TimeCapabilitiesAndConfig expectedCapabilitiesAndConfig =
+                    currentUserConfig.createCapabilitiesAndConfig(bypassUserPolicyChecks);
+            assertEquals(expectedCapabilitiesAndConfig.getCapabilities(),
+                    actualCapabilitiesAndConfig.getCapabilities());
+            assertEquals(expectedCapabilitiesAndConfig.getConfiguration(),
+                    actualCapabilitiesAndConfig.getConfiguration());
+        }
+
+        // Confirm updateConfiguration() calls through to the ServiceConfigAccessor and updates
+        // the cached copy.
+        {
+            boolean newAutoDetectionEnabled =
+                    !cachedConfigurationInternal.getAutoDetectionEnabledBehavior();
+            TimeConfiguration requestedChanges = new TimeConfiguration.Builder()
+                    .setAutoDetectionEnabled(newAutoDetectionEnabled)
+                    .build();
+            ConfigurationInternal expectedConfigAfterChange =
+                    new ConfigurationInternal.Builder(cachedConfigurationInternal)
+                            .setAutoDetectionEnabledSetting(newAutoDetectionEnabled)
+                            .build();
+
+            reset(mFakeServiceConfigAccessorSpy);
+            mTimeDetectorStrategy.updateConfiguration(
+                    currentUserConfig.getUserId(), requestedChanges, bypassUserPolicyChecks);
+            verify(mFakeServiceConfigAccessorSpy, times(1)).updateConfiguration(
+                    currentUserConfig.getUserId(), requestedChanges, bypassUserPolicyChecks);
+            assertEquals(expectedConfigAfterChange,
+                    mTimeDetectorStrategy.getCachedCapabilitiesAndConfigForTests());
+        }
     }
 
     @Test
@@ -1939,36 +2040,20 @@ public class TimeDetectorStrategyImplTest {
 
         private final List<Runnable> mAsyncRunnables = new ArrayList<>();
 
-        private ConfigurationInternal mConfigurationInternal;
         private boolean mWakeLockAcquired;
         private long mElapsedRealtimeMillis;
         private long mSystemClockMillis;
         private int mSystemClockConfidence = TIME_CONFIDENCE_LOW;
-        private StateChangeListener mConfigurationInternalChangeListener;
 
         // Tracking operations.
         private boolean mSystemClockWasSet;
 
-        void initializeConfig(ConfigurationInternal configurationInternal) {
-            mConfigurationInternal = configurationInternal;
-        }
-
         public void initializeFakeClocks(
                 TimestampedValue<Instant> timeInfo, @TimeConfidence int timeConfidence) {
             pokeElapsedRealtimeMillis(timeInfo.getReferenceTimeMillis());
             pokeSystemClockMillis(timeInfo.getValue().toEpochMilli(), timeConfidence);
         }
 
-        @Override
-        public void setConfigurationInternalChangeListener(StateChangeListener listener) {
-            mConfigurationInternalChangeListener = Objects.requireNonNull(listener);
-        }
-
-        @Override
-        public ConfigurationInternal getCurrentUserConfigurationInternal() {
-            return mConfigurationInternal;
-        }
-
         @Override
         public void acquireWakeLock() {
             if (mWakeLockAcquired) {
@@ -2019,7 +2104,7 @@ public class TimeDetectorStrategyImplTest {
         }
 
         @Override
-        public void dumpDebugLog(PrintWriter printWriter) {
+        public void dumpDebugLog(IndentingPrintWriter pw) {
             // No-op for tests
         }
 
@@ -2037,11 +2122,6 @@ public class TimeDetectorStrategyImplTest {
             mAsyncRunnables.clear();
         }
 
-        void simulateConfigurationInternalChange(ConfigurationInternal configurationInternal) {
-            mConfigurationInternal = configurationInternal;
-            mConfigurationInternalChangeListener.onChange();
-        }
-
         void pokeElapsedRealtimeMillis(long elapsedRealtimeMillis) {
             mElapsedRealtimeMillis = elapsedRealtimeMillis;
         }
@@ -2095,13 +2175,6 @@ public class TimeDetectorStrategyImplTest {
      */
     private class Script {
 
-        private final TimeDetectorStrategyImpl mTimeDetectorStrategy;
-
-        Script() {
-            mFakeEnvironment = new FakeEnvironment();
-            mTimeDetectorStrategy = new TimeDetectorStrategyImpl(mFakeEnvironment);
-        }
-
         Script pokeFakeClocks(TimestampedValue<Instant> initialClockTime,
                 @TimeConfidence int timeConfidence) {
             mFakeEnvironment.pokeElapsedRealtimeMillis(initialClockTime.getReferenceTimeMillis());
@@ -2122,7 +2195,8 @@ public class TimeDetectorStrategyImplTest {
          * Simulates the user / user's configuration changing.
          */
         Script simulateConfigurationInternalChange(ConfigurationInternal configurationInternal) {
-            mFakeEnvironment.simulateConfigurationInternalChange(configurationInternal);
+            mFakeServiceConfigAccessorSpy.simulateCurrentUserConfigurationInternalChange(
+                    configurationInternal);
             return this;
         }
 
@@ -2167,14 +2241,15 @@ public class TimeDetectorStrategyImplTest {
 
         Script simulateAutoTimeDetectionToggle() {
             ConfigurationInternal configurationInternal =
-                    mFakeEnvironment.getCurrentUserConfigurationInternal();
+                    mFakeServiceConfigAccessorSpy.getCurrentUserConfigurationInternal();
             boolean autoDetectionEnabledSetting =
                     !configurationInternal.getAutoDetectionEnabledSetting();
             ConfigurationInternal newConfigurationInternal =
                     new ConfigurationInternal.Builder(configurationInternal)
                             .setAutoDetectionEnabledSetting(autoDetectionEnabledSetting)
                             .build();
-            mFakeEnvironment.simulateConfigurationInternalChange(newConfigurationInternal);
+            mFakeServiceConfigAccessorSpy.simulateCurrentUserConfigurationInternalChange(
+                    newConfigurationInternal);
             return this;
         }
 
@@ -2389,4 +2464,12 @@ public class TimeDetectorStrategyImplTest {
         return LocalDateTime.of(year, monthInYear, day, hourOfDay, minute, second)
                 .toInstant(ZoneOffset.UTC);
     }
+
+    private void assertStateChangeNotificationsSent(
+            TestStateChangeListener stateChangeListener, int expectedCount) {
+        // The fake environment needs to be told to run posted work.
+        mFakeEnvironment.runAsyncRunnables();
+
+        stateChangeListener.assertNotificationsReceivedAndReset(expectedCount);
+    }
 }