From d748d91a024cf56ef4edf75c819f09d459d9706e Mon Sep 17 00:00:00 2001 From: Neil Fuller Date: Mon, 24 Apr 2023 16:48:22 +0100 Subject: [PATCH] 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 --- .../timedetector/ConfigurationInternal.java | 5 +- .../server/timedetector/EnvironmentImpl.java | 38 ++-- .../timedetector/TimeDetectorService.java | 29 ++- .../timedetector/TimeDetectorStrategy.java | 44 +++++ .../TimeDetectorStrategyImpl.java | 173 +++++++++++++----- .../FakeServiceConfigAccessor.java | 40 ++-- .../FakeTimeDetectorStrategy.java | 32 ++++ .../TimeDetectorInternalImplTest.java | 5 +- .../timedetector/TimeDetectorServiceTest.java | 10 +- .../TimeDetectorStrategyImplTest.java | 153 ++++++++++++---- 10 files changed, 384 insertions(+), 145 deletions(-) diff --git a/services/core/java/com/android/server/timedetector/ConfigurationInternal.java b/services/core/java/com/android/server/timedetector/ConfigurationInternal.java index 4f221b532b757..97181948352b1 100644 --- a/services/core/java/com/android/server/timedetector/ConfigurationInternal.java +++ b/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; diff --git a/services/core/java/com/android/server/timedetector/EnvironmentImpl.java b/services/core/java/com/android/server/timedetector/EnvironmentImpl.java index fc960d83dc3bf..c52f8f8874630 100644 --- a/services/core/java/com/android/server/timedetector/EnvironmentImpl.java +++ b/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 diff --git a/services/core/java/com/android/server/timedetector/TimeDetectorService.java b/services/core/java/com/android/server/timedetector/TimeDetectorService.java index 22f096b11f18a..d88f4268434b1 100644 --- a/services/core/java/com/android/server/timedetector/TimeDetectorService.java +++ b/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. diff --git a/services/core/java/com/android/server/timedetector/TimeDetectorStrategy.java b/services/core/java/com/android/server/timedetector/TimeDetectorStrategy.java index 11cec6663a376..15c0a809cde84 100644 --- a/services/core/java/com/android/server/timedetector/TimeDetectorStrategy.java +++ b/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 current user only. 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. + * + *

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. + * + *

This method returns {@code true} if the configuration was changed, {@code false} + * otherwise. + * + *

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); diff --git a/services/core/java/com/android/server/timedetector/TimeDetectorStrategyImpl.java b/services/core/java/com/android/server/timedetector/TimeDetectorStrategyImpl.java index b293bacfdc0d6..fd35df61c9522 100644 --- a/services/core/java/com/android/server/timedetector/TimeDetectorStrategyImpl.java +++ b/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 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(); diff --git a/services/tests/servicestests/src/com/android/server/timedetector/FakeServiceConfigAccessor.java b/services/tests/servicestests/src/com/android/server/timedetector/FakeServiceConfigAccessor.java index 93464cdd13d0c..d9bc74dfb1cbf 100644 --- a/services/tests/servicestests/src/com/android/server/timedetector/FakeServiceConfigAccessor.java +++ b/services/tests/servicestests/src/com/android/server/timedetector/FakeServiceConfigAccessor.java @@ -35,7 +35,7 @@ public class FakeServiceConfigAccessor implements ServiceConfigAccessor { private final List 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(); + } } } diff --git a/services/tests/servicestests/src/com/android/server/timedetector/FakeTimeDetectorStrategy.java b/services/tests/servicestests/src/com/android/server/timedetector/FakeTimeDetectorStrategy.java index 87aa2725491bb..a7a9c0cd86f23 100644 --- a/services/tests/servicestests/src/com/android/server/timedetector/FakeTimeDetectorStrategy.java +++ b/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) { } diff --git a/services/tests/servicestests/src/com/android/server/timedetector/TimeDetectorInternalImplTest.java b/services/tests/servicestests/src/com/android/server/timedetector/TimeDetectorInternalImplTest.java index a0845a64757e9..de5a37b43df83 100644 --- a/services/tests/servicestests/src/com/android/server/timedetector/TimeDetectorInternalImplTest.java +++ b/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) diff --git a/services/tests/servicestests/src/com/android/server/timedetector/TimeDetectorServiceTest.java b/services/tests/servicestests/src/com/android/server/timedetector/TimeDetectorServiceTest.java index daa6823428366..6b2d4b01dd089 100644 --- a/services/tests/servicestests/src/com/android/server/timedetector/TimeDetectorServiceTest.java +++ b/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); diff --git a/services/tests/servicestests/src/com/android/server/timedetector/TimeDetectorStrategyImplTest.java b/services/tests/servicestests/src/com/android/server/timedetector/TimeDetectorStrategyImplTest.java index 4df21e00d61cd..dd58135a8e87e 100644 --- a/services/tests/servicestests/src/com/android/server/timedetector/TimeDetectorStrategyImplTest.java +++ b/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 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 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 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); + } }