Revert "Revert "Re-add OfflineLocationTimeZoneDelegate.onDestroy""

Second attempt to land commit 96aaef1df9594f6c339c5d48ac3982e09b980c00 /
change-id I0918f91b4c15513a90721350633d4d68c4353240

This reverts commit 4754ac0d6be53a810a1fccbdafe7ce7f5a97f7f1 and removes
the exception thrown from unexpected state transitions. Those state
transitions are possible so they are now logged instead. Documentation
has also been improved here and in TimeZoneProviderService.

Original commit message:
Re-add OfflineLocationTimeZoneDelegate.onDestroy

Re-add OfflineLocationTimeZoneDelegate.onDestroy, the wiring to call it
and tests.

Bug: 188780558
Bug: 191444000
Test: Treehugger
Test: atest locationtzprovider/src/test/java/com/android/timezone/location/provider/core/OfflineLocationTimeZoneDelegateTest.java
Change-Id: I25fb556e5dc796a0eaa64937da412b566a09ac94

4 files changed, 163 insertions, 55 deletions

diff --git a/locationtzprovider/src/main/java/com/android/timezone/location/provider/OfflineLocationTimeZoneProviderService.java b/locationtzprovider/src/main/java/com/android/timezone/location/provider/OfflineLocationTimeZoneProviderService.java
index f8dc49a..c8b7e22 100644
--- a/locationtzprovider/src/main/java/com/android/timezone/location/provider/OfflineLocationTimeZoneProviderService.java
+++ b/locationtzprovider/src/main/java/com/android/timezone/location/provider/OfflineLocationTimeZoneProviderService.java
@@ -59,6 +59,11 @@ public final class OfflineLocationTimeZoneProviderService extends TimeZoneProvid
     }
 
     @Override
+    public void onDestroy() {
+        mDelegate.onDestroy();
+    }
+
+    @Override
     public void onStartUpdates(long initializationTimeoutMillis) {
         mDelegate.onStartUpdates(Duration.ofMillis(initializationTimeoutMillis));
     }
diff --git a/locationtzprovider/src/main/java/com/android/timezone/location/provider/core/Mode.java b/locationtzprovider/src/main/java/com/android/timezone/location/provider/core/Mode.java
index 6d0eda4..0233442 100644
--- a/locationtzprovider/src/main/java/com/android/timezone/location/provider/core/Mode.java
+++ b/locationtzprovider/src/main/java/com/android/timezone/location/provider/core/Mode.java
@@ -56,10 +56,10 @@ import java.util.Objects;
  *       - when the system server sends a "stopped" request it stops listening for the current
  *         location.
  *
- * {All states}
+ * {States except MODE_DESTROYED}
  *   -> {@link #MODE_FAILED} (terminal state)
  *       - when there is a fatal error.
- * {Most states}
+ * {All states}
  *   -> {@link #MODE_DESTROYED} (terminal state)
  *       - when the provider's service is destroyed, perhaps as part of the current user changing
  * </pre>
diff --git a/locationtzprovider/src/main/java/com/android/timezone/location/provider/core/OfflineLocationTimeZoneDelegate.java b/locationtzprovider/src/main/java/com/android/timezone/location/provider/core/OfflineLocationTimeZoneDelegate.java
index 33c26f8..f97e9c8 100644
--- a/locationtzprovider/src/main/java/com/android/timezone/location/provider/core/OfflineLocationTimeZoneDelegate.java
+++ b/locationtzprovider/src/main/java/com/android/timezone/location/provider/core/OfflineLocationTimeZoneDelegate.java
@@ -59,7 +59,10 @@ import java.util.Objects;
  * <p>Implementation details:
  *
  * <p>The instance interacts with multiple threads, but state changes occur in a single-threaded
- * manner through the use of a lock object, {@link #mLock}.
+ * manner through the use of a lock object, {@link #mLock}. Because multiple threads are involved,
+ * service lifecycle calls like {@link #onDestroy()} may be invoked by different threads than binder
+ * calls like {@link #onStopUpdates()}, leading to unintuitive ordering. See
+ * {@link android.service.timezone.TimeZoneProviderService} for details.
  *
  * <p>There are two listening modes:
  * <ul>
@@ -174,6 +177,24 @@ public final class OfflineLocationTimeZoneDelegate {
         }
     }
 
+    /** Called during {@link android.service.timezone.TimeZoneProviderService#onDestroy}. */
+    public void onDestroy() {
+        PiiLoggable entryCause = PiiLoggables.fromString("onDestroy() called");
+        logDebug(entryCause);
+
+        synchronized (mLock) {
+            cancelTimeoutsAndLocationCallbacks();
+
+            Mode currentMode = mCurrentMode.get();
+            if (currentMode.mModeEnum == MODE_STARTED) {
+                sendTimeZoneUncertainResultIfNeeded();
+            }
+            // The current mode can be set to MODE_DESTROYED in all cases, even from MODE_FAILED.
+            Mode newMode = new Mode(MODE_DESTROYED, entryCause);
+            mCurrentMode.set(newMode);
+        }
+    }
+
     /** Called during {@link android.service.timezone.TimeZoneProviderService#onStartUpdates}. */
     public void onStartUpdates(@NonNull Duration initializationTimeout) {
         Objects.requireNonNull(initializationTimeout);
@@ -184,23 +205,10 @@ public final class OfflineLocationTimeZoneDelegate {
 
         synchronized (mLock) {
             Mode currentMode = mCurrentMode.get();
-            switch (currentMode.mModeEnum) {
-                case MODE_STOPPED: {
-                    enterStartedMode(initializationTimeout, debugInfo);
-                    break;
-                }
-                case MODE_STARTED: {
-                    // No-op - the provider is already started.
-                    logWarn("Unexpected onStarted() received when in currentMode=" + currentMode);
-                    break;
-                }
-                case MODE_FAILED:
-                case MODE_DESTROYED:
-                default: {
-                    handleUnexpectedStateTransition(
-                            "Unexpected onStarted() received when in currentMode=" + currentMode);
-                    break;
-                }
+            if (currentMode.mModeEnum == MODE_STOPPED) {
+                enterStartedMode(initializationTimeout, debugInfo);
+            } else {
+                logWarn("Unexpected onStarted() received when in currentMode=" + currentMode);
             }
         }
     }
@@ -212,23 +220,15 @@ public final class OfflineLocationTimeZoneDelegate {
 
         synchronized (mLock) {
             Mode currentMode = mCurrentMode.get();
-            switch (currentMode.mModeEnum) {
-                case MODE_STOPPED: {
-                    // No-op - the provider is already stopped.
-                    logWarn("Unexpected onStopUpdates() when currentMode=" + currentMode);
-                    break;
-                }
-                case MODE_STARTED: {
-                    enterStoppedMode(debugInfo);
-                    break;
-                }
-                case MODE_FAILED:
-                case MODE_DESTROYED:
-                default: {
-                    handleUnexpectedStateTransition(
-                            "Unexpected onStopUpdates() when currentMode=" + currentMode);
-                    break;
-                }
+            if (currentMode.mModeEnum == MODE_STARTED) {
+                enterStoppedMode(debugInfo);
+            } else if (currentMode.mModeEnum == MODE_DESTROYED) {
+                // This can happen because onDestroy() and onStopUpdates() are handled by different
+                // threads: it is still logged, but at a lower priority than other unexpected
+                // transitions.
+                logDebug("Unexpected onStopUpdates() when currentMode=" + currentMode);
+            } else {
+                logWarn("Unexpected onStopUpdates() when currentMode=" + currentMode);
             }
         }
     }
@@ -278,7 +278,7 @@ public final class OfflineLocationTimeZoneDelegate {
                 String unexpectedStateDebugInfo = "Unexpected call to onActiveListeningResult(),"
                         + " activeListeningResult=" + activeListeningResult
                         + ", currentMode=" + currentMode;
-                handleUnexpectedLocationCallback(unexpectedStateDebugInfo);
+                reportUnexpectedLocationCallback(unexpectedStateDebugInfo);
                 return;
             }
 
@@ -320,7 +320,7 @@ public final class OfflineLocationTimeZoneDelegate {
                 String unexpectedStateDebugInfo = "Unexpected call to onPassiveListeningResult(),"
                         + " passiveListeningResult=" + passiveListeningResult
                         + ", currentMode=" + currentMode;
-                handleUnexpectedLocationCallback(unexpectedStateDebugInfo);
+                reportUnexpectedLocationCallback(unexpectedStateDebugInfo);
                 return;
             }
             logDebug("onPassiveListeningResult()"
@@ -397,7 +397,7 @@ public final class OfflineLocationTimeZoneDelegate {
             Mode currentMode = mCurrentMode.get();
             if (currentMode.mModeEnum != MODE_STARTED
                     || currentMode.mListenMode != LOCATION_LISTEN_MODE_PASSIVE) {
-                handleUnexpectedLocationCallback("Unexpected call to onPassiveListeningEnded()"
+                reportUnexpectedLocationCallback("Unexpected call to onPassiveListeningEnded()"
                         + ", currentMode=" + currentMode);
                 return;
             }
@@ -529,14 +529,7 @@ public final class OfflineLocationTimeZoneDelegate {
     }
 
     @GuardedBy("mLock")
-    private void handleUnexpectedStateTransition(@NonNull String debugInfo) {
-        // To help track down unexpected behavior, this fails hard.
-        logWarn(debugInfo);
-        throw new IllegalStateException(debugInfo);
-    }
-
-    @GuardedBy("mLock")
-    private void handleUnexpectedLocationCallback(@NonNull String debugInfo) {
+    private void reportUnexpectedLocationCallback(@NonNull String debugInfo) {
         // Unexpected location callbacks can occur when location listening is cancelled, but a
         // location is already available (e.g. the callback is already invoked but blocked or
         // sitting in a handler queue). This is logged but generally ignored.
@@ -566,10 +559,13 @@ public final class OfflineLocationTimeZoneDelegate {
 
         cancelTimeoutsAndLocationCallbacks();
 
-        sendPermanentFailureResult(failure);
+        // Avoid a transition from MODE_DESTROYED -> MODE_FAILED.
+        if (mCurrentMode.get().mModeEnum != MODE_DESTROYED) {
+            sendPermanentFailureResult(failure);
 
-        Mode newMode = new Mode(MODE_FAILED, entryCause);
-        mCurrentMode.set(newMode);
+            Mode newMode = new Mode(MODE_FAILED, entryCause);
+            mCurrentMode.set(newMode);
+        }
     }
 
     @GuardedBy("mLock")
diff --git a/locationtzprovider/src/test/java/com/android/timezone/location/provider/core/OfflineLocationTimeZoneDelegateTest.java b/locationtzprovider/src/test/java/com/android/timezone/location/provider/core/OfflineLocationTimeZoneDelegateTest.java
index e17656a..e1947ef 100644
--- a/locationtzprovider/src/test/java/com/android/timezone/location/provider/core/OfflineLocationTimeZoneDelegateTest.java
+++ b/locationtzprovider/src/test/java/com/android/timezone/location/provider/core/OfflineLocationTimeZoneDelegateTest.java
@@ -18,6 +18,7 @@ package com.android.timezone.location.provider.core;
 import static com.android.timezone.location.provider.core.OfflineLocationTimeZoneDelegate.LOCATION_LISTEN_MODE_ACTIVE;
 import static com.android.timezone.location.provider.core.OfflineLocationTimeZoneDelegate.LOCATION_LISTEN_MODE_PASSIVE;
 import static com.android.timezone.location.provider.core.TimeZoneProviderResult.RESULT_TYPE_SUGGESTION;
+import static com.android.timezone.location.provider.core.TimeZoneProviderResult.RESULT_TYPE_UNCERTAIN;
 
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -91,11 +92,13 @@ public class OfflineLocationTimeZoneDelegateTest {
         assertEquals(Mode.MODE_STOPPED, mDelegate.getCurrentModeEnumForTests());
         mTestEnvironment.assertIsNotListening();
         mTestEnvironment.assertNoActiveTimeouts();
+        mTestEnvironment.assertNoResultReported();
 
-        // The provider should have started an initialization timeout and followed the first
-        // instruction from the accountant.
         final Duration initializationTimeout = Duration.ofSeconds(20);
         mDelegate.onStartUpdates(initializationTimeout);
+
+        // The provider should have started an initialization timeout and followed the first
+        // instruction from the accountant.
         FakeEnvironment.TestTimeoutState<?> initializationTimeoutState =
                 mTestEnvironment.getActiveTimeoutState(0);
         initializationTimeoutState.assertDelay(initializationTimeout);
@@ -142,11 +145,13 @@ public class OfflineLocationTimeZoneDelegateTest {
         assertEquals(Mode.MODE_STOPPED, mDelegate.getCurrentModeEnumForTests());
         mTestEnvironment.assertIsNotListening();
         mTestEnvironment.assertNoActiveTimeouts();
+        mTestEnvironment.assertNoResultReported();
 
-        // The provider should have started an initialization timeout and followed the first
-        // instruction from the accountant.
         final Duration initializationTimeout = Duration.ofSeconds(20);
         mDelegate.onStartUpdates(initializationTimeout);
+
+        // The provider should have started an initialization timeout and followed the first
+        // instruction from the accountant.
         FakeEnvironment.TestTimeoutState<?> initializationTimeoutState =
                 mTestEnvironment.getActiveTimeoutState(0);
         initializationTimeoutState.assertDelay(initializationTimeout);
@@ -169,6 +174,102 @@ public class OfflineLocationTimeZoneDelegateTest {
                 passiveInstruction.listenMode, passiveInstruction.duration);
     }
 
+    @Test
+    public void onDestroy_neverStarted() throws Exception {
+        assertEquals(Mode.MODE_STOPPED, mDelegate.getCurrentModeEnumForTests());
+        mTestEnvironment.assertIsNotListening();
+        mTestEnvironment.assertNoActiveTimeouts();
+        mTestEnvironment.assertNoResultReported();
+
+        mDelegate.onDestroy();
+
+        assertEquals(Mode.MODE_DESTROYED, mDelegate.getCurrentModeEnumForTests());
+        mTestEnvironment.assertIsNotListening();
+        mTestEnvironment.assertNoActiveTimeouts();
+        mTestEnvironment.assertNoResultReported();
+    }
+
+    @Test
+    public void onDestroy_afterStarted() throws Exception {
+        // Prime the accountant with instructions.
+        ListeningInstruction activeInstruction = new ListeningInstruction(
+                LOCATION_LISTEN_MODE_ACTIVE, Duration.ofSeconds(15));
+        ListeningInstruction passiveInstruction = new ListeningInstruction(
+                LOCATION_LISTEN_MODE_PASSIVE, Duration.ofSeconds(25));
+        mTestLocationListeningAccountant.addInstruction(activeInstruction)
+                .addInstruction(passiveInstruction);
+
+        // Start of the test
+
+        assertEquals(Mode.MODE_STOPPED, mDelegate.getCurrentModeEnumForTests());
+        mTestEnvironment.assertIsNotListening();
+        mTestEnvironment.assertNoActiveTimeouts();
+        mTestEnvironment.assertNoResultReported();
+
+        final Duration initializationTimeout = Duration.ofSeconds(20);
+        mDelegate.onStartUpdates(initializationTimeout);
+
+        // The provider should have started an initialization timeout and followed the first
+        // instruction from the accountant.
+        FakeEnvironment.TestTimeoutState<?> initializationTimeoutState =
+                mTestEnvironment.getActiveTimeoutState(0);
+        initializationTimeoutState.assertDelay(initializationTimeout);
+        assertEquals(Mode.MODE_STARTED, mDelegate.getCurrentModeEnumForTests());
+        mTestEnvironment.assertIsLocationListening(
+                activeInstruction.listenMode, activeInstruction.duration);
+
+        // Destroyed without first being stopped.
+        mDelegate.onDestroy();
+
+        assertEquals(Mode.MODE_DESTROYED, mDelegate.getCurrentModeEnumForTests());
+        mTestEnvironment.assertIsNotListening();
+        mTestEnvironment.assertNoActiveTimeouts();
+        mTestEnvironment.assertUncertainResultReported();
+    }
+
+    @Test
+    public void onDestroy_afterStartedAndStopped() throws Exception {
+        // Prime the accountant with instructions.
+        ListeningInstruction activeInstruction = new ListeningInstruction(
+                LOCATION_LISTEN_MODE_ACTIVE, Duration.ofSeconds(15));
+        ListeningInstruction passiveInstruction = new ListeningInstruction(
+                LOCATION_LISTEN_MODE_PASSIVE, Duration.ofSeconds(25));
+        mTestLocationListeningAccountant.addInstruction(activeInstruction)
+                .addInstruction(passiveInstruction);
+
+        // Start of the test
+        assertEquals(Mode.MODE_STOPPED, mDelegate.getCurrentModeEnumForTests());
+        mTestEnvironment.assertIsNotListening();
+        mTestEnvironment.assertNoActiveTimeouts();
+        mTestEnvironment.assertNoResultReported();
+
+        final Duration initializationTimeout = Duration.ofSeconds(20);
+        mDelegate.onStartUpdates(initializationTimeout);
+
+        // The provider should have started an initialization timeout and followed the first
+        // instruction from the accountant.
+        FakeEnvironment.TestTimeoutState<?> initializationTimeoutState =
+                mTestEnvironment.getActiveTimeoutState(0);
+        initializationTimeoutState.assertDelay(initializationTimeout);
+        assertEquals(Mode.MODE_STARTED, mDelegate.getCurrentModeEnumForTests());
+        mTestEnvironment.assertIsLocationListening(
+                activeInstruction.listenMode, activeInstruction.duration);
+
+        mDelegate.onStopUpdates();
+        assertEquals(Mode.MODE_STOPPED, mDelegate.getCurrentModeEnumForTests());
+        mTestEnvironment.assertIsNotListening();
+        mTestEnvironment.assertNoActiveTimeouts();
+        mTestEnvironment.assertNoResultReported();
+
+        // Destroyed without first being stopped.
+        mDelegate.onDestroy();
+
+        assertEquals(Mode.MODE_DESTROYED, mDelegate.getCurrentModeEnumForTests());
+        mTestEnvironment.assertIsNotListening();
+        mTestEnvironment.assertNoActiveTimeouts();
+        mTestEnvironment.assertNoResultReported();
+    }
+
     private static class FakeEnvironment implements Environment {
 
         private final FakeGeoTimeZonesFinder mFakeGeoTimeZonesFinder = new FakeGeoTimeZonesFinder();
@@ -321,6 +422,12 @@ public class OfflineLocationTimeZoneDelegateTest {
             return this;
         }
 
+        FakeEnvironment assertUncertainResultReported() {
+            assertNotNull(mLastResultReported);
+            assertEquals(RESULT_TYPE_UNCERTAIN, mLastResultReported.getType());
+            return this;
+        }
+
         FakeEnvironment simulateTimePassing(Duration duration) {
             mElapsedRealtimeMillis += duration.toMillis();
             return this;