Add ID field to the Callback

This avoid an almost infinite loop in case of a cache hit, and allows for better tracking/debug.

Bug: 111939367
Test: manually and atest AttentionDetectorTest
Change-Id: Iaa037e133ea9106b4c974c67dae286542103277d

diff --git a/services/core/java/com/android/server/power/AttentionDetector.java b/services/core/java/com/android/server/power/AttentionDetector.java
index 5e829b2..a65a812 100644 (file)
--- a/services/core/java/com/android/server/power/AttentionDetector.java
+++ b/services/core/java/com/android/server/power/AttentionDetector.java
@@ -76,6 +76,12 @@ public class AttentionDetector {
     private final AtomicBoolean mRequested;
 
     /**
+     * Monotonously increasing ID for the requests sent.
+     */
+    @VisibleForTesting
+    protected int mRequestId;
+
+    /**
      * {@link android.service.attention.AttentionService} API timeout.
      */
     private long mMaxAttentionApiTimeoutMillis;
@@ -105,36 +111,13 @@ public class AttentionDetector {
     private AtomicLong mConsecutiveTimeoutExtendedCount = new AtomicLong(0);
 
     @VisibleForTesting
-    final AttentionCallbackInternal mCallback = new AttentionCallbackInternal() {
-        @Override
-        public void onSuccess(int result, long timestamp) {
-            Slog.v(TAG, "onSuccess: " + result);
-            if (mRequested.getAndSet(false)) {
-                synchronized (mLock) {
-                    if (mWakefulness != PowerManagerInternal.WAKEFULNESS_AWAKE) {
-                        if (DEBUG) Slog.d(TAG, "Device slept before receiving callback.");
-                        return;
-                    }
-                    if (result == AttentionService.ATTENTION_SUCCESS_PRESENT) {
-                        mOnUserAttention.run();
-                    } else {
-                        resetConsecutiveExtensionCount();
-                    }
-                }
-            }
-        }
-
-        @Override
-        public void onFailure(int error) {
-            Slog.i(TAG, "Failed to check attention: " + error);
-            mRequested.set(false);
-        }
-    };
+    AttentionCallbackInternalImpl mCallback;
 
     public AttentionDetector(Runnable onUserAttention, Object lock) {
         mOnUserAttention = onUserAttention;
         mLock = lock;
         mRequested = new AtomicBoolean(false);
+        mRequestId = 0;
 
         // Device starts with an awake state upon boot.
         mWakefulness = PowerManagerInternal.WAKEFULNESS_AWAKE;
@@ -196,9 +179,7 @@ public class AttentionDetector {
             return nextScreenDimming;
         } else if (mRequested.get()) {
             if (DEBUG) {
-                // TODO(b/128134941): consider adding a member ID increasing counter in
-                //  AttentionCallbackInternal to track this better.
-                Slog.d(TAG, "Pending attention callback, wait.");
+                Slog.d(TAG, "Pending attention callback with ID=" + mCallback.mId + ", wait.");
             }
             return whenToCheck;
         }
@@ -208,6 +189,8 @@ public class AttentionDetector {
         // This means that we must assume that the request was successful, and then cancel it
         // afterwards if AttentionManager couldn't deliver it.
         mRequested.set(true);
+        mRequestId++;
+        mCallback = new AttentionCallbackInternalImpl(mRequestId);
         final boolean sent = mAttentionManager.checkAttention(getAttentionTimeout(), mCallback);
         if (!sent) {
             mRequested.set(false);
@@ -301,4 +284,40 @@ public class AttentionDetector {
         pw.print(" mAttentionServiceSupported=" + isAttentionServiceSupported());
         pw.print(" mRequested=" + mRequested);
     }
+
+    @VisibleForTesting
+    final class AttentionCallbackInternalImpl extends AttentionCallbackInternal {
+        private final int mId;
+
+        AttentionCallbackInternalImpl(int id) {
+            this.mId = id;
+        }
+
+        @Override
+        public void onSuccess(int result, long timestamp) {
+            Slog.v(TAG, "onSuccess: " + result + ", ID: " + mId);
+            // If we don't check for request ID it's possible to get into a loop: success leads
+            // to the onUserAttention(), which in turn triggers updateUserActivity(), which will
+            // call back onSuccess() instantaneously if there is a cached value, and circle repeats.
+            if (mId == mRequestId && mRequested.getAndSet(false)) {
+                synchronized (mLock) {
+                    if (mWakefulness != PowerManagerInternal.WAKEFULNESS_AWAKE) {
+                        if (DEBUG) Slog.d(TAG, "Device slept before receiving callback.");
+                        return;
+                    }
+                    if (result == AttentionService.ATTENTION_SUCCESS_PRESENT) {
+                        mOnUserAttention.run();
+                    } else {
+                        resetConsecutiveExtensionCount();
+                    }
+                }
+            }
+        }
+
+        @Override
+        public void onFailure(int error) {
+            Slog.i(TAG, "Failed to check attention: " + error + ", ID: " + mId);
+            mRequested.set(false);
+        }
+    }
 }

diff --git a/services/tests/servicestests/src/com/android/server/power/AttentionDetectorTest.java b/services/tests/servicestests/src/com/android/server/power/AttentionDetectorTest.java
index 4de00f7..c30a7dd 100644 (file)
--- a/services/tests/servicestests/src/com/android/server/power/AttentionDetectorTest.java
+++ b/services/tests/servicestests/src/com/android/server/power/AttentionDetectorTest.java
@@ -22,6 +22,8 @@ import static com.google.common.truth.Truth.assertThat;
 
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.ArgumentMatchers.anyLong;
+import static org.mockito.Mockito.atMost;
+import static org.mockito.Mockito.doAnswer;
 import static org.mockito.Mockito.never;
 import static org.mockito.Mockito.reset;
 import static org.mockito.Mockito.verify;
@@ -216,6 +218,53 @@ public class AttentionDetectorTest extends AndroidTestCase {
     }
 
     @Test
+    public void testCallbackOnSuccess_doesNotCallNonCurrentCallback() {
+        mAttentionDetector.mRequestId = 5;
+        registerAttention(); // mRequestId = 6;
+        mAttentionDetector.mRequestId = 55;
+
+        mAttentionDetector.mCallback.onSuccess(AttentionService.ATTENTION_SUCCESS_PRESENT,
+                SystemClock.uptimeMillis());
+        verify(mOnUserAttention, never()).run();
+    }
+
+    @Test
+    public void testCallbackOnSuccess_callsCallbackAfterOldCallbackCame() {
+        mAttentionDetector.mRequestId = 5;
+        registerAttention(); // mRequestId = 6;
+        mAttentionDetector.mRequestId = 55;
+
+        mAttentionDetector.mCallback.onSuccess(AttentionService.ATTENTION_SUCCESS_PRESENT,
+                SystemClock.uptimeMillis()); // old callback came
+        mAttentionDetector.mRequestId = 6; // now back to current
+        mAttentionDetector.mCallback.onSuccess(AttentionService.ATTENTION_SUCCESS_PRESENT,
+                SystemClock.uptimeMillis());
+        verify(mOnUserAttention).run();
+    }
+
+    @Test
+    public void testCallbackOnSuccess_DoesNotGoIntoInfiniteLoop() {
+        // Mimic real behavior
+        doAnswer((invocation) -> {
+            // Mimic a cache hit: calling onSuccess() immediately
+            registerAttention();
+            mAttentionDetector.mRequestId++;
+            mAttentionDetector.mCallback.onSuccess(AttentionService.ATTENTION_SUCCESS_PRESENT,
+                    SystemClock.uptimeMillis());
+            return null;
+        }).when(mOnUserAttention).run();
+
+        registerAttention();
+        // This test fails with literal stack overflow:
+        // e.g. java.lang.StackOverflowError: stack size 1039KB
+        mAttentionDetector.mCallback.onSuccess(AttentionService.ATTENTION_SUCCESS_PRESENT,
+                SystemClock.uptimeMillis());
+
+        // We don't actually get here when the test fails
+        verify(mOnUserAttention, atMost(1)).run();
+    }
+
+    @Test
     public void testCallbackOnFailure_unregistersCurrentRequestCode() {
         registerAttention();
         mAttentionDetector.mCallback.onFailure(AttentionService.ATTENTION_FAILURE_UNKNOWN);
@@ -232,7 +281,6 @@ public class AttentionDetectorTest extends AndroidTestCase {
     }
 
     private class TestableAttentionDetector extends AttentionDetector {
-
         private boolean mAttentionServiceSupported;
 
         TestableAttentionDetector() {