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
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import io.sentry.NoOpSocketTagger;
import io.sentry.NoOpTransactionProfiler;
import io.sentry.NoopVersionDetector;
import io.sentry.ReplayBreadcrumbConverter;
import io.sentry.ScopeType;
import io.sentry.SendFireAndForgetEnvelopeSender;
import io.sentry.SendFireAndForgetOutboxSender;
Expand Down Expand Up @@ -247,6 +248,14 @@ static void initializeIntegrationsAndProcessors(
options.setCompositePerformanceCollector(new DefaultCompositePerformanceCollector(options));
}

if (options.getReplayController() != null) { // TODO: Triple-check this should be a null check
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@roman can you be my triple-check? the code above uses instanceof , but afaict the NoOpReplayController always gets overridden so i put a null check here

ReplayBreadcrumbConverter replayBreadcrumbConverter = options.getReplayController().getBreadcrumbConverter();
replayBreadcrumbConverter.setUserBeforeBreadcrumbCallback(options.getBeforeBreadcrumb());
options.setBeforeBreadcrumb(
replayBreadcrumbConverter
);
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Null Handling Error in Breadcrumb Conversion

A NullPointerException can occur if options.getReplayController().getBreadcrumbConverter() returns null, as the code attempts to call setUserBeforeBreadcrumbCallback on the resulting replayBreadcrumbConverter without a null check.

Fix in Cursor Fix in Web


// Check if the profiler was already instantiated in the app start.
// We use the Android profiler, that uses a global start/stop api, so we need to preserve the
// state of the profiler, and it's only possible retaining the instance.
Expand Down
2 changes: 2 additions & 0 deletions sentry-android-replay/api/sentry-android-replay.api
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,8 @@ public class io/sentry/android/replay/DefaultReplayBreadcrumbConverter : io/sent
public static final field $stable I
public fun <init> ()V
public fun convert (Lio/sentry/Breadcrumb;)Lio/sentry/rrweb/RRWebEvent;
public fun execute (Lio/sentry/Breadcrumb;Lio/sentry/Hint;)Lio/sentry/Breadcrumb;
public fun setUserBeforeBreadcrumbCallback (Lio/sentry/SentryOptions$BeforeBreadcrumbCallback;)V
}

public final class io/sentry/android/replay/GeneratedVideo {
Expand Down
Original file line number Diff line number Diff line change
@@ -1,16 +1,22 @@
package io.sentry.android.replay

import android.util.Log
import io.sentry.Breadcrumb
import io.sentry.Hint
import io.sentry.ReplayBreadcrumbConverter
import io.sentry.SentryLevel
import io.sentry.SentryOptions.BeforeBreadcrumbCallback
import io.sentry.SpanDataConvention
import io.sentry.rrweb.RRWebBreadcrumbEvent
import io.sentry.rrweb.RRWebEvent
import io.sentry.rrweb.RRWebSpanEvent
import io.sentry.util.network.NetworkRequestData
import java.util.Collections
import kotlin.LazyThreadSafetyMode.NONE

public open class DefaultReplayBreadcrumbConverter : ReplayBreadcrumbConverter {
internal companion object {
private const val MAX_HTTP_NETWORK_DETAILS = 32
private val snakecasePattern by lazy(NONE) { "_[a-z]".toRegex() }
private val supportedNetworkData =
HashSet<String>().apply {
Expand All @@ -24,10 +30,21 @@ public open class DefaultReplayBreadcrumbConverter : ReplayBreadcrumbConverter {
}

private var lastConnectivityState: String? = null
private val httpNetworkDetails =
Collections.synchronizedMap(
object : LinkedHashMap<Breadcrumb, NetworkRequestData>() {
override fun removeEldestEntry(
eldest: MutableMap.MutableEntry<Breadcrumb, NetworkRequestData>?
): Boolean {
return size > MAX_HTTP_NETWORK_DETAILS
}
}
)
Comment on lines +33 to +42

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: Breadcrumb.equals() omits data map, causing HashMap key collisions for concurrent requests, leading to incorrect network data attribution.
Severity: CRITICAL | Confidence: 1.00

🔍 Detailed Analysis

The Breadcrumb.equals() method omits the data map from its comparison. This allows distinct Breadcrumb objects, particularly those generated by HTTP requests occurring within the same millisecond but having different URLs or other data map contents, to be considered equal. When these Breadcrumb objects are used as keys in the httpNetworkDetails LinkedHashMap, one entry can unintentionally overwrite another, causing network details from one request to be incorrectly associated with a different request during session replay processing.

💡 Suggested Fix

Modify Breadcrumb.equals() to include the data map in its comparison, or use a different key for httpNetworkDetails that uniquely identifies each network request, such as a UUID or a composite key including relevant data fields.

🤖 Prompt for AI Agent
Fix this bug. In
sentry-android-replay/src/main/java/io/sentry/android/replay/DefaultReplayBreadcrumbConverter.kt
at lines 33-42: The `Breadcrumb.equals()` method omits the `data` map from its
comparison. This allows distinct `Breadcrumb` objects, particularly those generated by
HTTP requests occurring within the same millisecond but having different URLs or other
`data` map contents, to be considered equal. When these `Breadcrumb` objects are used as
keys in the `httpNetworkDetails` `LinkedHashMap`, one entry can unintentionally
overwrite another, causing network details from one request to be incorrectly associated
with a different request during session replay processing.

Did we get this right? 👍 / 👎 to inform future reviews.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@romtsn is there any issue with updating Breadcrumb#equals and Breadcrumb#hashmap to include data in the comparison?

I left it out in my commit cuz assumed we want Breadcrumbs to be equal if they have different data (e.g. it's the same breadcrumb if some more data gets added by a different caller at some point later)

private var userBeforeBreadcrumbCallback: BeforeBreadcrumbCallback? = null

override fun convert(breadcrumb: Breadcrumb): RRWebEvent? {
var breadcrumbMessage: String? = null
var breadcrumbCategory: String? = null
val breadcrumbCategory: String?
var breadcrumbLevel: SentryLevel? = null
val breadcrumbData = mutableMapOf<String, Any?>()
when {
Expand Down Expand Up @@ -120,10 +137,76 @@ public open class DefaultReplayBreadcrumbConverter : ReplayBreadcrumbConverter {
}
}

private fun Breadcrumb.isValidForRRWebSpan(): Boolean =
!(data["url"] as? String).isNullOrEmpty() &&
SpanDataConvention.HTTP_START_TIMESTAMP in data &&
SpanDataConvention.HTTP_END_TIMESTAMP in data
override fun setUserBeforeBreadcrumbCallback(beforeBreadcrumbCallback: BeforeBreadcrumbCallback?) {
this.userBeforeBreadcrumbCallback = beforeBreadcrumbCallback
}

/** Delegate to user-provided callback (if exists) to provide the final breadcrumb to process. */
override fun execute(breadcrumb: Breadcrumb, hint: Hint): Breadcrumb? {
val callback = userBeforeBreadcrumbCallback
val result =
if (callback != null) {
callback.execute(breadcrumb, hint)
} else {
breadcrumb
}

result?.let { finalBreadcrumb ->
extractNetworkRequestDataFromHint(finalBreadcrumb, hint)?.let { networkData ->
httpNetworkDetails[finalBreadcrumb] = networkData
}
}

Log.d(
"SentryNetwork",
"SentryNetwork: BeforeBreadcrumbCallback - Hint: $hint, Breadcrumb: $result",
)
return result
}

private fun extractNetworkRequestDataFromHint(
breadcrumb: Breadcrumb,
breadcrumbHint: Hint,
): NetworkRequestData? {
if (breadcrumb.type != "http" && breadcrumb.category != "http") {
return null
}

val networkDetails = breadcrumbHint.get("replay:networkDetails") as? NetworkRequestData
if (networkDetails != null) {
Log.d(
"SentryNetwork",
"SentryNetwork: Found structured NetworkRequestData in hint: $networkDetails",
)
return networkDetails
}

Log.d("SentryNetwork", "SentryNetwork: No structured NetworkRequestData found on hint")
return null
}

private fun Breadcrumb.isValidForRRWebSpan(): Boolean {
val url = data["url"] as? String
val hasStartTimestamp = SpanDataConvention.HTTP_START_TIMESTAMP in data
val hasEndTimestamp = SpanDataConvention.HTTP_END_TIMESTAMP in data

val urlValid = !url.isNullOrEmpty()
val isValid = urlValid && hasStartTimestamp && hasEndTimestamp

val reasons = mutableListOf<String>()
if (!urlValid) reasons.add("missing or empty URL")
if (!hasStartTimestamp) reasons.add("missing start timestamp")
if (!hasEndTimestamp) reasons.add("missing end timestamp")

Log.d(
"SentryReplay",
"Breadcrumb RRWeb span validation: ${if (isValid) "VALID" else "INVALID"}" +
if (!isValid) " (${reasons.joinToString(", ")})"
else "" + " - URL: ${url ?: "null"}, Category: ${category}",
)

return isValid
}

private fun String.snakeToCamelCase(): String =
replace(snakecasePattern) { it.value.last().toString().uppercase() }
Expand All @@ -132,6 +215,16 @@ public open class DefaultReplayBreadcrumbConverter : ReplayBreadcrumbConverter {
val breadcrumb = this
val httpStartTimestamp = breadcrumb.data[SpanDataConvention.HTTP_START_TIMESTAMP]
val httpEndTimestamp = breadcrumb.data[SpanDataConvention.HTTP_END_TIMESTAMP]

// Get the NetworkRequestData if available and remove it from the map
val networkDetailData = httpNetworkDetails.remove(breadcrumb)

Log.d(
"SentryNetwork",
"SentryNetwork: convert(breadcrumb=${breadcrumb.type}) httpNetworkDetails map size: ${httpNetworkDetails.size}, " +
"found network data for current breadcrumb: ${networkDetailData != null}",
)

return RRWebSpanEvent().apply {
timestamp = breadcrumb.timestamp.time
op = "resource.http"
Expand All @@ -151,13 +244,47 @@ public open class DefaultReplayBreadcrumbConverter : ReplayBreadcrumbConverter {
}

val breadcrumbData = mutableMapOf<String, Any?>()

// Add data from NetworkRequestData if available
networkDetailData?.let { networkData ->
networkData.method?.let { breadcrumbData["method"] = it }
networkData.statusCode?.let { breadcrumbData["statusCode"] = it }
networkData.requestBodySize?.let { breadcrumbData["requestBodySize"] = it }
networkData.responseBodySize?.let { breadcrumbData["responseBodySize"] = it }

networkData.request?.let { request ->
val requestData = mutableMapOf<String, Any?>()
request.size?.let { requestData["size"] = it }
request.body?.let { requestData["body"] = it }
if (request.headers.isNotEmpty()) {
requestData["headers"] = request.headers
}
if (requestData.isNotEmpty()) {
breadcrumbData["request"] = requestData
}
}

networkData.response?.let { response ->
val responseData = mutableMapOf<String, Any?>()
response.size?.let { responseData["size"] = it }
response.body?.let { responseData["body"] = it }
if (response.headers.isNotEmpty()) {
responseData["headers"] = response.headers
}
if (responseData.isNotEmpty()) {
breadcrumbData["response"] = responseData
}
}
}
// Original breadcrumb http data
for ((key, value) in breadcrumb.data) {
if (key in supportedNetworkData) {
breadcrumbData[
key.replace("content_length", "body_size").substringAfter(".").snakeToCamelCase(),
] = value
}
}

data = breadcrumbData
}
}
Expand Down
Loading