Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Ensure screenshots and view hierarchies are captured on the main thread #2712

Merged
merged 6 commits into from
May 17, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -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
Expand Down
3 changes: 2 additions & 1 deletion sentry-android-core/api/sentry-android-core.api
Original file line number Diff line number Diff line change
Expand Up @@ -317,8 +317,9 @@ public final class io/sentry/android/core/ViewHierarchyEventProcessor : io/sentr
public fun <init> (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 {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,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;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,13 +13,18 @@
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;
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;
Expand All @@ -29,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");
Expand All @@ -54,7 +60,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));
Expand All @@ -63,10 +69,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.");
Expand All @@ -90,6 +99,14 @@ public static byte[] snapshotViewHierarchyAsData(
@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,
@NotNull IMainThreadChecker mainThreadChecker,
@NotNull ILogger logger) {
if (activity == null) {
logger.log(SentryLevel.INFO, "Missing activity for view hierarchy snapshot.");
return null;
Expand All @@ -108,12 +125,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> viewHierarchy = new AtomicReference<>(null);
activity.runOnUiThread(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

l: potentially we could do handler.postAtFrontOfQueue instead to capture the VH/SS as soon as possible, but I don't know what could be the potential side effects of this. WDYT?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm that's a good one. Reading up on the docs, I wouldn't do it - at least not within this PR - maybe as a follow up investigation, as it could be relevant for other parts within our code as well?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid activity.runOnUiThread in favor of MainLooperHandler for testability.
See the previous comment related to this issue as well:
#2281 (comment)
Added a new comment here #2515 (comment)

() -> {
try {
viewHierarchy.set(snapshotViewHierarchy(decorView));
latch.countDown();
} catch (Throwable t) {
logger.log(SentryLevel.ERROR, "Failed to process view hierarchy.", t);
}
});
if (latch.await(CAPTURE_TIMEOUT_MS, TimeUnit.MILLISECONDS)) {
return viewHierarchy.get();
}
}
} catch (Throwable t) {
logger.log(SentryLevel.ERROR, "Failed to process view hierarchy.", t);
return null;
}
return null;
}

@NotNull
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,16 +10,32 @@
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;

@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,
final @NotNull ILogger logger,
final @NotNull BuildInfoProvider buildInfoProvider) {

if (!isActivityValid(activity, buildInfoProvider)
|| activity.getWindow() == null
|| activity.getWindow().getDecorView() == null
Expand All @@ -40,7 +56,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(CAPTURE_TIMEOUT_MS, TimeUnit.MILLISECONDS)) {
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.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,13 +10,17 @@ import io.sentry.MainEventProcessor
import io.sentry.SentryEvent
import io.sentry.SentryIntegrationPackageStorage
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.assertFalse
import kotlin.test.assertNotNull
import kotlin.test.assertNull
import kotlin.test.assertSame
import kotlin.test.assertTrue
Expand All @@ -30,6 +34,7 @@ class ScreenshotEventProcessorTest {
val window = mock<Window>()
val view = mock<View>()
val rootView = mock<View>()
val mainThreadChecker = mock<IMainThreadChecker>()
val options = SentryAndroidOptions().apply {
dsn = "https://key@sentry.io/proj"
}
Expand All @@ -41,10 +46,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<Runnable>(0).run()
}

whenever(mainThreadChecker.isMainThread).thenReturn(true)
}

fun getSut(attachScreenshot: Boolean = false): ScreenshotEventProcessor {
options.isAttachScreenshot = attachScreenshot
options.mainThreadChecker = mainThreadChecker

return ScreenshotEventProcessor(options, buildInfo)
}
Expand Down Expand Up @@ -156,6 +167,20 @@ class ScreenshotEventProcessorTest {
}

@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)
}

fun `when enabled, the feature is added to the integration list`() {
SentryIntegrationPackageStorage.getInstance().clearStorage()
val hint = Hint()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,12 @@ import io.sentry.SentryEvent
import io.sentry.SentryIntegrationPackageStorage
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
Expand Down Expand Up @@ -44,6 +46,7 @@ class ViewHierarchyEventProcessorTest {
}
}
val activity = mock<Activity>()
val mainThreadChecker = mock<IMainThreadChecker>()
val window = mock<Window>()
val view = mock<View>()
val options = SentryAndroidOptions().apply {
Expand All @@ -56,12 +59,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<Runnable>(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)
}

Expand All @@ -88,6 +96,7 @@ class ViewHierarchyEventProcessorTest {
fun `should return a view hierarchy as byte array`() {
val viewHierarchy = ViewHierarchyEventProcessor.snapshotViewHierarchyAsData(
fixture.activity,
fixture.mainThreadChecker,
fixture.serializer,
fixture.logger
)
Expand All @@ -100,6 +109,7 @@ class ViewHierarchyEventProcessorTest {
fun `should return null as bytes are empty array`() {
val viewHierarchy = ViewHierarchyEventProcessor.snapshotViewHierarchyAsData(
fixture.activity,
fixture.mainThreadChecker,
fixture.emptySerializer,
fixture.logger
)
Expand Down Expand Up @@ -149,6 +159,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(
Expand Down