Skip to content
Open
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
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,9 @@
### Features

- Make `ISpan.startChild` overloads with `SpanOptions` public ([#5927](https://github.com/getsentry/sentry-java/pull/5927))
- Add screenshot attachment button to the Android user feedback widget ([#5828](https://github.com/getsentry/sentry-java/pull/5828))
- Users can now attach a screenshot when submitting feedback. Enabled by default; can be disabled via `SentryFeedbackOptions.setEnableAttachScreenshot(false)` or the `io.sentry.feedback.enable-attach-screenshot` manifest flag.
- Requires the `androidx.activity` `>=1.8.2` dependency

### Fixes

Expand Down
1 change: 1 addition & 0 deletions gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@ apollo3-kotlin = { module = "com.apollographql.apollo3:apollo-runtime", version
apollo4-kotlin = { module = "com.apollographql.apollo:apollo-runtime", version = "4.1.1" }
androidx-appcompat = { module = "androidx.appcompat:appcompat", version = "1.3.0" }
androidx-annotation = { module = "androidx.annotation:annotation", version = "1.9.1" }
androidx-activity = { module = "androidx.activity:activity", version = "1.8.2" }
androidx-activity-compose = { module = "androidx.activity:activity-compose", version = "1.8.2" }
androidx-compose-foundation = { module = "androidx.compose.foundation:foundation", version.ref = "androidxCompose" }
androidx-compose-foundation-layout = { module = "androidx.compose.foundation:foundation-layout", version.ref = "androidxCompose" }
Expand Down
1 change: 1 addition & 0 deletions sentry-android-core/api/sentry-android-core.api
Original file line number Diff line number Diff line change
Expand Up @@ -574,6 +574,7 @@ public abstract interface class io/sentry/android/core/SentryUserFeedbackDialog$
public class io/sentry/android/core/SentryUserFeedbackForm : android/app/AlertDialog {
protected fun onCreate (Landroid/os/Bundle;)V
protected fun onStart ()V
protected fun onStop ()V
public fun setCancelable (Z)V
public fun setOnDismissListener (Landroid/content/DialogInterface$OnDismissListener;)V
public fun show ()V
Expand Down
3 changes: 3 additions & 0 deletions sentry-android-core/build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,9 @@ dependencies {
implementation(libs.androidx.lifecycle.common.java8)
implementation(libs.androidx.lifecycle.process)
implementation(libs.androidx.core)
// photo picker for user feedback screenshot attachments
compileOnly(libs.androidx.activity)

implementation(libs.epitaph)

errorprone(libs.errorprone.core)
Expand Down
5 changes: 5 additions & 0 deletions sentry-android-core/proguard-rules.pro
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,11 @@
-dontwarn io.sentry.compose.gestures.ComposeGestureTargetLocator
-dontwarn io.sentry.compose.viewhierarchy.ComposeViewHierarchyExporter

# androidx.activity is a compileOnly dependency, used by the user feedback screenshot picker
# its presence is checked at runtime before use
-dontwarn androidx.activity.ComponentActivity
-dontwarn androidx.activity.result.**

# To ensure that stack traces is unambiguous
# https://developer.android.com/studio/build/shrink-code#decode-stack-trace
-keepattributes LineNumberTable,SourceFile
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -187,6 +187,9 @@ final class ManifestMetadataReader {

static final String FEEDBACK_USE_SHAKE_GESTURE = "io.sentry.feedback.use-shake-gesture";

static final String FEEDBACK_ENABLE_ATTACH_SCREENSHOT =
"io.sentry.feedback.enable-attach-screenshot";

static final String SPOTLIGHT_ENABLE = "io.sentry.spotlight.enable";

static final String SPOTLIGHT_CONNECTION_URL = "io.sentry.spotlight.url";
Expand Down Expand Up @@ -728,6 +731,12 @@ static void applyMetadata(
feedbackOptions.setUseShakeGesture(
readBool(
metadata, logger, FEEDBACK_USE_SHAKE_GESTURE, feedbackOptions.isUseShakeGesture()));
feedbackOptions.setEnableAttachScreenshot(
readBool(
metadata,
logger,
FEEDBACK_ENABLE_ATTACH_SCREENSHOT,
feedbackOptions.isEnableAttachScreenshot()));

options.setStrictTraceContinuation(
readBool(
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,61 @@
package io.sentry.android.core;

import android.app.Activity;
import android.net.Uri;
import androidx.activity.ComponentActivity;
import androidx.activity.result.ActivityResultLauncher;
import androidx.activity.result.PickVisualMediaRequest;
import androidx.activity.result.contract.ActivityResultContracts;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;

/**
* Launches the androidx photo picker to attach an screenshot to user feedback. All
* androidx.activity references are isolated in this class, so it must only be loaded once {@link
* SentryUserFeedbackForm#isScreenshotPickerAvailable} has returned true. That check deliberately
* lives outside of this class, so that it can run without linking any androidx.activity type.
*/
final class SentryFeedbackScreenshotPicker {

interface OnScreenshotPicked {
void onScreenshotPicked(@NotNull Uri uri);
}

private final @NotNull ActivityResultLauncher<PickVisualMediaRequest> launcher;

private SentryFeedbackScreenshotPicker(
final @NotNull ActivityResultLauncher<PickVisualMediaRequest> launcher) {
this.launcher = launcher;
}

static @Nullable SentryFeedbackScreenshotPicker register(
final @NotNull Activity activity,
final @NotNull SentryFeedbackScreenshotPicker.OnScreenshotPicked callback) {
if (!(activity instanceof ComponentActivity)) {
return null;
}
final @NotNull ActivityResultLauncher<PickVisualMediaRequest> launcher =
((ComponentActivity) activity)
.getActivityResultRegistry()
.register(
"sentry_user_feedback_screenshot_picker",
new ActivityResultContracts.PickVisualMedia(),
(@Nullable Uri uri) -> {
if (uri != null) {
callback.onScreenshotPicked(uri);
}
});
return new SentryFeedbackScreenshotPicker(launcher);
Comment on lines +38 to +48

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Bug: The ActivityResultRegistry.register() call for the screenshot picker is made after the Activity's lifecycle is STARTED, which will throw an unhandled IllegalStateException and crash the app.
Severity: CRITICAL

Suggested Fix

The registration of the ActivityResultLauncher should be moved to a point in the lifecycle before the host Activity is started, such as in the onCreate() method of the Activity or Fragment. If registration must happen dynamically, wrap the call in a try-catch block to handle the IllegalStateException gracefully, although this would prevent the feature from working. The recommended approach is to register early in the lifecycle.

Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent. Verify if this is a real issue. If it is, propose a fix; if not, explain why it's
not valid.

Location:
sentry-android-core/src/main/java/io/sentry/android/core/SentryFeedbackScreenshotPicker.java#L37-L48

Potential issue: The `SentryUserFeedbackForm` dialog attempts to register an
`ActivityResultLauncher` for the screenshot picker within its `onStart()` method. This
registration is performed on the host `ComponentActivity`, which is already in the
`STARTED` or `RESUMED` state. According to Android's `ActivityResultRegistry`
documentation, `register()` must be called before the `LifecycleOwner` reaches the
`STARTED` state. Calling it after this point throws an `IllegalStateException`. Since
the call to `SentryFeedbackScreenshotPicker.register()` is not wrapped in a `try-catch`
block, this exception is unhandled and will crash the application when the feedback form
is displayed.

Also affects:

  • sentry-android-core/src/main/java/io/sentry/android/core/SentryUserFeedbackForm.java:385~385

}

void launch() {
launcher.launch(
new PickVisualMediaRequest.Builder()
.setMediaType(ActivityResultContracts.PickVisualMedia.ImageOnly.INSTANCE)
.build());
}

void unregister() {
launcher.unregister();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,17 +3,24 @@
import android.app.Activity;
import android.app.AlertDialog;
import android.app.Application;
import android.content.ContentResolver;
import android.content.Context;
import android.content.ContextWrapper;
import android.database.Cursor;
import android.net.Uri;
import android.os.Bundle;
import android.provider.OpenableColumns;
import android.view.View;
import android.view.Window;
import android.view.WindowManager;
import android.webkit.MimeTypeMap;
import android.widget.Button;
import android.widget.EditText;
import android.widget.ImageView;
import android.widget.TextView;
import android.widget.Toast;
import io.sentry.Attachment;
import io.sentry.Hint;
import io.sentry.IScopes;
import io.sentry.Sentry;
import io.sentry.SentryFeedbackOptions;
Expand All @@ -23,6 +30,11 @@
import io.sentry.protocol.Feedback;
import io.sentry.protocol.SentryId;
import io.sentry.protocol.User;
import io.sentry.util.ExceptionUtils;
import io.sentry.util.FileUtils;
import io.sentry.util.LoadClass;
import java.io.IOException;
import java.io.InputStream;
import java.lang.ref.WeakReference;
import org.jetbrains.annotations.NotNull;
import org.jetbrains.annotations.Nullable;
Expand All @@ -39,6 +51,9 @@ public class SentryUserFeedbackForm extends AlertDialog {
private @Nullable SentryShakeDetector shakeDetector;
private @Nullable Application.ActivityLifecycleCallbacks shakeLifecycleCallbacks;

private @Nullable SentryFeedbackScreenshotPicker screenshotPicker;
private @Nullable Uri selectedImageUri;

SentryUserFeedbackForm(
final @NotNull Context context,
final int themeResId,
Expand Down Expand Up @@ -191,6 +206,31 @@ protected void onCreate(Bundle savedInstanceState) {
findViewById(R.id.sentry_dialog_user_feedback_edt_description);
final @NotNull Button btnSend = findViewById(R.id.sentry_dialog_user_feedback_btn_send);
final @NotNull Button btnCancel = findViewById(R.id.sentry_dialog_user_feedback_btn_cancel);
final @NotNull Button btnAddScreenshot =
findViewById(R.id.sentry_dialog_user_feedback_btn_add_screenshot);

// The button is made visible in onStart, once the screenshot picker is registered successfully
btnAddScreenshot.setVisibility(View.GONE);
btnAddScreenshot.setOnClickListener(
v -> {
if (selectedImageUri == null) {
if (screenshotPicker != null) {
try {
screenshotPicker.launch();
} catch (Throwable t) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

gonna make the same comment that I did for the other usages of Throwable

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Switched to using the newly added ExceptionUtils.rethrowIfFatal, so OOM and linkage errors propagate. getUriSize now logs the throwable too, it was silent before.

ExceptionUtils.rethrowIfFatal(t);
// e.g. no photo picker on the device, or the host activity is already gone
Sentry.getCurrentScopes()
.getOptions()
.getLogger()
.log(SentryLevel.ERROR, "Failed to launch the screenshot picker.", t);
}
}
} else {
selectedImageUri = null;
btnAddScreenshot.setText(feedbackOptions.getAddScreenshotButtonLabel());
}
});

if (feedbackOptions.isShowBranding()) {
imgLogo.setVisibility(View.VISIBLE);
Expand Down Expand Up @@ -276,7 +316,9 @@ protected void onCreate(Bundle savedInstanceState) {
}

// Capture the feedback. If the ID is empty, it means that the feedback was not sent
final @NotNull SentryId id = Sentry.feedback().capture(feedback);
final @NotNull Hint hint = new Hint();
maybeAddImageAttachment(hint);
final @NotNull SentryId id = Sentry.feedback().capture(feedback, hint);
if (!id.equals(SentryId.EMPTY_ID)) {
Toast.makeText(
getContext(), feedbackOptions.getSuccessMessageText(), Toast.LENGTH_SHORT)
Expand Down Expand Up @@ -331,6 +373,7 @@ protected void onStart() {
edtMessage.setError(null);

final @NotNull SentryOptions options = Sentry.getCurrentScopes().getOptions();
maybeRegisterScreenshotPicker(options);
final @NotNull SentryFeedbackOptions feedbackOptions = options.getFeedbackOptions();
final @Nullable Runnable onFormOpen = feedbackOptions.getOnFormOpen();
if (onFormOpen != null) {
Expand All @@ -340,6 +383,146 @@ protected void onStart() {
currentReplayId = options.getReplayController().getReplayId();
}

@Override
protected void onStop() {
super.onStop();
if (screenshotPicker != null) {
screenshotPicker.unregister();
screenshotPicker = null;
}
}

private void maybeRegisterScreenshotPicker(final @NotNull SentryOptions options) {
// Clear any previously selected image so subsequent show() calls start with a fresh form
final @NotNull Button btnAddScreenshot =
findViewById(R.id.sentry_dialog_user_feedback_btn_add_screenshot);
selectedImageUri = null;
btnAddScreenshot.setText(resolvedFeedbackOptions.getAddScreenshotButtonLabel());

if (!resolvedFeedbackOptions.isEnableAttachScreenshot()) {
return;
}
final @Nullable Activity activity = getActivity(getContext());
if (activity != null && isScreenshotPickerAvailable(options)) {
screenshotPicker =
SentryFeedbackScreenshotPicker.register(
activity, uri -> onScreenshotPicked(options, btnAddScreenshot, uri));
}
Comment thread
cursor[bot] marked this conversation as resolved.
if (screenshotPicker != null) {
btnAddScreenshot.setVisibility(View.VISIBLE);
} else {
btnAddScreenshot.setVisibility(View.GONE);
options
.getLogger()
.log(
SentryLevel.WARNING,
"Feedback screenshot button won't be shown. It requires the androidx.activity "
+ "dependency and the feedback form being shown from a ComponentActivity.");
Comment thread
sentry[bot] marked this conversation as resolved.
}
}

/**
* Must be called before {@link SentryFeedbackScreenshotPicker} is touched for the first time, as
* that class links against androidx.activity types.
*/
private boolean isScreenshotPickerAvailable(final @NotNull SentryOptions options) {
final @NotNull LoadClass loadClass = resolvedFeedbackOptions.getLoadClass();
return loadClass.isClassAvailable("androidx.activity.ComponentActivity", options)
&& loadClass.isClassAvailable(
"androidx.activity.result.contract.ActivityResultContracts$PickVisualMedia", options);
}

private void onScreenshotPicked(
final @NotNull SentryOptions options,
final @NotNull Button btnAddScreenshot,
final @NotNull Uri uri) {
final long size = getUriSize(options, getContext().getContentResolver(), uri);
if (size > options.getMaxAttachmentSize()) {
Comment thread
sentry[bot] marked this conversation as resolved.
options
.getLogger()
.log(
SentryLevel.WARNING,
"Selected screenshot is larger than the maxAttachmentSize of %d bytes, dropping it.",
options.getMaxAttachmentSize());
Toast.makeText(
getContext(),
resolvedFeedbackOptions.getScreenshotTooLargeMessageText(),
Toast.LENGTH_SHORT)
.show();
return;
}
selectedImageUri = uri;
btnAddScreenshot.setText(resolvedFeedbackOptions.getRemoveScreenshotButtonLabel());
}

private void maybeAddImageAttachment(final @NotNull Hint hint) {
final @Nullable Uri imageUri = selectedImageUri;
if (imageUri == null) {
return;
}
try {
final @NotNull ContentResolver resolver = getContext().getContentResolver();
final @Nullable String resolvedMime = resolver.getType(imageUri);
final @NotNull String mime = resolvedMime != null ? resolvedMime : "image/png";
final @Nullable String resolvedExt =
MimeTypeMap.getSingleton().getExtensionFromMimeType(mime);
final @NotNull String ext = resolvedExt != null ? resolvedExt : "png";
hint.addAttachment(
new Attachment(
() ->
readUriBytes(
resolver,
imageUri,
Sentry.getCurrentScopes().getOptions().getMaxAttachmentSize()),
"screenshot." + ext,
mime,
"event.attachment",
false));
Comment thread
markushi marked this conversation as resolved.
} catch (Throwable t) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I decided I'm always going to comment whenever we add new usages of this. Sorry to bring it up again.

  1. How will we know that this feature is working in production if we swallow all errors?
  2. Since we're dealing with images, files and attachments how we we know we aren't swallowing a permissions issue or OOM?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Switched to using the newly added ExceptionUtils.rethrowIfFatal, so OOM and linkage errors propagate. getUriSize now logs the throwable too, it was silent before.

ExceptionUtils.rethrowIfFatal(t);
// e.g. the read permission for the picked Uri was revoked in the meantime
Sentry.getCurrentScopes()
.getOptions()
.getLogger()
.log(SentryLevel.ERROR, "Failed to attach image to feedback.", t);
}
}

private static long getUriSize(
final @NotNull SentryOptions options,
final @NotNull ContentResolver resolver,
final @NotNull Uri uri) {
try (final @Nullable Cursor cursor = resolver.query(uri, null, null, null, null)) {
if (cursor != null && cursor.moveToFirst()) {
final int sizeIndex = cursor.getColumnIndex(OpenableColumns.SIZE);
if (sizeIndex != -1 && !cursor.isNull(sizeIndex)) {
return cursor.getLong(sizeIndex);
}
}
} catch (Throwable t) {
ExceptionUtils.rethrowIfFatal(t);
options
.getLogger()
.log(
SentryLevel.WARNING,
"Unable to determine the size of the selected screenshot, the attachment size limit "
+ "is applied when the feedback is captured.",
t);
}
return -1;
}

private static byte[] readUriBytes(
final @NotNull ContentResolver resolver, final @NotNull Uri uri, final long maxSize)
throws IOException {
try (final @Nullable InputStream inputStream = resolver.openInputStream(uri)) {
if (inputStream == null) {
throw new IOException("Unable to open image attachment: " + uri);
}
return FileUtils.inputStreamToByteArray(inputStream, maxSize);
}
}

@Override
public void show() {
// If Sentry is disabled, don't show the dialog, but log a warning
Expand Down
Loading
Loading