Donate to e Foundation | Murena handsets with /e/OS | Own a part of Murena! Learn more

Commit faf53d2a authored by Gavin Corkery's avatar Gavin Corkery
Browse files

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
parent 70127316

core/tests/bugreports/src/com/android/os/bugreports/tests/BugreportManagerTest.java

+9 −3
Original line number Diff line number Diff line
@@ -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);

    }

services/core/java/com/android/server/os/BugreportManagerServiceImpl.java

+7 −0
Original line number Diff line number Diff line
@@ -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.