Merge "Prevent PreRebootLogger blocking shutdown thread" into rvc-dev

services/core/java/com/android/server/power/PreRebootLogger.java

@@ -16,6 +16,7 @@
 
 package com.android.server.power;
 
+import android.annotation.DurationMillisLong;
 import android.annotation.NonNull;
 import android.content.Context;
 import android.os.Environment;
@@ -23,7 +24,7 @@ import android.os.IBinder;
 import android.os.ParcelFileDescriptor;
 import android.os.RemoteException;
 import android.os.ServiceManager;
-import android.provider.Settings;
+import android.provider.Settings.Global;
 import android.util.Slog;
 
 import com.android.internal.annotations.GuardedBy;
@@ -34,6 +35,8 @@ import java.io.File;
 import java.io.FileNotFoundException;
 import java.io.FileWriter;
 import java.io.IOException;
+import java.util.concurrent.TimeUnit;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 /**
  * Provides utils to dump/wipe pre-reboot information.
@@ -46,10 +49,12 @@ final class PreRebootLogger {
     private static final String[] SERVICES_TO_DUMP = {Context.ROLLBACK_SERVICE, "package"};
 
     private static final Object sLock = new Object();
+    private static final long MAX_DUMP_TIME = TimeUnit.SECONDS.toMillis(20);
 
     /**
      * Process pre-reboot information. Dump pre-reboot information to {@link #PREREBOOT_DIR} if
-     * enabled {@link Settings.Global#ADB_ENABLED}; wipe dumped information otherwise.
+     * enabled {@link Settings.Global#ADB_ENABLED} and having active staged session; wipe dumped
+     * information otherwise.
      */
     static void log(Context context) {
         log(context, getDumpDir());
@@ -57,28 +62,49 @@ final class PreRebootLogger {
 
     @VisibleForTesting
     static void log(Context context, @NonNull File dumpDir) {
-        if (Settings.Global.getInt(
+        if (needDump(context)) {
-                context.getContentResolver(), Settings.Global.ADB_ENABLED, 0) == 1) {
+            dump(dumpDir, MAX_DUMP_TIME);
-            Slog.d(TAG, "Dumping pre-reboot information...");
-            dump(dumpDir);
         } else {
-            Slog.d(TAG, "Wiping pre-reboot information...");
             wipe(dumpDir);
         }
     }
 
-    private static void dump(@NonNull File dumpDir) {
+    private static boolean needDump(Context context) {
-        synchronized (sLock) {
+        return Global.getInt(context.getContentResolver(), Global.ADB_ENABLED, 0) == 1
-            for (String buffer : BUFFERS_TO_DUMP) {
+                && !context.getPackageManager().getPackageInstaller()
-                dumpLogsLocked(dumpDir, buffer);
+                        .getActiveStagedSessions().isEmpty();
-            }
+    }
-            for (String service : SERVICES_TO_DUMP) {
+
-                dumpServiceLocked(dumpDir, service);
+    @VisibleForTesting
+    static void dump(@NonNull File dumpDir, @DurationMillisLong long maxWaitTime) {
+        Slog.d(TAG, "Dumping pre-reboot information...");
+        final AtomicBoolean done = new AtomicBoolean(false);
+        final Thread t = new Thread(() -> {
+            synchronized (sLock) {
+                for (String buffer : BUFFERS_TO_DUMP) {
+                    dumpLogsLocked(dumpDir, buffer);
+                }
+                for (String service : SERVICES_TO_DUMP) {
+                    dumpServiceLocked(dumpDir, service);
+                }
             }
+            done.set(true);
+        });
+        t.start();
+
+        try {
+            t.join(maxWaitTime);
+        } catch (InterruptedException e) {
+            Slog.e(TAG, "Failed to dump pre-reboot information due to interrupted", e);
+        }
+
+        if (!done.get()) {
+            Slog.w(TAG, "Failed to dump pre-reboot information due to timeout");
         }
     }
 
     private static void wipe(@NonNull File dumpDir) {
+        Slog.d(TAG, "Wiping pre-reboot information...");
         synchronized (sLock) {
             for (File file : dumpDir.listFiles()) {
                 file.delete();
@@ -109,7 +135,7 @@ final class PreRebootLogger {
                     {"logcat", "-d", "-b", buffer, "-f", dumpFile.getAbsolutePath()};
             Runtime.getRuntime().exec(cmdline).waitFor();
         } catch (IOException | InterruptedException e) {
-            Slog.d(TAG, "Dump system log buffer before reboot fail", e);
+            Slog.e(TAG, "Failed to dump system log buffer before reboot", e);
         }
     }
 
@@ -127,7 +153,7 @@ final class PreRebootLogger {
                             | ParcelFileDescriptor.MODE_WRITE_ONLY);
             binder.dump(fd.getFileDescriptor(), ArrayUtils.emptyArray(String.class));
         } catch (FileNotFoundException | RemoteException e) {
-            Slog.d(TAG, String.format("Dump %s service before reboot fail", serviceName), e);
+            Slog.e(TAG, String.format("Failed to dump %s service before reboot", serviceName), e);
         }
     }
 }

services/tests/servicestests/src/com/android/server/power/PreRebootLoggerTest.java

@@ -23,6 +23,9 @@ import static com.google.common.truth.Truth.assertThat;
 import static org.mockito.Mockito.when;
 
 import android.content.Context;
+import android.content.pm.PackageInstaller;
+import android.content.pm.PackageInstaller.SessionInfo;
+import android.content.pm.PackageManager;
 import android.provider.Settings;
 import android.test.mock.MockContentResolver;
 
@@ -36,12 +39,15 @@ import com.google.common.io.Files;
 import org.junit.AfterClass;
 import org.junit.Before;
 import org.junit.BeforeClass;
+import org.junit.Rule;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.Mock;
-import org.mockito.MockitoAnnotations;
+import org.mockito.junit.MockitoJUnit;
+import org.mockito.junit.MockitoRule;
 
 import java.io.File;
+import java.util.List;
 
 /**
  * Tests for {@link PreRebootLogger}
@@ -49,7 +55,11 @@ import java.io.File;
 @SmallTest
 @RunWith(AndroidJUnit4.class)
 public class PreRebootLoggerTest {
+    @Rule public final MockitoRule mocks = MockitoJUnit.rule();
     @Mock Context mContext;
+    @Mock PackageManager mPackageManager;
+    @Mock PackageInstaller mPackageInstaller;
+    @Mock List<SessionInfo> mSessions;
     private MockContentResolver mContentResolver;
     private File mDumpDir;
 
@@ -64,29 +74,53 @@ public class PreRebootLoggerTest {
     }
 
     @Before
-    public void setup() {
+    public void enableAdbConfig() {
-        MockitoAnnotations.initMocks(this);
         mContentResolver = new MockContentResolver(getInstrumentation().getTargetContext());
         when(mContext.getContentResolver()).thenReturn(mContentResolver);
         mContentResolver.addProvider(Settings.AUTHORITY, new FakeSettingsProvider());
+        Settings.Global.putInt(mContentResolver, Settings.Global.ADB_ENABLED, 1);
+    }
 
+    @Before
+    public void prepareActiveStagedSessions() {
+        when(mContext.getPackageManager()).thenReturn(mPackageManager);
+        when(mPackageManager.getPackageInstaller()).thenReturn(mPackageInstaller);
+        when(mPackageInstaller.getActiveStagedSessions()).thenReturn(mSessions);
+        when(mSessions.isEmpty()).thenReturn(false);
+    }
+
+    @Before
+    public void setupDumpDir() {
         mDumpDir = Files.createTempDir();
-        mDumpDir.mkdir();
         mDumpDir.deleteOnExit();
     }
 
     @Test
-    public void log_adbEnabled_dumpsInformationProperly() {
+    public void log_dumpsInformationProperly() {
-        Settings.Global.putInt(mContentResolver, Settings.Global.ADB_ENABLED, 1);
-
         PreRebootLogger.log(mContext, mDumpDir);
 
         assertThat(mDumpDir.list()).asList().containsExactly("system", "package", "rollback");
     }
 
+    @Test
+    public void dump_exceedTimeout_wontBlockCurrentThread() {
+        PreRebootLogger.dump(mDumpDir, 1 /* maxWaitTime */);
+
+        assertThat(mDumpDir.listFiles()).asList().containsNoneOf("system", "package", "rollback");
+    }
+
+    @Test
+    public void log_noActiveStagedSession_wipesDumpedInformation() {
+        PreRebootLogger.log(mContext, mDumpDir);
+        when(mSessions.isEmpty()).thenReturn(true);
+
+        PreRebootLogger.log(mContext, mDumpDir);
+
+        assertThat(mDumpDir.listFiles()).isEmpty();
+    }
+
     @Test
     public void log_adbDisabled_wipesDumpedInformation() {
-        Settings.Global.putInt(mContentResolver, Settings.Global.ADB_ENABLED, 1);
         PreRebootLogger.log(mContext, mDumpDir);
         Settings.Global.putInt(mContentResolver, Settings.Global.ADB_ENABLED, 0);