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

Fix UncaughtExceptionHandlerIntegration Memory Leak #3398

Merged
merged 8 commits into from
Jun 25, 2024
Merged
Show file tree
Hide file tree
Changes from 6 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 @@ -10,6 +10,7 @@

- Fix Frame measurements in app start transactions ([#3382](https://github.com/getsentry/sentry-java/pull/3382))
- Fix timing metric value different from span duration ([#3368](https://github.com/getsentry/sentry-java/pull/3368))
- Fix UncaughtExceptionHandlerIntegration Memory Leak ([#3398](https://github.com/getsentry/sentry-java/pull/3398))

## 7.8.0

Expand Down
2 changes: 1 addition & 1 deletion sentry/api/sentry.api
Original file line number Diff line number Diff line change
Expand Up @@ -2978,7 +2978,7 @@ public final class io/sentry/TypeCheckHint {
public final class io/sentry/UncaughtExceptionHandlerIntegration : io/sentry/Integration, java/io/Closeable, java/lang/Thread$UncaughtExceptionHandler {
public fun <init> ()V
public fun close ()V
public final fun register (Lio/sentry/IHub;Lio/sentry/SentryOptions;)V
public fun register (Lio/sentry/IHub;Lio/sentry/SentryOptions;)V
public fun uncaughtException (Ljava/lang/Thread;Ljava/lang/Throwable;)V
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public UncaughtExceptionHandlerIntegration() {
}

@Override
public final void register(final @NotNull IHub hub, final @NotNull SentryOptions options) {
public void register(final @NotNull IHub hub, final @NotNull SentryOptions options) {
if (registered) {
options
.getLogger()
Expand Down Expand Up @@ -75,7 +75,16 @@ public final void register(final @NotNull IHub hub, final @NotNull SentryOptions
"default UncaughtExceptionHandler class='"
+ currentHandler.getClass().getName()
+ "'");
defaultExceptionHandler = currentHandler;

if (currentHandler instanceof UncaughtExceptionHandlerIntegration) {
final UncaughtExceptionHandlerIntegration currentHandlerIntegration =
(UncaughtExceptionHandlerIntegration) currentHandler;
defaultExceptionHandler = currentHandlerIntegration.defaultExceptionHandler;
} else {
defaultExceptionHandler = currentHandler;
}

// defaultExceptionHandler = currentHandler;
stefanosiano marked this conversation as resolved.
Show resolved Hide resolved
}

threadAdapter.setDefaultUncaughtExceptionHandler(this);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import io.sentry.hints.EventDropReason.MULTITHREADED_DEDUPLICATION
import io.sentry.protocol.SentryId
import io.sentry.util.HintUtils
import org.mockito.kotlin.any
import org.mockito.kotlin.anyOrNull
import org.mockito.kotlin.argThat
import org.mockito.kotlin.argWhere
import org.mockito.kotlin.argumentCaptor
Expand All @@ -19,6 +20,7 @@ import java.io.PrintStream
import java.nio.file.Files
import kotlin.concurrent.thread
import kotlin.test.Test
import kotlin.test.assertEquals
import kotlin.test.assertNotNull
import kotlin.test.assertTrue

Expand Down Expand Up @@ -291,4 +293,54 @@ class UncaughtExceptionHandlerIntegrationTest {
}
)
}

@Test
fun `multiple registrations do not cause the build-up of a tree of UncaughtExceptionHandlerIntegrations`() {
var currentDefaultHandler: Thread.UncaughtExceptionHandler? = null

val handler = mock<UncaughtExceptionHandler>()
whenever(handler.defaultUncaughtExceptionHandler).thenAnswer { currentDefaultHandler }

whenever(handler.setDefaultUncaughtExceptionHandler(anyOrNull<Thread.UncaughtExceptionHandler>())).then {
currentDefaultHandler = it.getArgument(0)
null
}

val integration1 = UncaughtExceptionHandlerIntegration(handler)
integration1.register(fixture.hub, fixture.options)

val integration2 = UncaughtExceptionHandlerIntegration(handler)
integration2.register(fixture.hub, fixture.options)

assertEquals(currentDefaultHandler, integration2)
integration2.close()

assertEquals(null, currentDefaultHandler)
}

@Test
fun `multiple registrations do not cause the build-up of a tree of UncaughtExceptionHandlerIntegrations, reset to inital`() {
val initialUncaughtExceptionHandler = Thread.UncaughtExceptionHandler { _, _ -> }

var currentDefaultHandler: Thread.UncaughtExceptionHandler? = initialUncaughtExceptionHandler

val handler = mock<UncaughtExceptionHandler>()
whenever(handler.defaultUncaughtExceptionHandler).thenAnswer { currentDefaultHandler }

whenever(handler.setDefaultUncaughtExceptionHandler(anyOrNull<Thread.UncaughtExceptionHandler>())).then {
currentDefaultHandler = it.getArgument(0)
null
}

val integration1 = UncaughtExceptionHandlerIntegration(handler)
integration1.register(fixture.hub, fixture.options)

val integration2 = UncaughtExceptionHandlerIntegration(handler)
integration2.register(fixture.hub, fixture.options)

assertEquals(currentDefaultHandler, integration2)
integration2.close()

assertEquals(initialUncaughtExceptionHandler, currentDefaultHandler)
}
}
Loading