[SR] Fix various crashes (#3970)

romtsn · web-flow · commit e22790622112 · 2024-12-10T09:55:35.000+01:00
* Fix IndexOutOfBoundsException in window tracking

* Fix IllegalStateException when adding/removing draw listener for a dead view

* Fix ConcurrentModificationException when registering window listeners

* Fix ConcurrentModificationException and ClassCastException when stopping GestureRecorder

* Fix ConcurrentModificationException when stopping WindowRecorder

* Changelog

* Remove redundant annotations
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,12 @@
 
 ## Unreleased
 
+### Fixes
+
+- Session Replay: fix various crashes and issues ([#3970](https://github.com/getsentry/sentry-java/pull/3970))
+  - Fix `IndexOutOfBoundsException` when tracking window changes
+  - Fix `IllegalStateException` when adding/removing draw listener for a dead view
+  - Fix `ConcurrentModificationException` when registering window listeners and stopping `WindowRecorder`/`GestureRecorder`
 - Add support for setting sentry-native handler_strategy ([#3671](https://github.com/getsentry/sentry-java/pull/3671))
 
 ### Dependencies
diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt
@@ -93,7 +93,7 @@ public class ReplayIntegration(
     private var recorder: Recorder? = null
     private var gestureRecorder: GestureRecorder? = null
     private val random by lazy { Random() }
-    private val rootViewsSpy by lazy(NONE) { RootViewsSpy.install() }
+    internal val rootViewsSpy by lazy(NONE) { RootViewsSpy.install() }
 
     // TODO: probably not everything has to be thread-safe here
     internal val isEnabled = AtomicBoolean(false)
@@ -263,6 +263,7 @@ public class ReplayIntegration(
         stop()
         recorder?.close()
         recorder = null
+        rootViewsSpy.close()
     }
 
     override fun onConfigurationChanged(newConfig: Configuration) {
diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt
@@ -23,8 +23,10 @@ import io.sentry.SentryLevel.WARNING
 import io.sentry.SentryOptions
 import io.sentry.SentryReplayOptions
 import io.sentry.android.replay.util.MainLooperHandler
+import io.sentry.android.replay.util.addOnDrawListenerSafe
 import io.sentry.android.replay.util.getVisibleRects
 import io.sentry.android.replay.util.gracefullyShutdown
+import io.sentry.android.replay.util.removeOnDrawListenerSafe
 import io.sentry.android.replay.util.submitSafely
 import io.sentry.android.replay.util.traverse
 import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode
@@ -204,13 +206,13 @@ internal class ScreenshotRecorder(
 
         // next bind the new root
         rootView = WeakReference(root)
-        root.viewTreeObserver?.addOnDrawListener(this)
+        root.addOnDrawListenerSafe(this)
         // invalidate the flag to capture the first frame after new window is attached
         contentChanged.set(true)
     }
 
     fun unbind(root: View?) {
-        root?.viewTreeObserver?.removeOnDrawListener(this)
+        root?.removeOnDrawListenerSafe(this)
     }
 
     fun pause() {
@@ -220,7 +222,7 @@ internal class ScreenshotRecorder(
 
     fun resume() {
         // can't use bind() as it will invalidate the weakref
-        rootView?.get()?.viewTreeObserver?.addOnDrawListener(this)
+        rootView?.get()?.addOnDrawListenerSafe(this)
         isCapturing.set(true)
     }
 
diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt
@@ -26,23 +26,28 @@ internal class WindowRecorder(
 
     private val isRecording = AtomicBoolean(false)
     private val rootViews = ArrayList<WeakReference<View>>()
+    private val rootViewsLock = Any()
     private var recorder: ScreenshotRecorder? = null
     private var capturingTask: ScheduledFuture<*>? = null
     private val capturer by lazy {
         Executors.newSingleThreadScheduledExecutor(RecorderExecutorServiceThreadFactory())
     }
 
     override fun onRootViewsChanged(root: View, added: Boolean) {
-        if (added) {
-            rootViews.add(WeakReference(root))
-            recorder?.bind(root)
-        } else {
-            recorder?.unbind(root)
-            rootViews.removeAll { it.get() == root }
+        synchronized(rootViewsLock) {
+            if (added) {
+                rootViews.add(WeakReference(root))
+                recorder?.bind(root)
+            } else {
+                recorder?.unbind(root)
+                rootViews.removeAll { it.get() == root }
 
-            val newRoot = rootViews.lastOrNull()?.get()
-            if (newRoot != null && root != newRoot) {
-                recorder?.bind(newRoot)
+                val newRoot = rootViews.lastOrNull()?.get()
+                if (newRoot != null && root != newRoot) {
+                    recorder?.bind(newRoot)
+                } else {
+                    Unit // synchronized block wants us to return something lol
+                }
             }
         }
     }
@@ -72,9 +77,11 @@ internal class WindowRecorder(
     }
 
     override fun stop() {
-        rootViews.forEach { recorder?.unbind(it.get()) }
+        synchronized(rootViewsLock) {
+            rootViews.forEach { recorder?.unbind(it.get()) }
+            rootViews.clear()
+        }
         recorder?.close()
-        rootViews.clear()
         recorder = null
         capturingTask?.cancel(false)
         capturingTask = null
diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/Windows.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/Windows.kt
@@ -25,7 +25,9 @@ import android.os.Looper
 import android.util.Log
 import android.view.View
 import android.view.Window
+import java.io.Closeable
 import java.util.concurrent.CopyOnWriteArrayList
+import java.util.concurrent.atomic.AtomicBoolean
 import kotlin.LazyThreadSafetyMode.NONE
 
 /**
@@ -41,35 +43,21 @@ internal val View.phoneWindow: Window?
         return WindowSpy.pullWindow(rootView)
     }
 
+@SuppressLint("PrivateApi")
 internal object WindowSpy {
 
     /**
-     * Originally, DecorView was an inner class of PhoneWindow. In the initial import in 2009,
-     * PhoneWindow is in com.android.internal.policy.impl.PhoneWindow and that didn't change until
-     * API 23.
-     * In API 22: https://android.googlesource.com/platform/frameworks/base/+/android-5.1.1_r38/policy/src/com/android/internal/policy/impl/PhoneWindow.java
-     * PhoneWindow was then moved to android.view and then again to com.android.internal.policy
-     * https://android.googlesource.com/platform/frameworks/base/+/b10e33ff804a831c71be9303146cea892b9aeb5d
-     * https://android.googlesource.com/platform/frameworks/base/+/6711f3b34c2ad9c622f56a08b81e313795fe7647
-     * In API 23: https://android.googlesource.com/platform/frameworks/base/+/android-6.0.0_r1/core/java/com/android/internal/policy/PhoneWindow.java
-     * Then DecorView moved out of PhoneWindow into its own class:
+     * DecorView moved out of PhoneWindow into its own class:
      * https://android.googlesource.com/platform/frameworks/base/+/8804af2b63b0584034f7ec7d4dc701d06e6a8754
      * In API 24: https://android.googlesource.com/platform/frameworks/base/+/android-7.0.0_r1/core/java/com/android/internal/policy/DecorView.java
      */
     private val decorViewClass by lazy(NONE) {
-        val sdkInt = SDK_INT
-        // TODO: we can only consider API 26
-        val decorViewClassName = when {
-            sdkInt >= 24 -> "com.android.internal.policy.DecorView"
-            sdkInt == 23 -> "com.android.internal.policy.PhoneWindow\$DecorView"
-            else -> "com.android.internal.policy.impl.PhoneWindow\$DecorView"
-        }
         try {
-            Class.forName(decorViewClassName)
+            Class.forName("com.android.internal.policy.DecorView")
         } catch (ignored: Throwable) {
             Log.d(
                 "WindowSpy",
-                "Unexpected exception loading $decorViewClassName on API $sdkInt",
+                "Unexpected exception loading DecorView on API $SDK_INT",
                 ignored
             )
             null
@@ -83,18 +71,16 @@ internal object WindowSpy {
      * https://android.googlesource.com/platform/frameworks/base/+/0daf2102a20d224edeb4ee45dd4ee91889ef3e0c
      * Then it was extracted into a separate class.
      *
-     * Hence the change of window field name from "this$0" to "mWindow" on API 24+.
+     * Hence we use "mWindow" on API 24+.
      */
     private val windowField by lazy(NONE) {
         decorViewClass?.let { decorViewClass ->
-            val sdkInt = SDK_INT
-            val fieldName = if (sdkInt >= 24) "mWindow" else "this$0"
             try {
-                decorViewClass.getDeclaredField(fieldName).apply { isAccessible = true }
+                decorViewClass.getDeclaredField("mWindow").apply { isAccessible = true }
             } catch (ignored: NoSuchFieldException) {
                 Log.d(
                     "WindowSpy",
-                    "Unexpected exception retrieving $decorViewClass#$fieldName on API $sdkInt",
+                    "Unexpected exception retrieving $decorViewClass#mWindow on API $SDK_INT",
                     ignored
                 )
                 null
@@ -134,13 +120,18 @@ internal fun interface OnRootViewsChangedListener {
 /**
  * A utility that holds the list of root views that WindowManager updates.
  */
-internal object RootViewsSpy {
+internal class RootViewsSpy private constructor() : Closeable {
+
+    private val isClosed = AtomicBoolean(false)
+    private val viewListLock = Any()
 
     val listeners: CopyOnWriteArrayList<OnRootViewsChangedListener> = object : CopyOnWriteArrayList<OnRootViewsChangedListener>() {
         override fun add(element: OnRootViewsChangedListener?): Boolean {
-            // notify listener about existing root views immediately
-            delegatingViewList.forEach {
-                element?.onRootViewsChanged(it, true)
+            synchronized(viewListLock) {
+                // notify listener about existing root views immediately
+                delegatingViewList.forEach {
+                    element?.onRootViewsChanged(it, true)
+                }
             }
             return super.add(element)
         }
@@ -168,13 +159,25 @@ internal object RootViewsSpy {
         }
     }
 
-    fun install(): RootViewsSpy {
-        return apply {
-            // had to do this as a first message of the main thread queue, otherwise if this is
-            // called from ContentProvider, it might be too early and the listener won't be installed
-            Handler(Looper.getMainLooper()).postAtFrontOfQueue {
-                WindowManagerSpy.swapWindowManagerGlobalMViews { mViews ->
-                    delegatingViewList.apply { addAll(mViews) }
+    override fun close() {
+        isClosed.set(true)
+        listeners.clear()
+    }
+
+    companion object {
+        fun install(): RootViewsSpy {
+            return RootViewsSpy().apply {
+                // had to do this on the main thread queue, otherwise if this is
+                // called from ContentProvider, it might be too early and the listener won't be installed
+                Handler(Looper.getMainLooper()).postAtFrontOfQueue {
+                    if (isClosed.get()) {
+                        return@postAtFrontOfQueue
+                    }
+                    WindowManagerSpy.swapWindowManagerGlobalMViews { mViews ->
+                        synchronized(viewListLock) {
+                            delegatingViewList.apply { addAll(mViews) }
+                        }
+                    }
                 }
             }
         }
@@ -206,9 +209,6 @@ internal object WindowManagerSpy {
     // You can discourage me all you want I'll still do it.
     @SuppressLint("PrivateApi", "ObsoleteSdkInt", "DiscouragedPrivateApi")
     fun swapWindowManagerGlobalMViews(swap: (ArrayList<View>) -> ArrayList<View>) {
-        if (SDK_INT < 19) {
-            return
-        }
         try {
             windowManagerInstance?.let { windowManagerInstance ->
                 mViewsField?.let { mViewsField ->
diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/gestures/GestureRecorder.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/gestures/GestureRecorder.kt
@@ -17,20 +17,25 @@ class GestureRecorder(
 ) : OnRootViewsChangedListener {
 
     private val rootViews = ArrayList<WeakReference<View>>()
+    private val rootViewsLock = Any()
 
     override fun onRootViewsChanged(root: View, added: Boolean) {
-        if (added) {
-            rootViews.add(WeakReference(root))
-            root.startGestureTracking()
-        } else {
-            root.stopGestureTracking()
-            rootViews.removeAll { it.get() == root }
+        synchronized(rootViewsLock) {
+            if (added) {
+                rootViews.add(WeakReference(root))
+                root.startGestureTracking()
+            } else {
+                root.stopGestureTracking()
+                rootViews.removeAll { it.get() == root }
+            }
         }
     }
 
     fun stop() {
-        rootViews.forEach { it.get()?.stopGestureTracking() }
-        rootViews.clear()
+        synchronized(rootViewsLock) {
+            rootViews.forEach { it.get()?.stopGestureTracking() }
+            rootViews.clear()
+        }
     }
 
     private fun View.startGestureTracking() {
@@ -53,8 +58,9 @@ class GestureRecorder(
             return
         }
 
-        if (window.callback is SentryReplayGestureRecorder) {
-            val delegate = (window.callback as SentryReplayGestureRecorder).delegate
+        val callback = window.callback
+        if (callback is SentryReplayGestureRecorder) {
+            val delegate = callback.delegate
             window.callback = delegate
         }
     }
diff --git a/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Views.kt b/sentry-android-replay/src/main/java/io/sentry/android/replay/util/Views.kt
@@ -17,6 +17,7 @@ import android.text.Spanned
 import android.text.style.ForegroundColorSpan
 import android.view.View
 import android.view.ViewGroup
+import android.view.ViewTreeObserver
 import android.widget.TextView
 import io.sentry.SentryOptions
 import io.sentry.android.replay.viewhierarchy.ComposeViewHierarchyNode
@@ -178,3 +179,17 @@ class AndroidTextLayout(private val layout: Layout) : TextLayout {
     override fun getLineBottom(line: Int): Int = layout.getLineBottom(line)
     override fun getLineStart(line: Int): Int = layout.getLineStart(line)
 }
+
+internal fun View?.addOnDrawListenerSafe(listener: ViewTreeObserver.OnDrawListener) {
+    if (this == null || viewTreeObserver == null || !viewTreeObserver.isAlive) {
+        return
+    }
+    viewTreeObserver.addOnDrawListener(listener)
+}
+
+internal fun View?.removeOnDrawListenerSafe(listener: ViewTreeObserver.OnDrawListener) {
+    if (this == null || viewTreeObserver == null || !viewTreeObserver.isAlive) {
+        return
+    }
+    viewTreeObserver.removeOnDrawListener(listener)
+}
diff --git a/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt b/sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt
@@ -45,6 +45,7 @@ import java.util.concurrent.TimeUnit
 import java.util.concurrent.atomic.AtomicBoolean
 import kotlin.test.BeforeTest
 import kotlin.test.assertEquals
+import kotlin.test.assertNotEquals
 
 @RunWith(AndroidJUnit4::class)
 @Config(
@@ -200,6 +201,40 @@ class ReplaySmokeTest {
             }
         )
     }
+
+    @Test
+    fun `works when double inited`() {
+        fixture.options.experimental.sessionReplay.sessionSampleRate = 1.0
+        fixture.options.cacheDirPath = tmpDir.newFolder().absolutePath
+
+        // first init + close
+        val falseHub = mock<IHub> {
+            doAnswer {
+                (it.arguments[0] as ScopeCallback).run(fixture.scope)
+            }.whenever(it).configureScope(any())
+        }
+        val falseReplay: ReplayIntegration = fixture.getSut(context)
+        falseReplay.register(falseHub, fixture.options)
+        falseReplay.start()
+        falseReplay.close()
+
+        // second init
+        val captured = AtomicBoolean(false)
+        whenever(fixture.hub.captureReplay(any(), anyOrNull())).then {
+            captured.set(true)
+        }
+        val replay: ReplayIntegration = fixture.getSut(context)
+        replay.register(fixture.hub, fixture.options)
+        replay.start()
+
+        val controller = buildActivity(ExampleActivity::class.java, null).setup()
+        controller.create().start().resume()
+        // wait for windows to be registered in our listeners
+        shadowOf(Looper.getMainLooper()).idle()
+
+        assertNotEquals(falseReplay.rootViewsSpy, replay.rootViewsSpy)
+        assertEquals(0, falseReplay.rootViewsSpy.listeners.size)
+    }
 }
 
 private class ExampleActivity : Activity() {
