Merge "Ensure log buffers are dumped to file on uncaught exceptions" into tm-dev

packages/SystemUI/shared/src/com/android/systemui/shared/plugins/PluginManagerImpl.java

@@ -31,13 +31,13 @@ import android.widget.Toast;
 import com.android.internal.messages.nano.SystemMessageProto.SystemMessage;
 import com.android.systemui.plugins.Plugin;
 import com.android.systemui.plugins.PluginListener;
+import com.android.systemui.shared.system.UncaughtExceptionPreHandlerManager;
 
 import java.io.FileDescriptor;
 import java.io.PrintWriter;
 import java.lang.Thread.UncaughtExceptionHandler;
 import java.util.List;
 import java.util.Map;
-import java.util.Optional;
 
 /**
  * @see Plugin
@@ -61,7 +61,7 @@ public class PluginManagerImpl extends BroadcastReceiver implements PluginManage
     public PluginManagerImpl(Context context,
             PluginActionManager.Factory actionManagerFactory,
             boolean debuggable,
-            Optional<UncaughtExceptionHandler> defaultHandlerOptional,
+            UncaughtExceptionPreHandlerManager preHandlerManager,
             PluginEnabler pluginEnabler,
             PluginPrefs pluginPrefs,
             List<String> privilegedPlugins) {
@@ -72,9 +72,7 @@ public class PluginManagerImpl extends BroadcastReceiver implements PluginManage
         mPluginPrefs = pluginPrefs;
         mPluginEnabler = pluginEnabler;
 
-        PluginExceptionHandler uncaughtExceptionHandler = new PluginExceptionHandler(
+        preHandlerManager.registerHandler(new PluginExceptionHandler());
-                defaultHandlerOptional);
-        Thread.setUncaughtExceptionPreHandler(uncaughtExceptionHandler);
     }
 
     public boolean isDebuggable() {
@@ -266,20 +264,12 @@ public class PluginManagerImpl extends BroadcastReceiver implements PluginManage
     }
 
     private class PluginExceptionHandler implements UncaughtExceptionHandler {
-        private final Optional<UncaughtExceptionHandler> mExceptionHandlerOptional;
 
-        private PluginExceptionHandler(
+        private PluginExceptionHandler() {}
-                Optional<UncaughtExceptionHandler> exceptionHandlerOptional) {
-            mExceptionHandlerOptional = exceptionHandlerOptional;
-        }
 
         @Override
         public void uncaughtException(Thread thread, Throwable throwable) {
             if (SystemProperties.getBoolean("plugin.debugging", false)) {
-                Throwable finalThrowable = throwable;
-                mExceptionHandlerOptional.ifPresent(
-                        handler -> handler.uncaughtException(thread, finalThrowable));
-
                 return;
             }
             // Search for and disable plugins that may have been involved in this crash.
@@ -297,11 +287,6 @@ public class PluginManagerImpl extends BroadcastReceiver implements PluginManage
             if (disabledAny) {
                 throwable = new CrashWhilePluginActiveException(throwable);
             }
-
-            // Run the normal exception handler so we can crash and cleanup our state.
-            Throwable finalThrowable = throwable;
-            mExceptionHandlerOptional.ifPresent(
-                    handler -> handler.uncaughtException(thread, finalThrowable));
         }
 
         private boolean checkStack(Throwable throwable) {

packages/SystemUI/shared/src/com/android/systemui/shared/system/UncaughtExceptionPreHandlerManager.kt

@@ -0,0 +1,71 @@
+package com.android.systemui.shared.system
+
+import android.util.Log
+import java.lang.Thread.UncaughtExceptionHandler
+import java.util.concurrent.CopyOnWriteArrayList
+import javax.inject.Inject
+import javax.inject.Singleton
+
+/**
+ * Sets the global (static var in Thread) uncaught exception pre-handler to an implementation that
+ * delegates to each item in a list of registered UncaughtExceptionHandlers.
+ */
+@Singleton
+class UncaughtExceptionPreHandlerManager @Inject constructor() {
+    private val handlers: MutableList<UncaughtExceptionHandler> = CopyOnWriteArrayList()
+    private val globalUncaughtExceptionPreHandler = GlobalUncaughtExceptionHandler()
+
+    /**
+     * Adds an exception pre-handler to the list of handlers. If this has not yet set the global
+     * (static var in Thread) uncaught exception pre-handler yet, it will do so.
+     */
+    fun registerHandler(handler: UncaughtExceptionHandler) {
+        checkGlobalHandlerSetup()
+        addHandler(handler)
+    }
+
+    /**
+     * Verifies that the global handler is set in Thread. If not, sets is up.
+     */
+    private fun checkGlobalHandlerSetup() {
+        val currentHandler = Thread.getUncaughtExceptionPreHandler()
+        if (currentHandler != globalUncaughtExceptionPreHandler) {
+            if (currentHandler is GlobalUncaughtExceptionHandler) {
+                throw IllegalStateException("Two UncaughtExceptionPreHandlerManagers created")
+            }
+            currentHandler?.let { addHandler(it) }
+            Thread.setUncaughtExceptionPreHandler(globalUncaughtExceptionPreHandler)
+        }
+    }
+
+    /**
+     * Adds a handler if it has not already been added, preserving order.
+     */
+    private fun addHandler(it: UncaughtExceptionHandler) {
+        if (it !in handlers) {
+            handlers.add(it)
+        }
+    }
+
+    /**
+     * Calls uncaughtException on all registered handlers, catching and logging any new exceptions.
+     */
+    fun handleUncaughtException(thread: Thread?, throwable: Throwable?) {
+        for (handler in handlers) {
+            try {
+                handler.uncaughtException(thread, throwable)
+            } catch (e: Exception) {
+                Log.wtf("Uncaught exception pre-handler error", e)
+            }
+        }
+    }
+
+    /**
+     * UncaughtExceptionHandler impl that will be set as Thread's pre-handler static variable.
+     */
+    inner class GlobalUncaughtExceptionHandler : UncaughtExceptionHandler {
+        override fun uncaughtException(thread: Thread?, throwable: Throwable?) {
+            handleUncaughtException(thread, throwable)
+        }
+    }
+}

packages/SystemUI/src/com/android/systemui/SystemUIService.java

@@ -71,6 +71,7 @@ public class SystemUIService extends Service {
 
         // Finish initializing dump logic
         mLogBufferFreezer.attach(mBroadcastDispatcher);
+        mDumpHandler.init();
 
         // If configured, set up a battery notification
         if (getResources().getBoolean(R.bool.config_showNotificationForUnknownBatteryState)) {

packages/SystemUI/src/com/android/systemui/dump/DumpHandler.kt

@@ -25,6 +25,7 @@ import com.android.systemui.dump.DumpHandler.Companion.PRIORITY_ARG_CRITICAL
 import com.android.systemui.dump.DumpHandler.Companion.PRIORITY_ARG_HIGH
 import com.android.systemui.dump.DumpHandler.Companion.PRIORITY_ARG_NORMAL
 import com.android.systemui.log.LogBuffer
+import com.android.systemui.shared.system.UncaughtExceptionPreHandlerManager
 import java.io.PrintWriter
 import javax.inject.Inject
 import javax.inject.Provider
@@ -82,8 +83,20 @@ class DumpHandler @Inject constructor(
     private val context: Context,
     private val dumpManager: DumpManager,
     private val logBufferEulogizer: LogBufferEulogizer,
-    private val startables: MutableMap<Class<*>, Provider<CoreStartable>>
+    private val startables: MutableMap<Class<*>, Provider<CoreStartable>>,
+    private val uncaughtExceptionPreHandlerManager: UncaughtExceptionPreHandlerManager
 ) {
+    /**
+     * Registers an uncaught exception handler
+     */
+    fun init() {
+        uncaughtExceptionPreHandlerManager.registerHandler { _, e ->
+            if (e is Exception) {
+                logBufferEulogizer.record(e)
+            }
+        }
+    }
+
     /**
      * Dump the diagnostics! Behavior can be controlled via [args].
      */

packages/SystemUI/src/com/android/systemui/plugins/PluginsModule.java

@@ -16,8 +16,6 @@
 
 package com.android.systemui.plugins;
 
-import static com.android.systemui.util.concurrency.GlobalConcurrencyModule.PRE_HANDLER;
-
 import android.app.NotificationManager;
 import android.content.Context;
 import android.content.pm.PackageManager;
@@ -32,12 +30,12 @@ import com.android.systemui.shared.plugins.PluginInstance;
 import com.android.systemui.shared.plugins.PluginManager;
 import com.android.systemui.shared.plugins.PluginManagerImpl;
 import com.android.systemui.shared.plugins.PluginPrefs;
+import com.android.systemui.shared.system.UncaughtExceptionPreHandlerManager;
 import com.android.systemui.util.concurrency.GlobalConcurrencyModule;
 import com.android.systemui.util.concurrency.ThreadFactory;
 
 import java.util.Arrays;
 import java.util.List;
-import java.util.Optional;
 import java.util.concurrent.Executor;
 
 import javax.inject.Named;
@@ -107,13 +105,12 @@ public abstract class PluginsModule {
             Context context,
             PluginActionManager.Factory instanceManagerFactory,
             @Named(PLUGIN_DEBUG) boolean debug,
-            @Named(PRE_HANDLER)
+            UncaughtExceptionPreHandlerManager preHandlerManager,
-                    Optional<Thread.UncaughtExceptionHandler> uncaughtExceptionHandlerOptional,
             PluginEnabler pluginEnabler,
             PluginPrefs pluginPrefs,
             @Named(PLUGIN_PRIVILEGED) List<String> privilegedPlugins) {
         return new PluginManagerImpl(context, instanceManagerFactory, debug,
-                uncaughtExceptionHandlerOptional, pluginEnabler, pluginPrefs,
+                preHandlerManager, pluginEnabler, pluginPrefs,
                 privilegedPlugins);
     }
 

packages/SystemUI/src/com/android/systemui/util/concurrency/GlobalConcurrencyModule.java

@@ -23,11 +23,9 @@ import android.os.Looper;
 import com.android.systemui.dagger.qualifiers.Main;
 import com.android.systemui.dagger.qualifiers.UiBackground;
 
-import java.util.Optional;
 import java.util.concurrent.Executor;
 import java.util.concurrent.Executors;
 
-import javax.inject.Named;
 import javax.inject.Singleton;
 
 import dagger.Binds;
@@ -110,11 +108,4 @@ public abstract class GlobalConcurrencyModule {
     @Binds
     @Singleton
     public abstract Execution provideExecution(ExecutionImpl execution);
-
-    /** */
-    @Provides
-    @Named(PRE_HANDLER)
-    public static Optional<Thread.UncaughtExceptionHandler> providesUncaughtExceptionHandler() {
-        return Optional.ofNullable(Thread.getUncaughtExceptionPreHandler());
-    }
 }

packages/SystemUI/tests/src/com/android/systemui/dump/DumpHandlerTest.kt

@@ -20,6 +20,7 @@ import androidx.test.filters.SmallTest
 import com.android.systemui.Dumpable
 import com.android.systemui.SysuiTestCase
 import com.android.systemui.log.LogBuffer
+import com.android.systemui.shared.system.UncaughtExceptionPreHandlerManager
 import com.android.systemui.util.mockito.any
 import org.junit.Before
 import org.junit.Test
@@ -37,6 +38,8 @@ class DumpHandlerTest : SysuiTestCase() {
 
     @Mock
     private lateinit var logBufferEulogizer: LogBufferEulogizer
+    @Mock
+    private lateinit var exceptionHandlerManager: UncaughtExceptionPreHandlerManager
 
     @Mock
     private lateinit var pw: PrintWriter
@@ -59,7 +62,13 @@ class DumpHandlerTest : SysuiTestCase() {
     fun setUp() {
         MockitoAnnotations.initMocks(this)
 
-        dumpHandler = DumpHandler(mContext, dumpManager, logBufferEulogizer, mutableMapOf())
+        dumpHandler = DumpHandler(
+            mContext,
+            dumpManager,
+            logBufferEulogizer,
+            mutableMapOf(),
+            exceptionHandlerManager
+        )
     }
 
     @Test

packages/SystemUI/tests/src/com/android/systemui/shared/plugins/PluginManagerTest.java

@@ -15,6 +15,7 @@ package com.android.systemui.shared.plugins;
 
 import static org.mockito.ArgumentMatchers.anyBoolean;
 import static org.mockito.Mockito.any;
+import static org.mockito.Mockito.atLeastOnce;
 import static org.mockito.Mockito.eq;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.verify;
@@ -27,65 +28,62 @@ import android.content.Intent;
 import android.content.pm.ApplicationInfo;
 import android.content.pm.PackageManager;
 import android.net.Uri;
-import android.test.suitebuilder.annotation.SmallTest;
 import android.testing.AndroidTestingRunner;
 import android.testing.TestableLooper.RunWithLooper;
 
+import androidx.test.filters.SmallTest;
+
 import com.android.internal.messages.nano.SystemMessageProto.SystemMessage;
 import com.android.systemui.SysuiTestCase;
 import com.android.systemui.plugins.Plugin;
 import com.android.systemui.plugins.PluginListener;
 import com.android.systemui.plugins.annotations.ProvidesInterface;
+import com.android.systemui.shared.system.UncaughtExceptionPreHandlerManager;
 
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.mockito.ArgumentCaptor;
+import org.mockito.Captor;
+import org.mockito.Mock;
 import org.mockito.Mockito;
+import org.mockito.MockitoAnnotations;
 
 import java.lang.Thread.UncaughtExceptionHandler;
 import java.util.ArrayList;
 import java.util.Collections;
-import java.util.Optional;
+import java.util.List;
 
 @SmallTest
 @RunWith(AndroidTestingRunner.class)
 @RunWithLooper
 public class PluginManagerTest extends SysuiTestCase {
-
     private static final String PRIVILEGED_PACKAGE = "com.android.systemui";
 
-    private PluginActionManager.Factory mMockFactory;
+    @Mock PluginActionManager.Factory mMockFactory;
-    private PluginActionManager<TestPlugin> mMockPluginInstance;
+    @Mock PluginActionManager<TestPlugin> mMockPluginInstance;
-    private PluginManagerImpl mPluginManager;
+    @Mock PluginListener<TestPlugin> mMockListener;
-    private PluginListener<TestPlugin> mMockListener;
+    @Mock PackageManager mMockPackageManager;
-    private PackageManager mMockPackageManager;
+    @Mock PluginEnabler mMockPluginEnabler;
-    private PluginEnabler mPluginEnabler;
+    @Mock PluginPrefs mMockPluginPrefs;
-    private PluginPrefs mPluginPrefs;
+    @Mock UncaughtExceptionPreHandlerManager mMockExPreHandlerManager;
 
-    private UncaughtExceptionHandler mRealExceptionHandler;
+    @Captor ArgumentCaptor<UncaughtExceptionHandler> mExPreHandlerCaptor;
-    private UncaughtExceptionHandler mMockExceptionHandler;
+
+    private PluginManagerImpl mPluginManager;
     private UncaughtExceptionHandler mPluginExceptionHandler;
 
     @Before
     public void setup() throws Exception {
-        mRealExceptionHandler = Thread.getUncaughtExceptionPreHandler();
+        MockitoAnnotations.initMocks(this);
-        mMockExceptionHandler = mock(UncaughtExceptionHandler.class);
-        mMockFactory = mock(PluginActionManager.Factory.class);
-        mMockPluginInstance = mock(PluginActionManager.class);
-        mPluginEnabler = mock(PluginEnabler.class);
-        mPluginPrefs = mock(PluginPrefs.class);
         when(mMockFactory.create(any(), any(), eq(TestPlugin.class), anyBoolean(), anyBoolean()))
                 .thenReturn(mMockPluginInstance);
 
-        mMockPackageManager = mock(PackageManager.class);
         mPluginManager = new PluginManagerImpl(
                 getContext(), mMockFactory, true,
-                Optional.of(mMockExceptionHandler), mPluginEnabler,
+                mMockExPreHandlerManager, mMockPluginEnabler,
-                mPluginPrefs, new ArrayList<>());
+                mMockPluginPrefs, new ArrayList<>());
-
+        captureExceptionHandler();
-        resetExceptionHandler();
-        mMockListener = mock(PluginListener.class);
     }
 
     @Test
@@ -108,9 +106,9 @@ public class PluginManagerTest extends SysuiTestCase {
     public void testNonDebuggable_nonPrivileged() {
         mPluginManager = new PluginManagerImpl(
                 getContext(), mMockFactory, false,
-                Optional.of(mMockExceptionHandler), mPluginEnabler,
+                mMockExPreHandlerManager, mMockPluginEnabler,
-                mPluginPrefs, new ArrayList<>());
+                mMockPluginPrefs, new ArrayList<>());
-        resetExceptionHandler();
+        captureExceptionHandler();
 
         String sourceDir = "myPlugin";
         ApplicationInfo applicationInfo = new ApplicationInfo();
@@ -127,9 +125,9 @@ public class PluginManagerTest extends SysuiTestCase {
     public void testNonDebuggable_privilegedPackage() {
         mPluginManager = new PluginManagerImpl(
                 getContext(), mMockFactory, false,
-                Optional.of(mMockExceptionHandler), mPluginEnabler,
+                mMockExPreHandlerManager, mMockPluginEnabler,
-                mPluginPrefs, Collections.singletonList(PRIVILEGED_PACKAGE));
+                mMockPluginPrefs, Collections.singletonList(PRIVILEGED_PACKAGE));
-        resetExceptionHandler();
+        captureExceptionHandler();
 
         String sourceDir = "myPlugin";
         ApplicationInfo privilegedApplicationInfo = new ApplicationInfo();
@@ -154,9 +152,6 @@ public class PluginManagerTest extends SysuiTestCase {
         verify(mMockPluginInstance, Mockito.atLeastOnce()).checkAndDisable(
                 ArgumentCaptor.forClass(String.class).capture());
         verify(mMockPluginInstance, Mockito.never()).disableAll();
-        verify(mMockExceptionHandler).uncaughtException(
-                ArgumentCaptor.forClass(Thread.class).capture(),
-                ArgumentCaptor.forClass(Throwable.class).capture());
     }
 
     @Test
@@ -169,9 +164,6 @@ public class PluginManagerTest extends SysuiTestCase {
         verify(mMockPluginInstance, Mockito.atLeastOnce()).checkAndDisable(
                 ArgumentCaptor.forClass(String.class).capture());
         verify(mMockPluginInstance).disableAll();
-        verify(mMockExceptionHandler).uncaughtException(
-                ArgumentCaptor.forClass(Thread.class).capture(),
-                ArgumentCaptor.forClass(Throwable.class).capture());
     }
 
     @Test
@@ -186,13 +178,15 @@ public class PluginManagerTest extends SysuiTestCase {
         intent.setData(Uri.parse("package://" + testComponent.flattenToString()));
         mPluginManager.onReceive(mContext, intent);
         verify(nm).cancel(eq(testComponent.getClassName()), eq(SystemMessage.NOTE_PLUGIN));
-        verify(mPluginEnabler).setDisabled(testComponent, PluginEnabler.DISABLED_INVALID_VERSION);
+        verify(mMockPluginEnabler).setDisabled(testComponent,
+                PluginEnabler.DISABLED_INVALID_VERSION);
     }
 
-    private void resetExceptionHandler() {
+    private void captureExceptionHandler() {
-        mPluginExceptionHandler = Thread.getUncaughtExceptionPreHandler();
+        verify(mMockExPreHandlerManager, atLeastOnce()).registerHandler(
-        // Set back the real exception handler so the test can crash if it wants to.
+                mExPreHandlerCaptor.capture());
-        Thread.setUncaughtExceptionPreHandler(mRealExceptionHandler);
+        List<UncaughtExceptionHandler> allValues = mExPreHandlerCaptor.getAllValues();
+        mPluginExceptionHandler = allValues.get(allValues.size() - 1);
     }
 
     @ProvidesInterface(action = TestPlugin.ACTION, version = TestPlugin.VERSION)

packages/SystemUI/tests/src/com/android/systemui/shared/system/UncaughtExceptionPreHandlerTest.kt

@@ -0,0 +1,74 @@
+package com.android.systemui.shared.system
+
+import androidx.test.filters.SmallTest
+import com.android.systemui.SysuiTestCase
+import com.google.common.truth.Truth.assertThat
+import org.junit.Assert.assertThrows
+import org.junit.Before
+import org.junit.Test
+import org.mockito.Mock
+import org.mockito.Mockito.only
+import org.mockito.Mockito.any
+import org.mockito.Mockito.verify
+import org.mockito.Mockito.`when`
+import org.mockito.MockitoAnnotations
+import java.lang.Thread.UncaughtExceptionHandler
+
+@SmallTest
+class UncaughtExceptionPreHandlerTest : SysuiTestCase() {
+    private lateinit var preHandlerManager: UncaughtExceptionPreHandlerManager
+
+    @Mock
+    private lateinit var mockHandler: UncaughtExceptionHandler
+
+    @Mock
+    private lateinit var mockHandler2: UncaughtExceptionHandler
+
+    @Before
+    fun setUp() {
+        MockitoAnnotations.initMocks(this)
+        Thread.setUncaughtExceptionPreHandler(null)
+        preHandlerManager = UncaughtExceptionPreHandlerManager()
+    }
+
+    @Test
+    fun registerHandler_registersOnceOnly() {
+        preHandlerManager.registerHandler(mockHandler)
+        preHandlerManager.registerHandler(mockHandler)
+        preHandlerManager.handleUncaughtException(Thread.currentThread(), Exception())
+        verify(mockHandler, only()).uncaughtException(any(), any())
+    }
+
+    @Test
+    fun registerHandler_setsUncaughtExceptionPreHandler() {
+        Thread.setUncaughtExceptionPreHandler(null)
+        preHandlerManager.registerHandler(mockHandler)
+        assertThat(Thread.getUncaughtExceptionPreHandler()).isNotNull()
+    }
+
+    @Test
+    fun registerHandler_preservesOriginalHandler() {
+        Thread.setUncaughtExceptionPreHandler(mockHandler)
+        preHandlerManager.registerHandler(mockHandler2)
+        preHandlerManager.handleUncaughtException(Thread.currentThread(), Exception())
+        verify(mockHandler, only()).uncaughtException(any(), any())
+    }
+
+    @Test
+    fun registerHandler_toleratesHandlersThatThrow() {
+        `when`(mockHandler2.uncaughtException(any(), any())).thenThrow(RuntimeException())
+        preHandlerManager.registerHandler(mockHandler2)
+        preHandlerManager.registerHandler(mockHandler)
+        preHandlerManager.handleUncaughtException(Thread.currentThread(), Exception())
+        verify(mockHandler2, only()).uncaughtException(any(), any())
+        verify(mockHandler, only()).uncaughtException(any(), any())
+    }
+
+    @Test
+    fun registerHandler_doesNotSetUpTwice() {
+        UncaughtExceptionPreHandlerManager().registerHandler(mockHandler2)
+        assertThrows(IllegalStateException::class.java) {
+            preHandlerManager.registerHandler(mockHandler)
+        }
+    }
+}