From c06fb81490bbbcb3aaba0b7f3f1c65b6593c073f Mon Sep 17 00:00:00 2001 From: Greyson Parrelli Date: Wed, 3 Nov 2021 10:11:22 -0400 Subject: [PATCH] Render better crash stack traces for executors. --- .../database/SqlCipherErrorHandler.java | 6 +-- .../securesms/jobmanager/JobManager.java | 2 +- .../OptimizedMessageNotifier.java | 4 +- .../recipients/LiveRecipientCache.java | 16 ++---- .../org/thoughtcrime/securesms/util/Util.java | 27 ---------- .../org/signal/core/util/ExceptionUtil.java | 53 +++++++++++++++++++ .../java/org/signal/core/util/ThreadUtil.java | 13 +++++ .../core/util/concurrent/SignalExecutors.java | 9 ++-- .../core/util/concurrent/TracingExecutor.kt | 22 ++++++++ .../util/concurrent/TracingExecutorService.kt | 22 ++++++++ .../TracingUncaughtExceptionHandler.kt | 16 ++++++ 11 files changed, 141 insertions(+), 49 deletions(-) create mode 100644 core-util/src/main/java/org/signal/core/util/ExceptionUtil.java create mode 100644 core-util/src/main/java/org/signal/core/util/concurrent/TracingExecutor.kt create mode 100644 core-util/src/main/java/org/signal/core/util/concurrent/TracingExecutorService.kt create mode 100644 core-util/src/main/java/org/signal/core/util/concurrent/TracingUncaughtExceptionHandler.kt diff --git a/app/src/main/java/org/thoughtcrime/securesms/database/SqlCipherErrorHandler.java b/app/src/main/java/org/thoughtcrime/securesms/database/SqlCipherErrorHandler.java index cd8a4b1c9..7bde9206b 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/database/SqlCipherErrorHandler.java +++ b/app/src/main/java/org/thoughtcrime/securesms/database/SqlCipherErrorHandler.java @@ -7,9 +7,9 @@ import androidx.annotation.NonNull; import net.zetetic.database.DatabaseErrorHandler; import net.zetetic.database.sqlcipher.SQLiteDatabase; +import org.signal.core.util.ExceptionUtil; import org.signal.core.util.logging.Log; import org.thoughtcrime.securesms.util.CursorUtil; -import org.thoughtcrime.securesms.util.Util; /** * The default error handler wipes the file. This one instead prints some diagnostics and then crashes so the original corrupt file isn't lost. @@ -45,7 +45,7 @@ public final class SqlCipherErrorHandler implements DatabaseErrorHandler { } } catch (Throwable t) { output.append("Failed to do integrity_check!").append("\n") - .append(Util.convertThrowableToString(t)); + .append(ExceptionUtil.convertThrowableToString(t)); } output.append("\n").append("===== PRAGMA cipher_integrity_check =====").append("\n"); @@ -56,7 +56,7 @@ public final class SqlCipherErrorHandler implements DatabaseErrorHandler { } } catch (Throwable t) { output.append("Failed to do cipher_integrity_check!").append("\n") - .append(Util.convertThrowableToString(t)); + .append(ExceptionUtil.convertThrowableToString(t)); } Log.e(TAG, output.toString()); diff --git a/app/src/main/java/org/thoughtcrime/securesms/jobmanager/JobManager.java b/app/src/main/java/org/thoughtcrime/securesms/jobmanager/JobManager.java index ee507b908..14c090d7b 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/jobmanager/JobManager.java +++ b/app/src/main/java/org/thoughtcrime/securesms/jobmanager/JobManager.java @@ -58,7 +58,7 @@ public class JobManager implements ConstraintObserver.Notifier { public JobManager(@NonNull Application application, @NonNull Configuration configuration) { this.application = application; this.configuration = configuration; - this.executor = new FilteredExecutor(configuration.getExecutorFactory().newSingleThreadExecutor("signal-JobManager"), ThreadUtil::isMainThread); + this.executor = ThreadUtil.trace(new FilteredExecutor(configuration.getExecutorFactory().newSingleThreadExecutor("signal-JobManager"), ThreadUtil::isMainThread)); this.jobTracker = configuration.getJobTracker(); this.jobController = new JobController(application, configuration.getJobStorage(), diff --git a/app/src/main/java/org/thoughtcrime/securesms/notifications/OptimizedMessageNotifier.java b/app/src/main/java/org/thoughtcrime/securesms/notifications/OptimizedMessageNotifier.java index 3151ea3d6..39111bddb 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/notifications/OptimizedMessageNotifier.java +++ b/app/src/main/java/org/thoughtcrime/securesms/notifications/OptimizedMessageNotifier.java @@ -7,12 +7,12 @@ import android.os.Handler; import androidx.annotation.MainThread; import androidx.annotation.NonNull; +import org.signal.core.util.ExceptionUtil; import org.signal.core.util.concurrent.SignalExecutors; import org.thoughtcrime.securesms.notifications.v2.MessageNotifierV2; import org.thoughtcrime.securesms.recipients.Recipient; import org.thoughtcrime.securesms.util.BubbleUtil; import org.thoughtcrime.securesms.util.LeakyBucketLimiter; -import org.thoughtcrime.securesms.util.Util; /** * Uses a leaky-bucket strategy to limiting notification updates. @@ -109,7 +109,7 @@ public class OptimizedMessageNotifier implements MessageNotifier { try { runnable.run(); } catch (RuntimeException e) { - throw Util.appendStackTrace(e, prettyException); + throw ExceptionUtil.joinStackTrace(e, prettyException); } }); } diff --git a/app/src/main/java/org/thoughtcrime/securesms/recipients/LiveRecipientCache.java b/app/src/main/java/org/thoughtcrime/securesms/recipients/LiveRecipientCache.java index 20ff868a3..ea4282ce9 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/recipients/LiveRecipientCache.java +++ b/app/src/main/java/org/thoughtcrime/securesms/recipients/LiveRecipientCache.java @@ -9,7 +9,9 @@ import androidx.annotation.NonNull; import net.zetetic.database.sqlcipher.SQLiteDatabase; +import org.signal.core.util.ThreadUtil; import org.signal.core.util.concurrent.SignalExecutors; +import org.signal.core.util.concurrent.TracingExecutorService; import org.signal.core.util.logging.Log; import org.thoughtcrime.securesms.database.DatabaseFactory; import org.thoughtcrime.securesms.database.RecipientDatabase; @@ -28,7 +30,6 @@ import java.util.ArrayList; import java.util.Collection; import java.util.List; import java.util.Map; -import java.util.UUID; import java.util.concurrent.Executor; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; @@ -60,7 +61,7 @@ public final class LiveRecipientCache { this.localRecipientId = new AtomicReference<>(null); this.unknown = new LiveRecipient(context, Recipient.UNKNOWN); this.db = DatabaseFactory.getInstance(context).getRawDatabase(); - this.resolveExecutor = new FilteredExecutor(SignalExecutors.BOUNDED, () -> !db.inTransaction()); + this.resolveExecutor = ThreadUtil.trace(new FilteredExecutor(SignalExecutors.BOUNDED, () -> !db.inTransaction())); } @AnyThread @@ -83,16 +84,7 @@ public final class LiveRecipientCache { } if (needsResolve) { - final LiveRecipient toResolve = live; - - MissingRecipientException prettyStackTraceError = new MissingRecipientException(toResolve.getId()); - resolveExecutor.execute(() -> { - try { - toResolve.resolve(); - } catch (MissingRecipientException e) { - throw prettyStackTraceError; - } - }); + resolveExecutor.execute(live::resolve); } return live; diff --git a/app/src/main/java/org/thoughtcrime/securesms/util/Util.java b/app/src/main/java/org/thoughtcrime/securesms/util/Util.java index 7bcc33423..1918f8eb1 100644 --- a/app/src/main/java/org/thoughtcrime/securesms/util/Util.java +++ b/app/src/main/java/org/thoughtcrime/securesms/util/Util.java @@ -515,31 +515,4 @@ public class Util { return defaultValue; } } - - /** - * Appends the stack trace of the provided throwable onto the provided primary exception. This is - * useful for when exceptions are thrown inside of asynchronous systems (like runnables in an - * executor) where you'd otherwise lose important parts of the stack trace. This lets you save a - * throwable at the entry point, and then combine it with any caught exceptions later. - * - * @return The provided primary exception, for convenience. - */ - public static RuntimeException appendStackTrace(@NonNull RuntimeException primary, @NonNull Throwable secondary) { - StackTraceElement[] now = primary.getStackTrace(); - StackTraceElement[] then = secondary.getStackTrace(); - StackTraceElement[] combined = new StackTraceElement[now.length + then.length]; - - System.arraycopy(now, 0, combined, 0, now.length); - System.arraycopy(then, 0, combined, now.length, then.length); - - primary.setStackTrace(combined); - - return primary; - } - - public static @NonNull String convertThrowableToString(@NonNull Throwable throwable) { - ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); - throwable.printStackTrace(new PrintStream(outputStream)); - return outputStream.toString(); - } } diff --git a/core-util/src/main/java/org/signal/core/util/ExceptionUtil.java b/core-util/src/main/java/org/signal/core/util/ExceptionUtil.java new file mode 100644 index 000000000..d759ec28d --- /dev/null +++ b/core-util/src/main/java/org/signal/core/util/ExceptionUtil.java @@ -0,0 +1,53 @@ +package org.signal.core.util; + +import androidx.annotation.NonNull; + +import java.io.ByteArrayOutputStream; +import java.io.PrintStream; + +public final class ExceptionUtil { + + private ExceptionUtil() {} + + /** + * Joins the stack trace of the inferred call site with the original exception. This is + * useful for when exceptions are thrown inside of asynchronous systems (like runnables in an + * executor) where you'd otherwise lose important parts of the stack trace. This lets you save a + * throwable at the entry point, and then combine it with any caught exceptions later. + * + * The resulting stack trace will look like this: + * + * Inferred + * Stack + * Trace + * [[ ↑↑ Inferred Trace ↑↑ ]] + * [[ ↓↓ Original Trace ↓↓ ]] + * Original + * Stack + * Trace + * + * @return The provided original exception, for convenience. + */ + public static E joinStackTrace(@NonNull E original, @NonNull Throwable inferred) { + StackTraceElement[] originalTrace = original.getStackTrace(); + StackTraceElement[] inferredTrace = inferred.getStackTrace(); + StackTraceElement[] combinedTrace = new StackTraceElement[originalTrace.length + inferredTrace.length + 2]; + + System.arraycopy(originalTrace, 0, combinedTrace, 0, originalTrace.length); + + combinedTrace[originalTrace.length] = new StackTraceElement("[[ ↑↑ Original Trace ↑↑ ]]", "", "", 0); + combinedTrace[originalTrace.length + 1] = new StackTraceElement("[[ ↓↓ Inferred Trace ↓↓ ]]", "", "", 0); + + System.arraycopy(inferredTrace, 0, combinedTrace, originalTrace.length + 2, inferredTrace.length); + + original.setStackTrace(combinedTrace); + + return original; + } + + public static @NonNull String convertThrowableToString(@NonNull Throwable throwable) { + ByteArrayOutputStream outputStream = new ByteArrayOutputStream(); + throwable.printStackTrace(new PrintStream(outputStream)); + return outputStream.toString(); + } +} diff --git a/core-util/src/main/java/org/signal/core/util/ThreadUtil.java b/core-util/src/main/java/org/signal/core/util/ThreadUtil.java index 7c2b02d39..a05961187 100644 --- a/core-util/src/main/java/org/signal/core/util/ThreadUtil.java +++ b/core-util/src/main/java/org/signal/core/util/ThreadUtil.java @@ -5,7 +5,12 @@ import android.os.Looper; import androidx.annotation.NonNull; +import org.signal.core.util.concurrent.TracingExecutor; +import org.signal.core.util.concurrent.TracingExecutorService; + import java.util.concurrent.CountDownLatch; +import java.util.concurrent.Executor; +import java.util.concurrent.ExecutorService; /** * Thread related utility functions. @@ -93,4 +98,12 @@ public final class ThreadUtil { Thread.sleep(millis); } catch (InterruptedException ignored) { } } + + public static Executor trace(Executor executor) { + return new TracingExecutor(executor); + } + + public static ExecutorService trace(ExecutorService executor) { + return new TracingExecutorService(executor); + } } diff --git a/core-util/src/main/java/org/signal/core/util/concurrent/SignalExecutors.java b/core-util/src/main/java/org/signal/core/util/concurrent/SignalExecutors.java index 91d4efe96..4bc6b926c 100644 --- a/core-util/src/main/java/org/signal/core/util/concurrent/SignalExecutors.java +++ b/core-util/src/main/java/org/signal/core/util/concurrent/SignalExecutors.java @@ -5,6 +5,7 @@ import android.os.HandlerThread; import androidx.annotation.NonNull; import org.signal.core.util.LinkedBlockingLifoQueue; +import org.signal.core.util.ThreadUtil; import java.util.concurrent.ExecutorService; import java.util.concurrent.Executors; @@ -16,10 +17,10 @@ import java.util.concurrent.atomic.AtomicInteger; public final class SignalExecutors { - public static final ExecutorService UNBOUNDED = Executors.newCachedThreadPool(new NumberedThreadFactory("signal-unbounded")); - public static final ExecutorService BOUNDED = Executors.newFixedThreadPool(4, new NumberedThreadFactory("signal-bounded")); - public static final ExecutorService SERIAL = Executors.newSingleThreadExecutor(new NumberedThreadFactory("signal-serial")); - public static final ExecutorService BOUNDED_IO = newCachedBoundedExecutor("signal-io-bounded", 1, 32, 30); + public static final ExecutorService UNBOUNDED = ThreadUtil.trace(Executors.newCachedThreadPool(new NumberedThreadFactory("signal-unbounded"))); + public static final ExecutorService BOUNDED = ThreadUtil.trace(Executors.newFixedThreadPool(4, new NumberedThreadFactory("signal-bounded"))); + public static final ExecutorService SERIAL = ThreadUtil.trace(Executors.newSingleThreadExecutor(new NumberedThreadFactory("signal-serial"))); + public static final ExecutorService BOUNDED_IO = ThreadUtil.trace(newCachedBoundedExecutor("signal-io-bounded", 1, 32, 30)); private SignalExecutors() {} diff --git a/core-util/src/main/java/org/signal/core/util/concurrent/TracingExecutor.kt b/core-util/src/main/java/org/signal/core/util/concurrent/TracingExecutor.kt new file mode 100644 index 000000000..324001b53 --- /dev/null +++ b/core-util/src/main/java/org/signal/core/util/concurrent/TracingExecutor.kt @@ -0,0 +1,22 @@ +package org.signal.core.util.concurrent + +import java.util.concurrent.Executor + +/** + * An executor that will keep track of the stack trace at the time of calling [execute] and use that to build a more useful stack trace in the event of a crash. + */ +internal class TracingExecutor(val wrapped: Executor) : Executor by wrapped { + + override fun execute(command: Runnable?) { + val callerStackTrace = Throwable() + + wrapped.execute { + val currentHandler: Thread.UncaughtExceptionHandler? = Thread.currentThread().uncaughtExceptionHandler + val originalHandler: Thread.UncaughtExceptionHandler? = if (currentHandler is TracingUncaughtExceptionHandler) currentHandler.originalHandler else currentHandler + + Thread.currentThread().uncaughtExceptionHandler = TracingUncaughtExceptionHandler(originalHandler, callerStackTrace) + + command?.run() + } + } +} \ No newline at end of file diff --git a/core-util/src/main/java/org/signal/core/util/concurrent/TracingExecutorService.kt b/core-util/src/main/java/org/signal/core/util/concurrent/TracingExecutorService.kt new file mode 100644 index 000000000..44c6ebfde --- /dev/null +++ b/core-util/src/main/java/org/signal/core/util/concurrent/TracingExecutorService.kt @@ -0,0 +1,22 @@ +package org.signal.core.util.concurrent + +import java.util.concurrent.ExecutorService + +/** + * An executor that will keep track of the stack trace at the time of calling [execute] and use that to build a more useful stack trace in the event of a crash. + */ +internal class TracingExecutorService(val wrapped: ExecutorService) : ExecutorService by wrapped { + + override fun execute(command: Runnable?) { + val callerStackTrace = Throwable() + + wrapped.execute { + val currentHandler: Thread.UncaughtExceptionHandler? = Thread.currentThread().uncaughtExceptionHandler + val originalHandler: Thread.UncaughtExceptionHandler? = if (currentHandler is TracingUncaughtExceptionHandler) currentHandler.originalHandler else currentHandler + + Thread.currentThread().uncaughtExceptionHandler = TracingUncaughtExceptionHandler(originalHandler, callerStackTrace) + + command?.run() + } + } +} \ No newline at end of file diff --git a/core-util/src/main/java/org/signal/core/util/concurrent/TracingUncaughtExceptionHandler.kt b/core-util/src/main/java/org/signal/core/util/concurrent/TracingUncaughtExceptionHandler.kt new file mode 100644 index 000000000..8ce330b9a --- /dev/null +++ b/core-util/src/main/java/org/signal/core/util/concurrent/TracingUncaughtExceptionHandler.kt @@ -0,0 +1,16 @@ +package org.signal.core.util.concurrent + +import org.signal.core.util.ExceptionUtil + +/** + * An uncaught exception handler that will combine a caller stack trace with the exception to print a more useful stack trace. + */ +internal class TracingUncaughtExceptionHandler ( + val originalHandler: Thread.UncaughtExceptionHandler?, + private val callerStackTrace: Throwable) : Thread.UncaughtExceptionHandler { + + override fun uncaughtException(thread: Thread, exception: Throwable) { + val updated = ExceptionUtil.joinStackTrace(exception, callerStackTrace) + originalHandler?.uncaughtException(thread, updated) + } +} \ No newline at end of file