Skip to content

Commit 68f8bfc

Browse files
romtsnromtsn
authored
Merge 94e8039 into b03eb7f 
2 parents b03eb7f + 94e8039 commit 68f8bfc

File tree

8 files changed

+137
-62
lines changed

8 files changed

+137
-62
lines changed

‎CHANGELOG.md

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,14 @@
11
# Changelog
22

3+
## Unreleased
4+
5+
### Fixes
6+
7+
- Session Replay: fix various crashes and issues ([#3970](https://github.com/getsentry/sentry-java/pull/3970))
8+
- Fix `IndexOutOfBoundsException` when tracking window changes
9+
- Fix `IllegalStateException` when adding/removing draw listener for a dead view
10+
- Fix `ConcurrentModificationException` when registering window listeners and stopping `WindowRecorder`/`GestureRecorder`
11+
312
## 7.18.1
413

514
### Fixes

‎sentry-android-replay/src/main/java/io/sentry/android/replay/ReplayIntegration.kt

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -93,7 +93,7 @@ public class ReplayIntegration(
9393
private var recorder: Recorder? = null
9494
private var gestureRecorder: GestureRecorder? = null
9595
private val random by lazy { Random() }
96-
private val rootViewsSpy by lazy(NONE) { RootViewsSpy.install() }
96+
internal val rootViewsSpy by lazy(NONE) { RootViewsSpy.install() }
9797

9898
// TODO: probably not everything has to be thread-safe here
9999
internal val isEnabled = AtomicBoolean(false)
@@ -263,6 +263,7 @@ public class ReplayIntegration(
263263
stop()
264264
recorder?.close()
265265
recorder = null
266+
rootViewsSpy.close()
266267
}
267268

268269
override fun onConfigurationChanged(newConfig: Configuration) {

‎sentry-android-replay/src/main/java/io/sentry/android/replay/ScreenshotRecorder.kt

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -23,8 +23,10 @@ import io.sentry.SentryLevel.WARNING
2323
import io.sentry.SentryOptions
2424
import io.sentry.SentryReplayOptions
2525
import io.sentry.android.replay.util.MainLooperHandler
26+
import io.sentry.android.replay.util.addOnDrawListenerSafe
2627
import io.sentry.android.replay.util.getVisibleRects
2728
import io.sentry.android.replay.util.gracefullyShutdown
29+
import io.sentry.android.replay.util.removeOnDrawListenerSafe
2830
import io.sentry.android.replay.util.submitSafely
2931
import io.sentry.android.replay.util.traverse
3032
import io.sentry.android.replay.viewhierarchy.ViewHierarchyNode
@@ -204,13 +206,13 @@ internal class ScreenshotRecorder(
204206

205207
// next bind the new root
206208
rootView = WeakReference(root)
207-
root.viewTreeObserver?.addOnDrawListener(this)
209+
root.addOnDrawListenerSafe(this)
208210
// invalidate the flag to capture the first frame after new window is attached
209211
contentChanged.set(true)
210212
}
211213

212214
fun unbind(root: View?) {
213-
root?.viewTreeObserver?.removeOnDrawListener(this)
215+
root?.removeOnDrawListenerSafe(this)
214216
}
215217

216218
fun pause() {
@@ -220,7 +222,7 @@ internal class ScreenshotRecorder(
220222

221223
fun resume() {
222224
// can't use bind() as it will invalidate the weakref
223-
rootView?.get()?.viewTreeObserver?.addOnDrawListener(this)
225+
rootView?.get()?.addOnDrawListenerSafe(this)
224226
isCapturing.set(true)
225227
}
226228

‎sentry-android-replay/src/main/java/io/sentry/android/replay/WindowRecorder.kt

Lines changed: 18 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -26,23 +26,28 @@ internal class WindowRecorder(
2626

2727
private val isRecording = AtomicBoolean(false)
2828
private val rootViews = ArrayList<WeakReference<View>>()
29+
private val rootViewsLock = Any()
2930
private var recorder: ScreenshotRecorder? = null
3031
private var capturingTask: ScheduledFuture<*>? = null
3132
private val capturer by lazy {
3233
Executors.newSingleThreadScheduledExecutor(RecorderExecutorServiceThreadFactory())
3334
}
3435

3536
override fun onRootViewsChanged(root: View, added: Boolean) {
36-
if (added) {
37-
rootViews.add(WeakReference(root))
38-
recorder?.bind(root)
39-
} else {
40-
recorder?.unbind(root)
41-
rootViews.removeAll { it.get() == root }
37+
synchronized(rootViewsLock) {
38+
if (added) {
39+
rootViews.add(WeakReference(root))
40+
recorder?.bind(root)
41+
} else {
42+
recorder?.unbind(root)
43+
rootViews.removeAll { it.get() == root }
4244

43-
val newRoot = rootViews.lastOrNull()?.get()
44-
if (newRoot != null && root != newRoot) {
45-
recorder?.bind(newRoot)
45+
val newRoot = rootViews.lastOrNull()?.get()
46+
if (newRoot != null && root != newRoot) {
47+
recorder?.bind(newRoot)
48+
} else {
49+
Unit // synchronized block wants us to return something lol
50+
}
4651
}
4752
}
4853
}
@@ -72,9 +77,11 @@ internal class WindowRecorder(
7277
}
7378

7479
override fun stop() {
75-
rootViews.forEach { recorder?.unbind(it.get()) }
80+
synchronized(rootViewsLock) {
81+
rootViews.forEach { recorder?.unbind(it.get()) }
82+
rootViews.clear()
83+
}
7684
recorder?.close()
77-
rootViews.clear()
7885
recorder = null
7986
capturingTask?.cancel(false)
8087
capturingTask = null

‎sentry-android-replay/src/main/java/io/sentry/android/replay/Windows.kt

Lines changed: 37 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,9 @@ import android.os.Looper
2525
import android.util.Log
2626
import android.view.View
2727
import android.view.Window
28+
import java.io.Closeable
2829
import java.util.concurrent.CopyOnWriteArrayList
30+
import java.util.concurrent.atomic.AtomicBoolean
2931
import kotlin.LazyThreadSafetyMode.NONE
3032

3133
/**
@@ -41,35 +43,21 @@ internal val View.phoneWindow: Window?
4143
return WindowSpy.pullWindow(rootView)
4244
}
4345

46+
@SuppressLint("PrivateApi")
4447
internal object WindowSpy {
4548

4649
/**
47-
* Originally, DecorView was an inner class of PhoneWindow. In the initial import in 2009,
48-
* PhoneWindow is in com.android.internal.policy.impl.PhoneWindow and that didn't change until
49-
* API 23.
50-
* In API 22: https://android.googlesource.com/platform/frameworks/base/+/android-5.1.1_r38/policy/src/com/android/internal/policy/impl/PhoneWindow.java
51-
* PhoneWindow was then moved to android.view and then again to com.android.internal.policy
52-
* https://android.googlesource.com/platform/frameworks/base/+/b10e33ff804a831c71be9303146cea892b9aeb5d
53-
* https://android.googlesource.com/platform/frameworks/base/+/6711f3b34c2ad9c622f56a08b81e313795fe7647
54-
* In API 23: https://android.googlesource.com/platform/frameworks/base/+/android-6.0.0_r1/core/java/com/android/internal/policy/PhoneWindow.java
55-
* Then DecorView moved out of PhoneWindow into its own class:
50+
* DecorView moved out of PhoneWindow into its own class:
5651
* https://android.googlesource.com/platform/frameworks/base/+/8804af2b63b0584034f7ec7d4dc701d06e6a8754
5752
* In API 24: https://android.googlesource.com/platform/frameworks/base/+/android-7.0.0_r1/core/java/com/android/internal/policy/DecorView.java
5853
*/
5954
private val decorViewClass by lazy(NONE) {
60-
val sdkInt = SDK_INT
61-
// TODO: we can only consider API 26
62-
val decorViewClassName = when {
63-
sdkInt >= 24 -> "com.android.internal.policy.DecorView"
64-
sdkInt == 23 -> "com.android.internal.policy.PhoneWindow\$DecorView"
65-
else -> "com.android.internal.policy.impl.PhoneWindow\$DecorView"
66-
}
6755
try {
68-
Class.forName(decorViewClassName)
56+
Class.forName("com.android.internal.policy.DecorView")
6957
} catch (ignored: Throwable) {
7058
Log.d(
7159
"WindowSpy",
72-
"Unexpected exception loading $decorViewClassName on API $sdkInt",
60+
"Unexpected exception loading DecorView on API $SDK_INT",
7361
ignored
7462
)
7563
null
@@ -83,18 +71,16 @@ internal object WindowSpy {
8371
* https://android.googlesource.com/platform/frameworks/base/+/0daf2102a20d224edeb4ee45dd4ee91889ef3e0c
8472
* Then it was extracted into a separate class.
8573
*
86-
* Hence the change of window field name from "this$0" to "mWindow" on API 24+.
74+
* Hence we use "mWindow" on API 24+.
8775
*/
8876
private val windowField by lazy(NONE) {
8977
decorViewClass?.let { decorViewClass ->
90-
val sdkInt = SDK_INT
91-
val fieldName = if (sdkInt >= 24) "mWindow" else "this$0"
9278
try {
93-
decorViewClass.getDeclaredField(fieldName).apply { isAccessible = true }
79+
decorViewClass.getDeclaredField("mWindow").apply { isAccessible = true }
9480
} catch (ignored: NoSuchFieldException) {
9581
Log.d(
9682
"WindowSpy",
97-
"Unexpected exception retrieving $decorViewClass#$fieldName on API $sdkInt",
83+
"Unexpected exception retrieving $decorViewClass#mWindow on API $SDK_INT",
9884
ignored
9985
)
10086
null
@@ -134,13 +120,18 @@ internal fun interface OnRootViewsChangedListener {
134120
/**
135121
* A utility that holds the list of root views that WindowManager updates.
136122
*/
137-
internal object RootViewsSpy {
123+
internal class RootViewsSpy private constructor() : Closeable {
124+
125+
private val isClosed = AtomicBoolean(false)
126+
private val viewListLock = Any()
138127

139128
val listeners: CopyOnWriteArrayList<OnRootViewsChangedListener> = object : CopyOnWriteArrayList<OnRootViewsChangedListener>() {
140129
override fun add(element: OnRootViewsChangedListener?): Boolean {
141-
// notify listener about existing root views immediately
142-
delegatingViewList.forEach {
143-
element?.onRootViewsChanged(it, true)
130+
synchronized(viewListLock) {
131+
// notify listener about existing root views immediately
132+
delegatingViewList.forEach {
133+
element?.onRootViewsChanged(it, true)
134+
}
144135
}
145136
return super.add(element)
146137
}
@@ -168,13 +159,25 @@ internal object RootViewsSpy {
168159
}
169160
}
170161

171-
fun install(): RootViewsSpy {
172-
return apply {
173-
// had to do this as a first message of the main thread queue, otherwise if this is
174-
// called from ContentProvider, it might be too early and the listener won't be installed
175-
Handler(Looper.getMainLooper()).postAtFrontOfQueue {
176-
WindowManagerSpy.swapWindowManagerGlobalMViews { mViews ->
177-
delegatingViewList.apply { addAll(mViews) }
162+
override fun close() {
163+
isClosed.set(true)
164+
listeners.clear()
165+
}
166+
167+
companion object {
168+
fun install(): RootViewsSpy {
169+
return RootViewsSpy().apply {
170+
// had to do this on the main thread queue, otherwise if this is
171+
// called from ContentProvider, it might be too early and the listener won't be installed
172+
Handler(Looper.getMainLooper()).postAtFrontOfQueue {
173+
if (isClosed.get()) {
174+
return@postAtFrontOfQueue
175+
}
176+
WindowManagerSpy.swapWindowManagerGlobalMViews { mViews ->
177+
synchronized(viewListLock) {
178+
delegatingViewList.apply { addAll(mViews) }
179+
}
180+
}
178181
}
179182
}
180183
}
@@ -206,9 +209,6 @@ internal object WindowManagerSpy {
206209
// You can discourage me all you want I'll still do it.
207210
@SuppressLint("PrivateApi", "ObsoleteSdkInt", "DiscouragedPrivateApi")
208211
fun swapWindowManagerGlobalMViews(swap: (ArrayList<View>) -> ArrayList<View>) {
209-
if (SDK_INT < 19) {
210-
return
211-
}
212212
try {
213213
windowManagerInstance?.let { windowManagerInstance ->
214214
mViewsField?.let { mViewsField ->

‎sentry-android-replay/src/main/java/io/sentry/android/replay/gestures/GestureRecorder.kt

Lines changed: 16 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -17,20 +17,25 @@ class GestureRecorder(
1717
) : OnRootViewsChangedListener {
1818

1919
private val rootViews = ArrayList<WeakReference<View>>()
20+
private val rootViewsLock = Any()
2021

2122
override fun onRootViewsChanged(root: View, added: Boolean) {
22-
if (added) {
23-
rootViews.add(WeakReference(root))
24-
root.startGestureTracking()
25-
} else {
26-
root.stopGestureTracking()
27-
rootViews.removeAll { it.get() == root }
23+
synchronized(rootViewsLock) {
24+
if (added) {
25+
rootViews.add(WeakReference(root))
26+
root.startGestureTracking()
27+
} else {
28+
root.stopGestureTracking()
29+
rootViews.removeAll { it.get() == root }
30+
}
2831
}
2932
}
3033

3134
fun stop() {
32-
rootViews.forEach { it.get()?.stopGestureTracking() }
33-
rootViews.clear()
35+
synchronized(rootViewsLock) {
36+
rootViews.forEach { it.get()?.stopGestureTracking() }
37+
rootViews.clear()
38+
}
3439
}
3540

3641
private fun View.startGestureTracking() {
@@ -53,8 +58,9 @@ class GestureRecorder(
5358
return
5459
}
5560

56-
if (window.callback is SentryReplayGestureRecorder) {
57-
val delegate = (window.callback as SentryReplayGestureRecorder).delegate
61+
val callback = window.callback
62+
if (callback is SentryReplayGestureRecorder) {
63+
val delegate = callback.delegate
5864
window.callback = delegate
5965
}
6066
}

‎sentry-android-replay/src/main/java/io/sentry/android/replay/util/Views.kt

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import android.text.Spanned
1717
import android.text.style.ForegroundColorSpan
1818
import android.view.View
1919
import android.view.ViewGroup
20+
import android.view.ViewTreeObserver
2021
import android.widget.TextView
2122
import io.sentry.SentryOptions
2223
import io.sentry.android.replay.viewhierarchy.ComposeViewHierarchyNode
@@ -178,3 +179,17 @@ class AndroidTextLayout(private val layout: Layout) : TextLayout {
178179
override fun getLineBottom(line: Int): Int = layout.getLineBottom(line)
179180
override fun getLineStart(line: Int): Int = layout.getLineStart(line)
180181
}
182+
183+
internal fun View?.addOnDrawListenerSafe(listener: ViewTreeObserver.OnDrawListener) {
184+
if (this == null || viewTreeObserver == null || !viewTreeObserver.isAlive) {
185+
return
186+
}
187+
viewTreeObserver.addOnDrawListener(listener)
188+
}
189+
190+
internal fun View?.removeOnDrawListenerSafe(listener: ViewTreeObserver.OnDrawListener) {
191+
if (this == null || viewTreeObserver == null || !viewTreeObserver.isAlive) {
192+
return
193+
}
194+
viewTreeObserver.removeOnDrawListener(listener)
195+
}

‎sentry-android-replay/src/test/java/io/sentry/android/replay/ReplaySmokeTest.kt

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ import java.util.concurrent.TimeUnit
4545
import java.util.concurrent.atomic.AtomicBoolean
4646
import kotlin.test.BeforeTest
4747
import kotlin.test.assertEquals
48+
import kotlin.test.assertNotEquals
4849

4950
@RunWith(AndroidJUnit4::class)
5051
@Config(
@@ -200,6 +201,40 @@ class ReplaySmokeTest {
200201
}
201202
)
202203
}
204+
205+
@Test
206+
fun `works when double inited`() {
207+
fixture.options.experimental.sessionReplay.sessionSampleRate = 1.0
208+
fixture.options.cacheDirPath = tmpDir.newFolder().absolutePath
209+
210+
// first init + close
211+
val falseHub = mock<IHub> {
212+
doAnswer {
213+
(it.arguments[0] as ScopeCallback).run(fixture.scope)
214+
}.whenever(it).configureScope(any())
215+
}
216+
val falseReplay: ReplayIntegration = fixture.getSut(context)
217+
falseReplay.register(falseHub, fixture.options)
218+
falseReplay.start()
219+
falseReplay.close()
220+
221+
// second init
222+
val captured = AtomicBoolean(false)
223+
whenever(fixture.hub.captureReplay(any(), anyOrNull())).then {
224+
captured.set(true)
225+
}
226+
val replay: ReplayIntegration = fixture.getSut(context)
227+
replay.register(fixture.hub, fixture.options)
228+
replay.start()
229+
230+
val controller = buildActivity(ExampleActivity::class.java, null).setup()
231+
controller.create().start().resume()
232+
// wait for windows to be registered in our listeners
233+
shadowOf(Looper.getMainLooper()).idle()
234+
235+
assertNotEquals(falseReplay.rootViewsSpy, replay.rootViewsSpy)
236+
assertEquals(0, falseReplay.rootViewsSpy.listeners.size)
237+
}
203238
}
204239

205240
private class ExampleActivity : Activity() {

0 commit comments

Comments
 (0)