From ae44314b3658fe780a0be1a1832a18e8b1e9be40 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Tue, 25 Apr 2023 09:05:55 +0200 Subject: [PATCH 1/4] Fix ensure screenshots and view hierarchy are captured on the main thread --- .../api/sentry-android-core.api | 4 +- .../core/ScreenshotEventProcessor.java | 4 +- .../core/ViewHierarchyEventProcessor.java | 41 +++++++++++++++---- .../core/internal/util/ScreenshotUtils.java | 23 ++++++++++- .../core/ScreenshotEventProcessorTest.kt | 26 ++++++++++++ .../core/ViewHierarchyEventProcessorTest.kt | 26 ++++++++++++ 6 files changed, 112 insertions(+), 12 deletions(-) diff --git a/sentry-android-core/api/sentry-android-core.api b/sentry-android-core/api/sentry-android-core.api index eb8f89f062..298c66f72b 100644 --- a/sentry-android-core/api/sentry-android-core.api +++ b/sentry-android-core/api/sentry-android-core.api @@ -291,9 +291,9 @@ public final class io/sentry/android/core/UserInteractionIntegration : android/a public final class io/sentry/android/core/ViewHierarchyEventProcessor : io/sentry/EventProcessor { public fun (Lio/sentry/android/core/SentryAndroidOptions;)V public fun process (Lio/sentry/SentryEvent;Lio/sentry/Hint;)Lio/sentry/SentryEvent; - public static fun snapshotViewHierarchy (Landroid/app/Activity;Lio/sentry/ILogger;)Lio/sentry/protocol/ViewHierarchy; + public static fun snapshotViewHierarchy (Landroid/app/Activity;Lio/sentry/util/thread/IMainThreadChecker;Lio/sentry/ILogger;)Lio/sentry/protocol/ViewHierarchy; public static fun snapshotViewHierarchy (Landroid/view/View;)Lio/sentry/protocol/ViewHierarchy; - public static fun snapshotViewHierarchyAsData (Landroid/app/Activity;Lio/sentry/ISerializer;Lio/sentry/ILogger;)[B + public static fun snapshotViewHierarchyAsData (Landroid/app/Activity;Lio/sentry/util/thread/IMainThreadChecker;Lio/sentry/ISerializer;Lio/sentry/ILogger;)[B } public final class io/sentry/android/core/cache/AndroidEnvelopeCache : io/sentry/cache/EnvelopeCache { diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java b/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java index 550634d309..aa1daa0b96 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/ScreenshotEventProcessor.java @@ -48,7 +48,9 @@ public ScreenshotEventProcessor( return event; } - final byte[] screenshot = takeScreenshot(activity, options.getLogger(), buildInfoProvider); + final byte[] screenshot = + takeScreenshot( + activity, options.getMainThreadChecker(), options.getLogger(), buildInfoProvider); if (screenshot == null) { return event; } diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java b/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java index 675715a383..ee9a52fb7a 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java @@ -17,8 +17,12 @@ import io.sentry.util.HintUtils; import io.sentry.util.JsonSerializationUtils; import io.sentry.util.Objects; +import io.sentry.util.thread.IMainThreadChecker; import java.util.ArrayList; import java.util.List; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import java.util.concurrent.atomic.AtomicReference; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -50,7 +54,7 @@ public ViewHierarchyEventProcessor(final @NotNull SentryAndroidOptions options) final @Nullable Activity activity = CurrentActivityHolder.getInstance().getActivity(); final @Nullable ViewHierarchy viewHierarchy = - snapshotViewHierarchy(activity, options.getLogger()); + snapshotViewHierarchy(activity, options.getMainThreadChecker(), options.getLogger()); if (viewHierarchy != null) { hint.setViewHierarchy(Attachment.fromViewHierarchy(viewHierarchy)); @@ -59,10 +63,13 @@ public ViewHierarchyEventProcessor(final @NotNull SentryAndroidOptions options) return event; } - @Nullable public static byte[] snapshotViewHierarchyAsData( - @Nullable Activity activity, @NotNull ISerializer serializer, @NotNull ILogger logger) { - @Nullable ViewHierarchy viewHierarchy = snapshotViewHierarchy(activity, logger); + @Nullable Activity activity, + @NotNull IMainThreadChecker mainThreadChecker, + @NotNull ISerializer serializer, + @NotNull ILogger logger) { + @Nullable + ViewHierarchy viewHierarchy = snapshotViewHierarchy(activity, mainThreadChecker, logger); if (viewHierarchy == null) { logger.log(SentryLevel.ERROR, "Could not get ViewHierarchy."); @@ -85,7 +92,9 @@ public static byte[] snapshotViewHierarchyAsData( @Nullable public static ViewHierarchy snapshotViewHierarchy( - @Nullable Activity activity, @NotNull ILogger logger) { + @Nullable Activity activity, + @NotNull IMainThreadChecker mainThreadChecker, + @NotNull ILogger logger) { if (activity == null) { logger.log(SentryLevel.INFO, "Missing activity for view hierarchy snapshot."); return null; @@ -104,12 +113,28 @@ public static ViewHierarchy snapshotViewHierarchy( } try { - final @NotNull ViewHierarchy viewHierarchy = snapshotViewHierarchy(decorView); - return viewHierarchy; + if (mainThreadChecker.isMainThread()) { + return snapshotViewHierarchy(decorView); + } else { + final CountDownLatch latch = new CountDownLatch(1); + final AtomicReference viewHierarchy = new AtomicReference<>(null); + activity.runOnUiThread( + () -> { + try { + viewHierarchy.set(snapshotViewHierarchy(decorView)); + latch.countDown(); + } catch (Throwable t) { + logger.log(SentryLevel.ERROR, "Failed to process view hierarchy.", t); + } + }); + if (latch.await(1, TimeUnit.SECONDS)) { + return viewHierarchy.get(); + } + } } catch (Throwable t) { logger.log(SentryLevel.ERROR, "Failed to process view hierarchy.", t); - return null; } + return null; } @NotNull diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/ScreenshotUtils.java b/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/ScreenshotUtils.java index b9a528382e..f668c07a2a 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/ScreenshotUtils.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/ScreenshotUtils.java @@ -10,7 +10,10 @@ import io.sentry.ILogger; import io.sentry.SentryLevel; import io.sentry.android.core.BuildInfoProvider; +import io.sentry.util.thread.IMainThreadChecker; import java.io.ByteArrayOutputStream; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; import org.jetbrains.annotations.ApiStatus; import org.jetbrains.annotations.NotNull; @@ -18,8 +21,10 @@ public class ScreenshotUtils { public static @Nullable byte[] takeScreenshot( final @NotNull Activity activity, + final @NotNull IMainThreadChecker mainThreadChecker, final @NotNull ILogger logger, final @NotNull BuildInfoProvider buildInfoProvider) { + if (!isActivityValid(activity, buildInfoProvider) || activity.getWindow() == null || activity.getWindow().getDecorView() == null @@ -40,7 +45,23 @@ public class ScreenshotUtils { Bitmap.createBitmap(view.getWidth(), view.getHeight(), Bitmap.Config.ARGB_8888); final Canvas canvas = new Canvas(bitmap); - view.draw(canvas); + if (mainThreadChecker.isMainThread()) { + view.draw(canvas); + } else { + final @NotNull CountDownLatch latch = new CountDownLatch(1); + activity.runOnUiThread( + () -> { + try { + view.draw(canvas); + latch.countDown(); + } catch (Throwable e) { + logger.log(SentryLevel.ERROR, "Taking screenshot failed (view.draw).", e); + } + }); + if (!latch.await(1, TimeUnit.SECONDS)) { + return null; + } + } // 0 meaning compress for small size, 100 meaning compress for max quality. // Some formats, like PNG which is lossless, will ignore the quality setting. diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt index 2d208db720..99d3377792 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ScreenshotEventProcessorTest.kt @@ -9,12 +9,16 @@ import io.sentry.Hint import io.sentry.MainEventProcessor import io.sentry.SentryEvent import io.sentry.TypeCheckHint.ANDROID_ACTIVITY +import io.sentry.util.thread.IMainThreadChecker import org.junit.runner.RunWith +import org.mockito.kotlin.any import org.mockito.kotlin.mock +import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import kotlin.test.BeforeTest import kotlin.test.Test import kotlin.test.assertEquals +import kotlin.test.assertNotNull import kotlin.test.assertNull import kotlin.test.assertSame import kotlin.test.assertTrue @@ -28,6 +32,7 @@ class ScreenshotEventProcessorTest { val window = mock() val view = mock() val rootView = mock() + val mainThreadChecker = mock() val options = SentryAndroidOptions().apply { dsn = "https://key@sentry.io/proj" } @@ -39,10 +44,16 @@ class ScreenshotEventProcessorTest { whenever(view.rootView).thenReturn(rootView) whenever(window.decorView).thenReturn(view) whenever(activity.window).thenReturn(window) + whenever(activity.runOnUiThread(any())).then { + it.getArgument(0).run() + } + + whenever(mainThreadChecker.isMainThread).thenReturn(true) } fun getSut(attachScreenshot: Boolean = false): ScreenshotEventProcessor { options.isAttachScreenshot = attachScreenshot + options.mainThreadChecker = mainThreadChecker return ScreenshotEventProcessor(options, buildInfo) } @@ -153,5 +164,20 @@ class ScreenshotEventProcessorTest { assertNull(hint.screenshot) } + @Test + fun `when screenshot event processor is called from background thread it executes on main thread`() { + val sut = fixture.getSut(true) + whenever(fixture.mainThreadChecker.isMainThread).thenReturn(false) + + CurrentActivityHolder.getInstance().setActivity(fixture.activity) + + val hint = Hint() + val event = fixture.mainProcessor.process(getEvent(), hint) + sut.process(event, hint) + + verify(fixture.activity).runOnUiThread(any()) + assertNotNull(hint.screenshot) + } + private fun getEvent(): SentryEvent = SentryEvent(Throwable("Throwable")) } diff --git a/sentry-android-core/src/test/java/io/sentry/android/core/ViewHierarchyEventProcessorTest.kt b/sentry-android-core/src/test/java/io/sentry/android/core/ViewHierarchyEventProcessorTest.kt index a74e83858a..15470d5b36 100644 --- a/sentry-android-core/src/test/java/io/sentry/android/core/ViewHierarchyEventProcessorTest.kt +++ b/sentry-android-core/src/test/java/io/sentry/android/core/ViewHierarchyEventProcessorTest.kt @@ -11,10 +11,12 @@ import io.sentry.JsonSerializer import io.sentry.SentryEvent import io.sentry.TypeCheckHint import io.sentry.protocol.SentryException +import io.sentry.util.thread.IMainThreadChecker import org.junit.runner.RunWith import org.mockito.invocation.InvocationOnMock import org.mockito.kotlin.any import org.mockito.kotlin.mock +import org.mockito.kotlin.verify import org.mockito.kotlin.whenever import java.io.Writer import kotlin.test.BeforeTest @@ -42,6 +44,7 @@ class ViewHierarchyEventProcessorTest { } } val activity = mock() + val mainThreadChecker = mock() val window = mock() val view = mock() val options = SentryAndroidOptions().apply { @@ -54,12 +57,17 @@ class ViewHierarchyEventProcessorTest { whenever(window.decorView).thenReturn(view) whenever(window.peekDecorView()).thenReturn(view) whenever(activity.window).thenReturn(window) + whenever(activity.runOnUiThread(any())).then { + it.getArgument(0).run() + } + whenever(mainThreadChecker.isMainThread).thenReturn(true) CurrentActivityHolder.getInstance().setActivity(activity) } fun getSut(attachViewHierarchy: Boolean = false): ViewHierarchyEventProcessor { options.isAttachViewHierarchy = attachViewHierarchy + options.mainThreadChecker = mainThreadChecker return ViewHierarchyEventProcessor(options) } @@ -86,6 +94,7 @@ class ViewHierarchyEventProcessorTest { fun `should return a view hierarchy as byte array`() { val viewHierarchy = ViewHierarchyEventProcessor.snapshotViewHierarchyAsData( fixture.activity, + fixture.mainThreadChecker, fixture.serializer, fixture.logger ) @@ -98,6 +107,7 @@ class ViewHierarchyEventProcessorTest { fun `should return null as bytes are empty array`() { val viewHierarchy = ViewHierarchyEventProcessor.snapshotViewHierarchyAsData( fixture.activity, + fixture.mainThreadChecker, fixture.emptySerializer, fixture.logger ) @@ -147,6 +157,22 @@ class ViewHierarchyEventProcessorTest { assertNotNull(hint.viewHierarchy) } + @Test + fun `when an event errored in the background, the view hierarchy should captured on the main thread`() { + whenever(fixture.mainThreadChecker.isMainThread).thenReturn(false) + + val (event, hint) = fixture.process( + true, + SentryEvent().apply { + exceptions = listOf(SentryException()) + } + ) + + verify(fixture.activity).runOnUiThread(any()) + assertNotNull(event) + assertNotNull(hint.viewHierarchy) + } + @Test fun `when an event did not error, the view hierarchy should be attached if the feature is enabled`() { val (event, hint) = fixture.process( From dc9b01383134ed338cfe88c3783391d78a358135 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Wed, 17 May 2023 16:44:21 +0200 Subject: [PATCH 2/4] Update Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f1d09eae0..53915c1880 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -25,6 +25,7 @@ - Remove null keys/values before creating concurrent hashmap in order to avoid NPE ([#2708](https://github.com/getsentry/sentry-java/pull/2708)) - Exclude SentryOptions from R8/ProGuard obfuscation ([#2699](https://github.com/getsentry/sentry-java/pull/2699)) - This fixes AGP 8.+ incompatibility, where full R8 mode is enforced +- Ensure screenshots and view hierarchies are captured on the main thread ([#2712](https://github.com/getsentry/sentry-java/pull/2712)) ### Dependencies From 0d44e90d8a428beed8a7a366bc37116071147af2 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Wed, 17 May 2023 16:51:16 +0200 Subject: [PATCH 3/4] Ensure non-breaking hybrid changes --- sentry-android-core/api/sentry-android-core.api | 1 + .../android/core/ViewHierarchyEventProcessor.java | 10 +++++++++- .../android/core/internal/util/ScreenshotUtils.java | 13 ++++++++++++- 3 files changed, 22 insertions(+), 2 deletions(-) diff --git a/sentry-android-core/api/sentry-android-core.api b/sentry-android-core/api/sentry-android-core.api index 2a00cd461b..d28fcfab8b 100644 --- a/sentry-android-core/api/sentry-android-core.api +++ b/sentry-android-core/api/sentry-android-core.api @@ -316,6 +316,7 @@ public final class io/sentry/android/core/UserInteractionIntegration : android/a public final class io/sentry/android/core/ViewHierarchyEventProcessor : io/sentry/EventProcessor, io/sentry/IntegrationName { public fun (Lio/sentry/android/core/SentryAndroidOptions;)V public fun process (Lio/sentry/SentryEvent;Lio/sentry/Hint;)Lio/sentry/SentryEvent; + public static fun snapshotViewHierarchy (Landroid/app/Activity;Lio/sentry/ILogger;)Lio/sentry/protocol/ViewHierarchy; public static fun snapshotViewHierarchy (Landroid/app/Activity;Lio/sentry/util/thread/IMainThreadChecker;Lio/sentry/ILogger;)Lio/sentry/protocol/ViewHierarchy; public static fun snapshotViewHierarchy (Landroid/view/View;)Lio/sentry/protocol/ViewHierarchy; public static fun snapshotViewHierarchyAsData (Landroid/app/Activity;Lio/sentry/util/thread/IMainThreadChecker;Lio/sentry/ISerializer;Lio/sentry/ILogger;)[B diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java b/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java index 0e41d95172..6c051cb796 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/ViewHierarchyEventProcessor.java @@ -13,6 +13,7 @@ import io.sentry.SentryEvent; import io.sentry.SentryLevel; import io.sentry.android.core.internal.gestures.ViewUtils; +import io.sentry.android.core.internal.util.AndroidMainThreadChecker; import io.sentry.protocol.ViewHierarchy; import io.sentry.protocol.ViewHierarchyNode; import io.sentry.util.HintUtils; @@ -33,6 +34,7 @@ public final class ViewHierarchyEventProcessor implements EventProcessor, IntegrationName { private final @NotNull SentryAndroidOptions options; + private static final long CAPTURE_TIMEOUT_MS = 1000; public ViewHierarchyEventProcessor(final @NotNull SentryAndroidOptions options) { this.options = Objects.requireNonNull(options, "SentryAndroidOptions is required"); @@ -94,6 +96,12 @@ public static byte[] snapshotViewHierarchyAsData( return bytes; } + @Nullable + public static ViewHierarchy snapshotViewHierarchy( + @Nullable Activity activity, @NotNull ILogger logger) { + return snapshotViewHierarchy(activity, AndroidMainThreadChecker.getInstance(), logger); + } + @Nullable public static ViewHierarchy snapshotViewHierarchy( @Nullable Activity activity, @@ -131,7 +139,7 @@ public static ViewHierarchy snapshotViewHierarchy( logger.log(SentryLevel.ERROR, "Failed to process view hierarchy.", t); } }); - if (latch.await(1, TimeUnit.SECONDS)) { + if (latch.await(CAPTURE_TIMEOUT_MS, TimeUnit.MILLISECONDS)) { return viewHierarchy.get(); } } diff --git a/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/ScreenshotUtils.java b/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/ScreenshotUtils.java index f668c07a2a..88f3e126de 100644 --- a/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/ScreenshotUtils.java +++ b/sentry-android-core/src/main/java/io/sentry/android/core/internal/util/ScreenshotUtils.java @@ -19,6 +19,17 @@ @ApiStatus.Internal public class ScreenshotUtils { + + private static final long CAPTURE_TIMEOUT_MS = 1000; + + public static @Nullable byte[] takeScreenshot( + final @NotNull Activity activity, + final @NotNull ILogger logger, + final @NotNull BuildInfoProvider buildInfoProvider) { + return takeScreenshot( + activity, AndroidMainThreadChecker.getInstance(), logger, buildInfoProvider); + } + public static @Nullable byte[] takeScreenshot( final @NotNull Activity activity, final @NotNull IMainThreadChecker mainThreadChecker, @@ -58,7 +69,7 @@ public class ScreenshotUtils { logger.log(SentryLevel.ERROR, "Taking screenshot failed (view.draw).", e); } }); - if (!latch.await(1, TimeUnit.SECONDS)) { + if (!latch.await(CAPTURE_TIMEOUT_MS, TimeUnit.MILLISECONDS)) { return null; } } From f472319124319740c1627106e5211b591cdd5ab9 Mon Sep 17 00:00:00 2001 From: Markus Hintersteiner Date: Wed, 17 May 2023 16:55:00 +0200 Subject: [PATCH 4/4] Fix Changelog --- CHANGELOG.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 53915c1880..fc0d177263 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Fixes + +- Ensure screenshots and view hierarchies are captured on the main thread ([#2712](https://github.com/getsentry/sentry-java/pull/2712)) + ## 6.19.0 ### Features @@ -25,7 +31,6 @@ - Remove null keys/values before creating concurrent hashmap in order to avoid NPE ([#2708](https://github.com/getsentry/sentry-java/pull/2708)) - Exclude SentryOptions from R8/ProGuard obfuscation ([#2699](https://github.com/getsentry/sentry-java/pull/2699)) - This fixes AGP 8.+ incompatibility, where full R8 mode is enforced -- Ensure screenshots and view hierarchies are captured on the main thread ([#2712](https://github.com/getsentry/sentry-java/pull/2712)) ### Dependencies