Skip to content
Draft
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

### Features

- Add opt-in `strictCallbackMode` to propagate user callback failures as SDK exception or error wrappers. Events containing these failures are silently excluded from capture, including when nested in another exception, to avoid sending data without callback filtering. Configure it through SDK options, `strict-callback-mode` in external configuration, or `io.sentry.strict-callback-mode` in the Android manifest. Disabled by default ([#6173](https://github.com/getsentry/sentry-java/pull/6173))

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.

what does this mean?

Events containing these failures are silently excluded from capture.

It just isn't clear since it seems to contradict the previous sentence. maybe adding "Otherwise..." would help clarify.

- Add `LocalSentrySpan` to `sentry-compose` so apps can provide a parent `ISpan` to a composable subtree and have nested `SentryTraced` spans attach to it ([#6112]https://github.com/getsentry/sentry-java/pull/6112)
- Add `dataCollection`, a fine-grained replacement for `sendDefaultPii`, for controlling data collected automatically by SDK integrations ([#5759](https://github.com/getsentry/sentry-java/pull/5759))
- `sendDefaultPii` remains supported for backwards compatibility. When `dataCollection` is not configured, the SDK preserves the existing `sendDefaultPii` behavior.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -300,6 +300,7 @@ private void startShakeDetection(final @NotNull Activity activity) {
if (dialog != null) {
onDialogGone(dialog);
}
io.sentry.util.ExceptionUtils.maybeRethrow(e);
options
.getLogger()
.log(SentryLevel.ERROR, "Failed to show feedback dialog on shake.", e);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ final class ManifestMetadataReader {

static final String DSN = "io.sentry.dsn";
static final String DEBUG = "io.sentry.debug";
static final String STRICT_CALLBACK_MODE = "io.sentry.strict-callback-mode";
static final String DEBUG_LEVEL = "io.sentry.debug.level";
static final String SAMPLE_RATE = "io.sentry.sample-rate";
static final String ANR_ENABLE = "io.sentry.anr.enable";
Expand Down Expand Up @@ -254,6 +255,8 @@ static void applyMetadata(

if (metadata != null) {
options.setDebug(readBool(metadata, logger, DEBUG, options.isDebug()));
options.setStrictCallbackMode(
readBool(metadata, logger, STRICT_CALLBACK_MODE, options.isStrictCallbackMode()));

if (options.isDebug()) {
final @Nullable String level =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,11 @@ private boolean isMaskingEnabled() {
if (!beforeCaptureCallback.execute(event, hint, shouldDebounce)) {
return event;
}
} catch (Exception e) {
} catch (Exception | Error e) {
io.sentry.util.CallbackUtils.rethrowIfStrictCallbackMode(options, e);
if (e instanceof Error) {

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 always agree with narrowing the number of swallowed errors.

That being said, this would be a breaking change to consumers since we will now throw an exception that we previously did not catch. That's fine if we do this in a major, otherwise if we want to do this in a minor, we should discuss!

Also, if the goal of this is to rethrow unrecoverable errors, we created ExceptionUtils.rethrowIfFatal for that.

throw (Error) e;
}
options
.getLogger()
.log(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,7 @@ public static void init(
try {
configuration.configure(options);
} catch (Throwable t) {
io.sentry.util.CallbackUtils.rethrowIfStrictCallbackMode(options, t);
// let it slip, but log it
options
.getLogger()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import io.sentry.protocol.Feedback;
import io.sentry.protocol.SentryId;
import io.sentry.protocol.User;
import io.sentry.util.CallbackUtils;
import io.sentry.util.ExceptionUtils;
import io.sentry.util.FileUtils;
import io.sentry.util.LoadClass;
Expand Down Expand Up @@ -65,10 +66,14 @@ public class SentryUserFeedbackForm extends AlertDialog {
this.resolvedFeedbackOptions =
new SentryFeedbackOptions(Sentry.getCurrentScopes().getOptions().getFeedbackOptions());
if (configuration != null) {
configuration.configure(context, resolvedFeedbackOptions);
CallbackUtils.run(
Sentry.getCurrentScopes().getOptions(),
() -> configuration.configure(context, resolvedFeedbackOptions));
}
if (configurator != null) {
configurator.configure(resolvedFeedbackOptions);
CallbackUtils.run(
Sentry.getCurrentScopes().getOptions(),
() -> configurator.configure(resolvedFeedbackOptions));
}
SentryIntegrationPackageStorage.getInstance().addIntegration("UserFeedbackWidget");
maybeStartShakeDetection(context);
Expand Down Expand Up @@ -338,37 +343,44 @@ protected void onCreate(Bundle savedInstanceState) {
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)
.show();
final @Nullable SentryFeedbackOptions.SentryFeedbackCallback onSubmitSuccess =
feedbackOptions.getOnSubmitSuccess();
if (onSubmitSuccess != null) {
try {
onSubmitSuccess.call(feedback);
} catch (Exception e) {
Sentry.getCurrentScopes()
.getOptions()
.getLogger()
.log(SentryLevel.ERROR, "onSubmitSuccess callback threw an exception.", e);
try {
if (!id.equals(SentryId.EMPTY_ID)) {
Toast.makeText(
getContext(), feedbackOptions.getSuccessMessageText(), Toast.LENGTH_SHORT)
.show();
final @Nullable SentryFeedbackOptions.SentryFeedbackCallback onSubmitSuccess =
feedbackOptions.getOnSubmitSuccess();
if (onSubmitSuccess != null) {
try {
CallbackUtils.run(
Sentry.getCurrentScopes().getOptions(), () -> onSubmitSuccess.call(feedback));
} catch (Exception e) {
ExceptionUtils.maybeRethrow(e);
Sentry.getCurrentScopes()
.getOptions()
.getLogger()
.log(SentryLevel.ERROR, "onSubmitSuccess callback threw an exception.", e);
}
}
}
} else {
final @Nullable SentryFeedbackOptions.SentryFeedbackCallback onSubmitError =
feedbackOptions.getOnSubmitError();
if (onSubmitError != null) {
try {
onSubmitError.call(feedback);
} catch (Exception e) {
Sentry.getCurrentScopes()
.getOptions()
.getLogger()
.log(SentryLevel.ERROR, "onSubmitError callback threw an exception.", e);
} else {
final @Nullable SentryFeedbackOptions.SentryFeedbackCallback onSubmitError =
feedbackOptions.getOnSubmitError();
if (onSubmitError != null) {
try {
CallbackUtils.run(
Sentry.getCurrentScopes().getOptions(), () -> onSubmitError.call(feedback));
} catch (Exception e) {
ExceptionUtils.maybeRethrow(e);
Sentry.getCurrentScopes()
.getOptions()
.getLogger()
.log(SentryLevel.ERROR, "onSubmitError callback threw an exception.", e);
}
}
}
} finally {
cancel();
}
cancel();
});

btnCancel.setText(feedbackOptions.getCancelButtonLabel());
Expand All @@ -388,13 +400,15 @@ public void setOnDismissListener(final @Nullable OnDismissListener listener) {
// User-provided callback: a crash in it must not take down the app or skip the
// cleanup and the user's own dismiss listener below
try {
onFormClose.run();
CallbackUtils.run(options, onFormClose);
} catch (Exception e) {
ExceptionUtils.maybeRethrow(e);
options
.getLogger()
.log(SentryLevel.ERROR, "onFormClose callback threw an exception.", e);
} finally {
currentReplayId = null;
}
currentReplayId = null;
if (delegate != null) {
delegate.onDismiss(dialog);
}
Expand Down Expand Up @@ -426,8 +440,9 @@ protected void onStart() {
final @Nullable Runnable onFormOpen = feedbackOptions.getOnFormOpen();
if (onFormOpen != null) {
try {
onFormOpen.run();
CallbackUtils.run(options, onFormOpen);
} catch (Exception e) {
ExceptionUtils.maybeRethrow(e);
options.getLogger().log(SentryLevel.ERROR, "onFormOpen callback threw an exception.", e);
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,11 @@ public ViewHierarchyEventProcessor(final @NotNull SentryAndroidOptions options)
if (!beforeCaptureCallback.execute(event, hint, shouldDebounce)) {
return event;
}
} catch (Exception e) {
} catch (Exception | Error e) {
io.sentry.util.CallbackUtils.rethrowIfStrictCallbackMode(options, e);
if (e instanceof Error) {
throw (Error) e;
}
options
.getLogger()
.log(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ public boolean dispatchTouchEvent(final @Nullable MotionEvent motionEvent) {
try {
handleTouchEvent(copy);
} catch (Throwable e) {
io.sentry.util.ExceptionUtils.maybeRethrow(e);

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.

nit: import (here and elsewhere in the PR)

if (options != null) {
options.getLogger().log(SentryLevel.ERROR, "Error dispatching touch event", e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,39 @@ class ManifestMetadataReaderTest {

private val fixture = Fixture()

@Test
fun `strict callback mode defaults to false when metadata is absent`() {
ManifestMetadataReader.applyMetadata(
fixture.getContext(),
fixture.options,
fixture.buildInfoProvider,
)
assertThat(fixture.options.isStrictCallbackMode).isFalse()
}

@Test
fun `strict callback mode preserves programmatic value when metadata is absent`() {
fixture.options.isStrictCallbackMode = true
ManifestMetadataReader.applyMetadata(
fixture.getContext(),
fixture.options,
fixture.buildInfoProvider,
)
assertThat(fixture.options.isStrictCallbackMode).isTrue()
}

@Test
fun `strict callback mode reads true and false from manifest`() {
for (value in listOf(true, false)) {
fixture.options.isStrictCallbackMode = !value
ContextUtils.resetInstance()
val context =
fixture.getContext(bundleOf(ManifestMetadataReader.STRICT_CALLBACK_MODE to value))
ManifestMetadataReader.applyMetadata(context, fixture.options, fixture.buildInfoProvider)
assertThat(fixture.options.isStrictCallbackMode).isEqualTo(value)
}
}

@BeforeTest
fun `set up`() {
ContextUtils.resetInstance()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -314,6 +314,22 @@ class ScreenshotEventProcessorTest {
assertNull(hint.screenshot)
}

@Test
fun `strict capture failures propagate without attaching screenshot`() {
CurrentActivityHolder.getInstance().setActivity(fixture.activity)
fixture.options.isStrictCallbackMode = true
val processor = fixture.getSut(true)
for (failure in listOf(IllegalStateException("private"), LinkageError("private"))) {
fixture.options.setBeforeScreenshotCaptureCallback { _, _, _ -> throw failure }
val event = SentryEvent().apply { exceptions = listOf(SentryException()) }
val hint = Hint()
val thrown = kotlin.test.assertFails { processor.process(event, hint) }
assertThat(io.sentry.util.CallbackUtils.isCallbackException(thrown)).isTrue()
assertThat(thrown.cause).isSameInstanceAs(failure)
assertThat(hint.screenshot).isNull()
}
}

@Test
fun `when capture callback throws, skips screenshot and retains event`() {
CurrentActivityHolder.getInstance().setActivity(fixture.activity)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,82 @@ class SentryUserFeedbackFormTest {
fixture.mockedSentry.close()
}

@Test
fun `strict submit callbacks propagate failures and close the form`() {
fixture.options.isStrictCallbackMode = true
for (success in listOf(false, true)) {
for (failure in listOf(IllegalStateException("private"), LinkageError("private"))) {
fixture.options.feedbackOptions.setOnSubmitSuccess { throw failure }
fixture.options.feedbackOptions.setOnSubmitError { throw failure }
whenever(fixture.mockFeedbackApi.capture(any<Feedback>(), anyOrNull()))
.thenReturn(if (success) SentryId() else SentryId.EMPTY_ID)
val sut = fixture.getSut()
sut.show()
sut
.findViewById<EditText>(R.id.sentry_dialog_user_feedback_edt_description)
.setText("message")
val thrown =
kotlin.test.assertFails {
sut.findViewById<Button>(R.id.sentry_dialog_user_feedback_btn_send).performClick()
}
assertThat(io.sentry.util.CallbackUtils.isCallbackException(thrown)).isTrue()
assertThat(thrown.cause).isSameInstanceAs(failure)
assertThat(sut.isShowing).isFalse()
shadowOf(Looper.getMainLooper()).idle()
}
}
}

@Test
fun `strict feedback configurators propagate marked failures`() {
fixture.options.isStrictCallbackMode = true
for (failure in listOf(IllegalStateException("private"), LinkageError("private"))) {
val configured =
kotlin.test.assertFails { fixture.getSut(configuration = { _, _ -> throw failure }) }
val customized = kotlin.test.assertFails { fixture.getSut(configurator = { throw failure }) }
for (thrown in listOf(configured, customized)) {
com.google.common.truth.Truth.assertThat(
io.sentry.util.CallbackUtils.isCallbackException(thrown)
)
.isTrue()
com.google.common.truth.Truth.assertThat(thrown.cause).isSameInstanceAs(failure)
}
}
}

@Test
fun `strict form open callback propagates marked failures`() {
fixture.options.isStrictCallbackMode = true
for (failure in listOf(IllegalStateException("private"), LinkageError("private"))) {
fixture.options.feedbackOptions.onFormOpen = Runnable { throw failure }
val sut = fixture.getSut()
val thrown = kotlin.test.assertFails { sut.show() }
com.google.common.truth.Truth.assertThat(
io.sentry.util.CallbackUtils.isCallbackException(thrown)
)
.isTrue()
com.google.common.truth.Truth.assertThat(thrown.cause).isSameInstanceAs(failure)
sut.dismiss()
}
}

@Test
fun `strict form close callback propagates marked failures`() {
fixture.options.isStrictCallbackMode = true
for (failure in listOf(IllegalStateException("private"), LinkageError("private"))) {
fixture.options.feedbackOptions.onFormClose = Runnable { throw failure }
val sut = fixture.getSut()
sut.show()
sut.dismiss()
val thrown = kotlin.test.assertFails { shadowOf(Looper.getMainLooper()).idle() }
com.google.common.truth.Truth.assertThat(
io.sentry.util.CallbackUtils.isCallbackException(thrown)
)
.isTrue()
com.google.common.truth.Truth.assertThat(thrown.cause).isSameInstanceAs(failure)
}
}

@Test
fun `feedback dialog is shown when sdk is enabled`() {
fixture.options.isEnabled = true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -344,6 +344,21 @@ class ViewHierarchyEventProcessorTest {
assertNull(hint.viewHierarchy)
}

@Test
fun `strict capture failures propagate without attaching view hierarchy`() {
fixture.options.isStrictCallbackMode = true
val processor = fixture.getSut(true)
for (failure in listOf(IllegalStateException("private"), LinkageError("private"))) {
fixture.options.setBeforeViewHierarchyCaptureCallback { _, _, _ -> throw failure }
val event = SentryEvent().apply { exceptions = listOf(SentryException()) }
val hint = Hint()
val thrown = kotlin.test.assertFails { processor.process(event, hint) }
assertThat(io.sentry.util.CallbackUtils.isCallbackException(thrown)).isTrue()
assertThat(thrown.cause).isSameInstanceAs(failure)
assertThat(hint.viewHierarchy).isNull()
}
}

@Test
fun `when capture callback throws, skips view hierarchy and retains event`() {
fixture.options.isDebug = true
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -464,8 +464,9 @@ public class ReplayIntegration(
hint.set(TypeCheckHint.REPLAY_FRAME_BITMAP, copy)
observer.onMaskedFrameCaptured(hint, frameTimeStamp, screen)
} catch (e: Throwable) {
options.logger.log(ERROR, "Error in ReplayFrameObserver", e)
copy.recycle()
io.sentry.util.CallbackUtils.rethrowIfStrictCallbackMode(options, e)
options.logger.log(ERROR, "Error in ReplayFrameObserver", e)
}
}
}
Expand All @@ -487,8 +488,9 @@ public class ReplayIntegration(
hint.set(TypeCheckHint.REPLAY_FRAME_BITMAP, bitmap)
observer.onMaskedFrameCaptured(hint, frameTimestamp, screen)
} catch (e: Throwable) {
options.logger.log(ERROR, "Error in ReplayFrameObserver", e)
bitmap.recycle()
io.sentry.util.CallbackUtils.rethrowIfStrictCallbackMode(options, e)
options.logger.log(ERROR, "Error in ReplayFrameObserver", e)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ internal class ScreenshotRecorder(
contentChanged.set(false)
screenshotStrategy.capture(root)
} catch (e: Throwable) {
io.sentry.util.ExceptionUtils.maybeRethrow(e)
options.logger.log(WARNING, "Failed to capture replay recording", e)
}
}
Expand Down
Loading
Loading