Skip to content

Commit

Permalink
[SR] Fix memory leak in Compose masking (#3985)
Browse files Browse the repository at this point in the history
* Fix memory leak in Compose masking for Session Replay

* Formatting

* Changelog

* Have window for AGP matrix in emulators

* WIP

* add no-window

* Fix tests

* Fix

* update proguard rules
  • Loading branch information
romtsn authored Dec 16, 2024
1 parent a4f2eef commit 00f91f4
Show file tree
Hide file tree
Showing 8 changed files with 97 additions and 6 deletions.
2 changes: 1 addition & 1 deletion .github/workflows/agp-matrix.yml
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ jobs:
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()
Expand Down
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
### Fixes

- Change TTFD timeout to 25 seconds ([#3984](https://github.com/getsentry/sentry-java/pull/3984))
- Session Replay: Fix memory leak when masking Compose screens ([#3985](https://github.com/getsentry/sentry-java/pull/3985))

## 7.19.0

Expand Down
1 change: 1 addition & 0 deletions buildSrc/src/main/java/Config.kt
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -125,6 +126,7 @@ dependencies {
androidTestImplementation(Config.TestLibs.mockWebserver)
androidTestImplementation(Config.TestLibs.androidxJunit)
androidTestImplementation(Config.TestLibs.leakCanaryInstrumentation)
androidTestImplementation(Config.TestLibs.awaitility3)
androidTestUtil(Config.TestLibs.androidxTestOrchestrator)
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,4 @@
-dontwarn org.mockito.internal.**
-dontwarn org.jetbrains.annotations.**
-dontwarn io.sentry.android.replay.ReplayIntegration
-keep class curtains.** { *; }
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
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 org.hamcrest.CoreMatchers.`is`
import org.junit.Assume.assumeThat
import org.junit.Before
import shark.AndroidReferenceMatchers
import shark.IgnoredReferenceMatcher
import shark.ReferencePattern
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
@Suppress("KotlinConstantConditions")
assumeThat(
BuildConfig.ENVIRONMENT != "github",
`is`(true)
)
}

@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()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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,
Expand All @@ -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
Expand Down

0 comments on commit 00f91f4

Please sign in to comment.