From faf53d2a76e268d30f9b9edb205a7152840c3c2f Mon Sep 17 00:00:00 2001
From: Gavin Corkery <gavincorkery@google.com>
Date: Thu, 14 Jan 2021 15:31:59 +0000
Subject: [PATCH] Remove dumpstate error race condition

Removes the race condition between the dumpstate binder service
death recipient and the dumpstate listener error callback. This
is done by adding a small wait when the death recipient is
invoked. This ensures that the caller of the Bugreport API does
not erroneously receive an incorrect runtime error callback.

Alters internal tests to account for this new delay.

Bug: 147703592
Test: atest BugreportManagerTest
Test: Manual. Take bugreport, ensure that the death recipient
      does not send an error callback when dumpstate finishes
      successfully, or fails and exits. Test error case by
      manually invoking signalErrorAndExit in DumpstateService.
Change-Id: I8c5b14c88502f0cf4f4f0e5c41d90d6065bfd1d4
---
 .../os/bugreports/tests/BugreportManagerTest.java    | 12 +++++++++---
 .../server/os/BugreportManagerServiceImpl.java       |  7 +++++++
 2 files changed, 16 insertions(+), 3 deletions(-)

diff --git a/core/tests/bugreports/src/com/android/os/bugreports/tests/BugreportManagerTest.java b/core/tests/bugreports/src/com/android/os/bugreports/tests/BugreportManagerTest.java
index 222471a830b1e..09f16a82cd7f3 100644
--- a/core/tests/bugreports/src/com/android/os/bugreports/tests/BugreportManagerTest.java
+++ b/core/tests/bugreports/src/com/android/os/bugreports/tests/BugreportManagerTest.java
@@ -76,6 +76,12 @@ public class BugreportManagerTest {
     private static final long DUMPSTATE_STARTUP_TIMEOUT_MS = TimeUnit.SECONDS.toMillis(10);
     private static final long UIAUTOMATOR_TIMEOUT_MS = TimeUnit.SECONDS.toMillis(10);
 
+
+    // A small timeout used when waiting for the result of a BugreportCallback to be received.
+    // This value must be at least 1000ms since there is an intentional delay in
+    // BugreportManagerServiceImpl in the error case.
+    private static final long CALLBACK_RESULT_TIMEOUT_MS = 1500;
+
     // Sent by Shell when its bugreport finishes (contains final bugreport/screenshot file name
     // associated with the bugreport).
     private static final String INTENT_BUGREPORT_FINISHED =
@@ -185,7 +191,7 @@ public class BugreportManagerTest {
         ParcelFileDescriptor bugreportFd2 = parcelFd(bugreportFile2);
         ParcelFileDescriptor screenshotFd2 = parcelFd(screenshotFile2);
         mBrm.startBugreport(bugreportFd2, screenshotFd2, wifi(), mExecutor, callback2);
-        Thread.sleep(500 /* .5s */);
+        Thread.sleep(CALLBACK_RESULT_TIMEOUT_MS);
 
         // Verify #2 encounters an error.
         assertThat(callback2.getErrorCode()).isEqualTo(
@@ -194,7 +200,7 @@ public class BugreportManagerTest {
 
         // Cancel #1 so we can move on to the next test.
         mBrm.cancelBugreport();
-        Thread.sleep(500 /* .5s */);
+        waitTillDoneOrTimeout(callback);
         assertThat(callback.isDone()).isTrue();
         assertFdsAreClosed(mBugreportFd, mScreenshotFd);
     }
@@ -220,7 +226,7 @@ public class BugreportManagerTest {
         // Try again, with DUMP permission.
         getPermissions();
         mBrm.cancelBugreport();
-        Thread.sleep(500 /* .5s */);
+        waitTillDoneOrTimeout(callback);
         assertThat(callback.isDone()).isTrue();
         assertFdsAreClosed(mBugreportFd, mScreenshotFd);
     }
diff --git a/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java b/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java
index ef0f0eeb56f8d..4ff75fa06077f 100644
--- a/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java
+++ b/services/core/java/com/android/server/os/BugreportManagerServiceImpl.java
@@ -336,6 +336,13 @@ class BugreportManagerServiceImpl extends IDumpstate.Stub {
 
         @Override
         public void binderDied() {
+            try {
+                // Allow a small amount of time for any error or finished callbacks to be made.
+                // This ensures that the listener does not receive an erroneous runtime error
+                // callback.
+                Thread.sleep(1000);
+            } catch (InterruptedException ignored) {
+            }
             synchronized (mLock) {
                 if (!mDone) {
                     // If we have not gotten a "done" callback this must be a crash.