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

[SR] Fix Session Replay crashes #3628

Merged
merged 23 commits into from
Aug 12, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
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
5 changes: 5 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,11 @@
- Align next segment timestamp with the end of the buffered segment when converting from buffer mode to session mode
- Persist `buffer` replay type for the entire replay when converting from buffer mode to session mode
- Properly store screen names for `buffer` mode
- Session Replay: fix various crashes and issues ([#3628](https://github.com/getsentry/sentry-java/pull/3628))
- Fix video not being encoded on Pixel devices
- Fix SIGABRT native crashes on Xiaomi devices when encoding a video
- Fix `RejectedExecutionException` when redacting a screenshot
- Fix `FileNotFoundException` when persisting segment values

### Chores

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import io.sentry.SentryReplayOptions
import io.sentry.android.replay.util.MainLooperHandler
import io.sentry.android.replay.util.getVisibleRects
import io.sentry.android.replay.util.gracefullyShutdown
import io.sentry.android.replay.util.submitSafely
import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode
import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode.ImageViewHierarchyNode
import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode.TextViewHierarchyNode
Expand Down Expand Up @@ -122,7 +123,7 @@ internal class ScreenshotRecorder(
val viewHierarchy = ViewHierarchyNode.fromView(root, null, 0, options)
root.traverse(viewHierarchy)

recorder.submit {
recorder.submitSafely(options, "screenshot_recorder.redact") {
val canvas = Canvas(bitmap)
canvas.setMatrix(prescaledMatrix)
viewHierarchy.traverse { node ->
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -100,10 +100,10 @@ internal abstract class BaseCaptureStrategy(
) {
cache = replayCacheProvider?.invoke(replayId, recorderConfig) ?: ReplayCache(options, replayId, recorderConfig)

this.currentReplayId = replayId
this.currentSegment = segmentId
this.replayType = replayType ?: (if (this is SessionCaptureStrategy) SESSION else BUFFER)
this.recorderConfig = recorderConfig
this.currentSegment = segmentId
this.currentReplayId = replayId

segmentTimestamp = DateUtils.getCurrentDateTime()
replayStartTimestamp.set(dateProvider.currentTimeMillis)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,6 @@ internal class SessionCaptureStrategy(
}

override fun onConfigurationChanged(recorderConfig: ScreenshotRecorderConfig) {
val currentSegmentTimestamp = segmentTimestamp ?: return
createCurrentSegment("onConfigurationChanged") { segment ->
if (segment is ReplaySegment.Created) {
segment.capture(hub)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,8 @@ import android.os.Build.VERSION
import android.os.Build.VERSION_CODES
import android.text.Layout
import android.view.View
import android.widget.TextView
import java.lang.NullPointerException

/**
* Adapted copy of AccessibilityNodeInfo from https://cs.android.com/android/platform/superproject/+/master:frameworks/base/core/java/android/view/View.java;l=10718
Expand All @@ -26,7 +28,7 @@ internal fun View.isVisibleToUser(): Pair<Boolean, Rect?> {
}
// An invisible predecessor or one with alpha zero means
// that this view is not visible to the user.
var current: Any = this
var current: Any? = this
while (current is View) {
val view = current
val transitionAlpha = if (VERSION.SDK_INT >= VERSION_CODES.Q) view.transitionAlpha else 1f
Expand All @@ -53,7 +55,10 @@ internal fun Drawable?.isRedactable(): Boolean {
// TODO: otherwise maybe check for the bitmap size and don't redact those that take a lot of height (e.g. a background of a whatsapp chat)
return when (this) {
is InsetDrawable, is ColorDrawable, is VectorDrawable, is GradientDrawable -> false
is BitmapDrawable -> !bitmap.isRecycled && bitmap.height > 10 && bitmap.width > 10
is BitmapDrawable -> {
val bmp = bitmap ?: return false
return !bmp.isRecycled && bmp.height > 10 && bmp.width > 10
}
else -> true
}
}
Expand Down Expand Up @@ -84,3 +89,15 @@ internal fun Layout?.getVisibleRects(globalRect: Rect, paddingLeft: Int, padding
}
return rects
}

/**
* [TextView.getVerticalOffset] which is used by [TextView.getTotalPaddingTop] may throw an NPE on
* some devices (Redmi), so we try-catch it specifically for an NPE and then fallback to
* [TextView.getExtendedPaddingTop]
*/
internal val TextView.totalPaddingTopSafe: Int
get() = try {
totalPaddingTop
} catch (e: NullPointerException) {
extendedPaddingTop
}
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,9 @@ import android.annotation.TargetApi
import android.graphics.Bitmap
import android.media.MediaCodec
import android.media.MediaCodecInfo
import android.media.MediaCodecList
import android.media.MediaFormat
import android.os.Build
import android.view.Surface
import io.sentry.SentryLevel.DEBUG
import io.sentry.SentryOptions
Expand All @@ -50,8 +52,22 @@ internal class SimpleVideoEncoder(
val onClose: (() -> Unit)? = null
) {

private val hasExynosCodec: Boolean by lazy(NONE) {
// MediaCodecList ctor will initialize an internal in-memory static cache of codecs, so this
// call is only expensive the first time
MediaCodecList(MediaCodecList.REGULAR_CODECS)
.codecInfos
.any { it.name.contains("c2.exynos") }
}

internal val mediaCodec: MediaCodec = run {
val codec = MediaCodec.createEncoderByType(muxerConfig.mimeType)
// c2.exynos.h264.encoder seems to have problems encoding the video (Pixel and Samsung devices)
// so we use the default encoder instead
val codec = if (hasExynosCodec) {
MediaCodec.createByCodecName("c2.android.avc.encoder")
} else {
MediaCodec.createEncoderByType(muxerConfig.mimeType)
}

codec
}
Expand Down Expand Up @@ -139,10 +155,13 @@ internal class SimpleVideoEncoder(
}

fun encode(image: Bitmap) {
// NOTE do not use `lockCanvas` like what is done in bitmap2video
// This is because https://developer.android.com/reference/android/media/MediaCodec#createInputSurface()
// says that, "Surface.lockCanvas(android.graphics.Rect) may fail or produce unexpected results."
val canvas = surface?.lockHardwareCanvas()
// it seems that Xiaomi devices have problems with hardware canvas, so we have to use
// lockCanvas instead https://stackoverflow.com/a/73520742
val canvas = if (Build.MANUFACTURER.contains("xiaomi", ignoreCase = true)) {
surface?.lockCanvas(null)
} else {
surface?.lockHardwareCanvas()
}
canvas?.drawBitmap(image, 0f, 0f, null)
surface?.unlockCanvasAndPost(canvas)
drainCodec(false)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import android.widget.TextView
import io.sentry.SentryOptions
import io.sentry.android.replay.util.isRedactable
import io.sentry.android.replay.util.isVisibleToUser
import io.sentry.android.replay.util.totalPaddingTopSafe

@TargetApi(26)
sealed class ViewHierarchyNode(
Expand Down Expand Up @@ -245,7 +246,7 @@ sealed class ViewHierarchyNode(
layout = view.layout,
dominantColor = view.currentTextColor.toOpaque(),
paddingLeft = view.totalPaddingLeft,
paddingTop = view.totalPaddingTop,
paddingTop = view.totalPaddingTopSafe,
x = view.x,
y = view.y,
width = view.width,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ class BufferCaptureStrategyTest {
(it.arguments[0] as ScopeCallback).run(scope)
}.whenever(it).configureScope(any())
}
var persistedSegment = mutableMapOf<String, String?>()
var persistedSegment = LinkedHashMap<String, String?>()
val replayCache = mock<ReplayCache> {
on { frames }.thenReturn(mutableListOf(ReplayFrame(File("1720693523997.jpg"), 1720693523997)))
on { persistSegmentValues(any(), anyOrNull()) }.then {
Expand Down Expand Up @@ -293,4 +293,19 @@ class BufferCaptureStrategyTest {

verify(fixture.hub).captureReplay(any(), any())
}

@Test
fun `replayId should be set and serialized first`() {
val strategy = fixture.getSut()
val replayId = SentryId()

strategy.start(fixture.recorderConfig, 0, replayId)

assertEquals(
replayId.toString(),
fixture.persistedSegment.values.first(),
"The replayId must be set first, so when we clean up stale replays" +
"the current replay cache folder is not being deleted."
)
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ class SessionCaptureStrategyTest {
(it.arguments[0] as ScopeCallback).run(scope)
}.whenever(it).configureScope(any())
}
var persistedSegment = mutableMapOf<String, String?>()
var persistedSegment = LinkedHashMap<String, String?>()
val replayCache = mock<ReplayCache> {
on { frames }.thenReturn(mutableListOf(ReplayFrame(File("1720693523997.jpg"), 1720693523997)))
on { persistSegmentValues(any(), anyOrNull()) }.then {
Expand Down Expand Up @@ -352,4 +352,19 @@ class SessionCaptureStrategyTest {
}
)
}

@Test
fun `replayId should be set and serialized first`() {
val strategy = fixture.getSut()
val replayId = SentryId()

strategy.start(fixture.recorderConfig, 0, replayId)

assertEquals(
replayId.toString(),
fixture.persistedSegment.values.first(),
"The replayId must be set first, so when we clean up stale replays" +
"the current replay cache folder is not being deleted."
)
}
}
Loading