Merge 4d296ec into 84b57d0

getsentry · Dec 12, 2024 · 8ba5fb7 · 8ba5fb7
2 parents 84b57d0 + 4d296ec
commit 8ba5fb7
Show file tree

Hide file tree

Showing 6 changed files with 85 additions and 5 deletions.
diff --git 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

diff --git 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
@@ -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/...n-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/ReplayTest.kt b/...n-tests/sentry-uitest-android/src/androidTest/java/io/sentry/uitest/android/ReplayTest.kt
@@ -0,0 +1,70 @@
+package io.sentry.uitest.android
+
+import androidx.lifecycle.Lifecycle
+import androidx.test.core.app.launchActivity
+import io.sentry.SentryOptions
+import leakcanary.LeakAssertions
+import leakcanary.LeakCanary
+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<ComposeActivity>()
+        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
@@ -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/...d-replay/src/main/java/io/sentry/android/replay/viewhierarchy/ComposeViewHierarchyNode.kt b/...d-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<LayoutCoordinates>? = 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