From 1d1a928488b1d524ffc4d94741c5ca5d1a87b34e Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Thu, 12 Dec 2024 23:34:32 +0100 Subject: [PATCH 1/9] Fix memory leak in Compose masking for Session Replay --- buildSrc/src/main/java/Config.kt | 1 + .../sentry-uitest-android/build.gradle.kts | 1 + .../io/sentry/uitest/android/ReplayTest.kt | 75 +++++++++++++++++++ .../io/sentry/android/replay/util/Nodes.kt | 5 +- .../viewhierarchy/ComposeViewHierarchyNode.kt | 7 +- 5 files changed, 84 insertions(+), 5 deletions(-) create mode 100644 sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/ReplayTest.kt diff --git a/buildSrc/src/main/java/Config.kt b/buildSrc/src/main/java/Config.kt index 2715dd5767..bf5dba03be 100644 --- a/buildSrc/src/main/java/Config.kt +++ b/buildSrc/src/main/java/Config.kt @@ -194,6 +194,7 @@ object Config { val mockitoKotlin = "org.mockito.kotlin:mockito-kotlin:4.1.0" val mockitoInline = "org.mockito:mockito-inline:4.8.0" val awaitility = "org.awaitility:awaitility-kotlin:4.1.1" + val awaitility3 = "org.awaitility:awaitility-kotlin:3.1.6" // need this due to a conflict of awaitility4+ and espresso on hamcrest val mockWebserver = "com.squareup.okhttp3:mockwebserver:${Libs.okHttpVersion}" val jsonUnit = "net.javacrumbs.json-unit:json-unit:2.32.0" val hsqldb = "org.hsqldb:hsqldb:2.6.1" diff --git a/sentry-android-integration-tests/sentry-uitest-android/build.gradle.kts b/sentry-android-integration-tests/sentry-uitest-android/build.gradle.kts index 7755166a4e..1ae9ccd053 100644 --- a/sentry-android-integration-tests/sentry-uitest-android/build.gradle.kts +++ b/sentry-android-integration-tests/sentry-uitest-android/build.gradle.kts @@ -125,6 +125,7 @@ dependencies { androidTestImplementation(Config.TestLibs.mockWebserver) androidTestImplementation(Config.TestLibs.androidxJunit) androidTestImplementation(Config.TestLibs.leakCanaryInstrumentation) + androidTestImplementation(Config.TestLibs.awaitility3) androidTestUtil(Config.TestLibs.androidxTestOrchestrator) } diff --git a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/ReplayTest.kt b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/ReplayTest.kt new file mode 100644 index 0000000000..4d1ef12a55 --- /dev/null +++ b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/ReplayTest.kt @@ -0,0 +1,75 @@ +package io.sentry.uitest.android + +import android.app.Application +import androidx.lifecycle.Lifecycle +import androidx.test.core.app.launchActivity +import io.sentry.SentryOptions +import kotlinx.coroutines.delay +import kotlinx.coroutines.runBlocking +import leakcanary.AppWatcher +import leakcanary.LeakAssertions +import leakcanary.LeakCanary +import org.awaitility.Duration +import org.awaitility.kotlin.await +import shark.AndroidReferenceMatchers +import shark.IgnoredReferenceMatcher +import shark.ReferencePattern +import java.util.concurrent.atomic.AtomicBoolean +import kotlin.test.Test + +class ReplayTest : BaseUiTest() { + @Test + fun composeReplayDoesNotLeak() { + val sent = AtomicBoolean(false) + + LeakCanary.config = LeakCanary.config.copy( + referenceMatchers = AndroidReferenceMatchers.appDefaults + + listOf( + IgnoredReferenceMatcher( + ReferencePattern.InstanceFieldPattern( + "com.saucelabs.rdcinjector.testfairy.TestFairyEventQueue", + "context" + ) + ), + // Seems like a false-positive returned by LeakCanary when curtains is used in + // the host application (LeakCanary uses it itself internally). We use kind of + // the same approach which possibly clashes with LeakCanary's internal state. + // Only the case when replay is enabled. + // TODO: check if it's actually a leak on our side, or a false-positive and report to LeakCanary's github issue tracker + IgnoredReferenceMatcher( + ReferencePattern.InstanceFieldPattern( + "curtains.internal.RootViewsSpy", + "delegatingViewList" + ) + ) + ) + ('a'..'z').map { char -> + IgnoredReferenceMatcher( + ReferencePattern.StaticFieldPattern( + "com.testfairy.modules.capture.TouchListener", + "$char" + ) + ) + } + ) + + val activityScenario = launchActivity() + activityScenario.moveToState(Lifecycle.State.RESUMED) + + initSentry { + it.experimental.sessionReplay.sessionSampleRate = 1.0 + + it.beforeSendReplay = + SentryOptions.BeforeSendReplayCallback { event, _ -> + sent.set(true) + event + } + } + + // wait until first segment is being sent + await.untilTrue(sent) + + activityScenario.moveToState(Lifecycle.State.DESTROYED) + + LeakAssertions.assertNoLeaks() + } +} diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Nodes.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Nodes.kt index 5608371722..38c2d583b4 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Nodes.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Nodes.kt @@ -8,6 +8,7 @@ import androidx.compose.ui.graphics.Color import androidx.compose.ui.graphics.ColorProducer import androidx.compose.ui.graphics.painter.Painter import androidx.compose.ui.layout.LayoutCoordinates +import androidx.compose.ui.layout.findRootCoordinates import androidx.compose.ui.node.LayoutNode import androidx.compose.ui.text.TextLayoutResult import kotlin.math.roundToInt @@ -165,8 +166,8 @@ private inline fun Float.fastCoerceAtMost(maximumValue: Float): Float { * * @return boundaries of this layout relative to the window's origin. */ -internal fun LayoutCoordinates.boundsInWindow(root: LayoutCoordinates?): Rect { - root ?: return Rect() +internal fun LayoutCoordinates.boundsInWindow(rootCoordinates: LayoutCoordinates?): Rect { + val root = rootCoordinates ?: findRootCoordinates() val rootWidth = root.size.width.toFloat() val rootHeight = root.size.height.toFloat() diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/viewhierarchy/ComposeViewHierarchyNode.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/viewhierarchy/ComposeViewHierarchyNode.kt index 5640fbc96f..652e8cd040 100644 --- a/sentry-android-replay/src/main/java/io/sentry/android/replay/viewhierarchy/ComposeViewHierarchyNode.kt +++ b/sentry-android-replay/src/main/java/io/sentry/android/replay/viewhierarchy/ComposeViewHierarchyNode.kt @@ -27,6 +27,7 @@ import io.sentry.android.replay.util.toOpaque import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode.GenericViewHierarchyNode import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode.ImageViewHierarchyNode import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode.TextViewHierarchyNode +import java.lang.ref.WeakReference @TargetApi(26) internal object ComposeViewHierarchyNode { @@ -62,7 +63,7 @@ internal object ComposeViewHierarchyNode { return options.experimental.sessionReplay.maskViewClasses.contains(className) } - private var _rootCoordinates: LayoutCoordinates? = null + private var _rootCoordinates: WeakReference? = null private fun fromComposeNode( node: LayoutNode, @@ -77,11 +78,11 @@ internal object ComposeViewHierarchyNode { } if (isComposeRoot) { - _rootCoordinates = node.coordinates.findRootCoordinates() + _rootCoordinates = WeakReference(node.coordinates.findRootCoordinates()) } val semantics = node.collapsedSemantics - val visibleRect = node.coordinates.boundsInWindow(_rootCoordinates) + val visibleRect = node.coordinates.boundsInWindow(_rootCoordinates?.get()) val isVisible = !node.outerCoordinator.isTransparent() && (semantics == null || !semantics.contains(SemanticsProperties.InvisibleToUser)) && visibleRect.height() > 0 && visibleRect.width() > 0 From b6bec9c953344c5bc6f5f4dfdb31751ba322a526 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Thu, 12 Dec 2024 23:37:10 +0100 Subject: [PATCH 2/9] Formatting --- .../androidTest/java/io/sentry/uitest/android/ReplayTest.kt | 5 ----- 1 file changed, 5 deletions(-) diff --git a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/ReplayTest.kt b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/ReplayTest.kt index 4d1ef12a55..dbdad3b892 100644 --- a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/ReplayTest.kt +++ b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/ReplayTest.kt @@ -1,15 +1,10 @@ package io.sentry.uitest.android -import android.app.Application import androidx.lifecycle.Lifecycle import androidx.test.core.app.launchActivity import io.sentry.SentryOptions -import kotlinx.coroutines.delay -import kotlinx.coroutines.runBlocking -import leakcanary.AppWatcher import leakcanary.LeakAssertions import leakcanary.LeakCanary -import org.awaitility.Duration import org.awaitility.kotlin.await import shark.AndroidReferenceMatchers import shark.IgnoredReferenceMatcher From 4d296ecc5799dd3c8be8a67d7c6f50181b9b6986 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Thu, 12 Dec 2024 23:39:39 +0100 Subject: [PATCH 3/9] Changelog --- CHANGELOG.md | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e7a4d24918..2e3d7f1242 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,11 @@ # Changelog +## Unreleased + +### Fixes + +- Session Replay: Fix memory leak when masking Compose screens ([#3985](https://github.com/getsentry/sentry-java/pull/3985)) + ## 7.19.0 ### Fixes From 62636bc9b0bc561922b6594fdf4c5d41bb888e78 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Fri, 13 Dec 2024 11:19:31 +0100 Subject: [PATCH 4/9] Have window for AGP matrix in emulators --- .github/workflows/agp-matrix.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/agp-matrix.yml b/.github/workflows/agp-matrix.yml index 964107ecd5..8e0366b147 100644 --- a/.github/workflows/agp-matrix.yml +++ b/.github/workflows/agp-matrix.yml @@ -63,7 +63,7 @@ jobs: with: api-level: 30 force-avd-creation: false - emulator-options: -no-snapshot-save -no-window -gpu swiftshader_indirect -noaudio -no-boot-anim -camera-back none + emulator-options: -no-snapshot-save -noaudio -no-boot-anim -camera-back none disable-animations: true disable-spellchecker: true target: 'aosp_atd' From cd9e6e86ed3a995bd58403bd23e34d1a3fe1f3bb Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Fri, 13 Dec 2024 12:19:10 +0100 Subject: [PATCH 5/9] WIP --- .github/workflows/agp-matrix.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/agp-matrix.yml b/.github/workflows/agp-matrix.yml index 8e0366b147..2239e68a93 100644 --- a/.github/workflows/agp-matrix.yml +++ b/.github/workflows/agp-matrix.yml @@ -63,7 +63,7 @@ jobs: with: api-level: 30 force-avd-creation: false - emulator-options: -no-snapshot-save -noaudio -no-boot-anim -camera-back none + emulator-options: -no-snapshot-save -gpu host -noaudio -no-boot-anim -camera-back none disable-animations: true disable-spellchecker: true target: 'aosp_atd' From 800d56d51d32de58a5f290a1f38d16071e27cb56 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Fri, 13 Dec 2024 12:36:41 +0100 Subject: [PATCH 6/9] add no-window --- .github/workflows/agp-matrix.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/agp-matrix.yml b/.github/workflows/agp-matrix.yml index 2239e68a93..4b8038f433 100644 --- a/.github/workflows/agp-matrix.yml +++ b/.github/workflows/agp-matrix.yml @@ -63,7 +63,7 @@ jobs: with: api-level: 30 force-avd-creation: false - emulator-options: -no-snapshot-save -gpu host -noaudio -no-boot-anim -camera-back none + emulator-options: -no-snapshot-save -no-window -gpu host -noaudio -no-boot-anim -camera-back none disable-animations: true disable-spellchecker: true target: 'aosp_atd' From 9eb818434a9f9516f4de4211cd07d0c74171d372 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Fri, 13 Dec 2024 14:52:51 +0100 Subject: [PATCH 7/9] Fix tests --- .github/workflows/agp-matrix.yml | 4 ++-- .../sentry-uitest-android/proguard-rules.pro | 1 + .../java/io/sentry/uitest/android/ReplayTest.kt | 13 +++++++++++++ 3 files changed, 16 insertions(+), 2 deletions(-) diff --git a/.github/workflows/agp-matrix.yml b/.github/workflows/agp-matrix.yml index 4b8038f433..6d83dbbabf 100644 --- a/.github/workflows/agp-matrix.yml +++ b/.github/workflows/agp-matrix.yml @@ -63,14 +63,14 @@ jobs: with: api-level: 30 force-avd-creation: false - emulator-options: -no-snapshot-save -no-window -gpu host -noaudio -no-boot-anim -camera-back none + emulator-options: -no-snapshot-save -no-window -gpu swiftshader_indirect -noaudio -no-boot-anim -camera-back none disable-animations: true disable-spellchecker: true target: 'aosp_atd' arch: x86 channel: canary # Necessary for ATDs disk-size: 4096M - script: ./gradlew sentry-android-integration-tests:sentry-uitest-android:connectedReleaseAndroidTest -DtestBuildType=release --daemon + script: ./gradlew sentry-android-integration-tests:sentry-uitest-android:connectedReleaseAndroidTest -DtestBuildType=release -Denvironment=github --daemon - name: Upload test results if: always() diff --git a/sentry-android-integration-tests/sentry-uitest-android/proguard-rules.pro b/sentry-android-integration-tests/sentry-uitest-android/proguard-rules.pro index 02f5e80ba3..b0c5d6e11d 100644 --- a/sentry-android-integration-tests/sentry-uitest-android/proguard-rules.pro +++ b/sentry-android-integration-tests/sentry-uitest-android/proguard-rules.pro @@ -40,3 +40,4 @@ -dontwarn org.mockito.internal.** -dontwarn org.jetbrains.annotations.** -dontwarn io.sentry.android.replay.ReplayIntegration +-keep class curtains.** diff --git a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/ReplayTest.kt b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/ReplayTest.kt index dbdad3b892..addaac363f 100644 --- a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/ReplayTest.kt +++ b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/ReplayTest.kt @@ -6,6 +6,9 @@ import io.sentry.SentryOptions import leakcanary.LeakAssertions import leakcanary.LeakCanary import org.awaitility.kotlin.await +import org.hamcrest.CoreMatchers.`is` +import org.junit.Assume.assumeThat +import org.junit.Before import shark.AndroidReferenceMatchers import shark.IgnoredReferenceMatcher import shark.ReferencePattern @@ -13,6 +16,16 @@ import java.util.concurrent.atomic.AtomicBoolean import kotlin.test.Test class ReplayTest : BaseUiTest() { + + @Before + fun setup() { + // we can't run on GH actions emulator, because they don't allow capturing screenshots properly + assumeThat( + System.getProperty("environment") != "github", + `is`(true) + ) + } + @Test fun composeReplayDoesNotLeak() { val sent = AtomicBoolean(false) From 6ede0ae427d5eab09d0bac477fc5ef2010514c56 Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Fri, 13 Dec 2024 16:05:23 +0100 Subject: [PATCH 8/9] Fix --- .../sentry-uitest-android/build.gradle.kts | 1 + .../androidTest/java/io/sentry/uitest/android/ReplayTest.kt | 3 ++- 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/sentry-android-integration-tests/sentry-uitest-android/build.gradle.kts b/sentry-android-integration-tests/sentry-uitest-android/build.gradle.kts index 1ae9ccd053..02609ba787 100644 --- a/sentry-android-integration-tests/sentry-uitest-android/build.gradle.kts +++ b/sentry-android-integration-tests/sentry-uitest-android/build.gradle.kts @@ -26,6 +26,7 @@ android { // This doesn't work on some devices with Android 11+. Clearing package data resets permissions. // Check the readme for more info. testInstrumentationRunnerArguments["clearPackageData"] = "true" + buildConfigField("String", "ENVIRONMENT", "\"${System.getProperty("environment", "")}\"") } testOptions { diff --git a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/ReplayTest.kt b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/ReplayTest.kt index addaac363f..ffea8f2e04 100644 --- a/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/ReplayTest.kt +++ b/sentry-android-integration-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/ReplayTest.kt @@ -20,8 +20,9 @@ class ReplayTest : BaseUiTest() { @Before fun setup() { // we can't run on GH actions emulator, because they don't allow capturing screenshots properly + @Suppress("KotlinConstantConditions") assumeThat( - System.getProperty("environment") != "github", + BuildConfig.ENVIRONMENT != "github", `is`(true) ) } From 13bfd36adb929b686972cde6e28a8a5f9a9082bc Mon Sep 17 00:00:00 2001 From: Roman Zavarnitsyn Date: Fri, 13 Dec 2024 16:25:32 +0100 Subject: [PATCH 9/9] update proguard rules --- .../sentry-uitest-android/proguard-rules.pro | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/sentry-android-integration-tests/sentry-uitest-android/proguard-rules.pro b/sentry-android-integration-tests/sentry-uitest-android/proguard-rules.pro index b0c5d6e11d..5de2dac4bd 100644 --- a/sentry-android-integration-tests/sentry-uitest-android/proguard-rules.pro +++ b/sentry-android-integration-tests/sentry-uitest-android/proguard-rules.pro @@ -40,4 +40,4 @@ -dontwarn org.mockito.internal.** -dontwarn org.jetbrains.annotations.** -dontwarn io.sentry.android.replay.ReplayIntegration --keep class curtains.** +-keep class curtains.** { *; }