Skip to content

Commit 7770dd7

Browse files
romtsnmarkushi
romtsn
authored and
markushi
committed
Various fixes to instrumentations running on the main thread (#4051)
* Get rid of redundant requireNonNull * Do not instrument Window.Callback multiple times * Do not instrument FileIO if tracing is disabled * Do not traverse children if a touch event is not within view groups bounds * Add test for SentryFileOutputStream * Fix test * Fix test * Changelog * pr id * Fix api dump
1 parent 29a4185 commit 7770dd7

File tree

12 files changed

+217
-48
lines changed

12 files changed

+217
-48
lines changed

‎sentry-android-core/src/main/java/io/sentry/android/core/UserInteractionIntegration.java

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,11 @@ private void startTracking(final @NotNull Activity activity) {
5050
delegate = new NoOpWindowCallback();
5151
}
5252

53+
if (delegate instanceof SentryWindowCallback) {
54+
// already instrumented
55+
return;
56+
}
57+
5358
final SentryGestureListener gestureListener =
5459
new SentryGestureListener(activity, scopes, options);
5560
window.setCallback(new SentryWindowCallback(delegate, activity, gestureListener, options));

‎sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/AndroidViewGestureTargetLocator.java

Lines changed: 6 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -18,26 +18,23 @@ public final class AndroidViewGestureTargetLocator implements GestureTargetLocat
1818
private static final String ORIGIN = "old_view_system";
1919

2020
private final boolean isAndroidXAvailable;
21-
private final int[] coordinates = new int[2];
2221

2322
public AndroidViewGestureTargetLocator(final boolean isAndroidXAvailable) {
2423
this.isAndroidXAvailable = isAndroidXAvailable;
2524
}
2625

2726
@Override
2827
public @Nullable UiElement locate(
29-
@NotNull Object root, float x, float y, UiElement.Type targetType) {
28+
@Nullable Object root, float x, float y, UiElement.Type targetType) {
3029
if (!(root instanceof View)) {
3130
return null;
3231
}
3332
final View view = (View) root;
34-
if (touchWithinBounds(view, x, y)) {
35-
if (targetType == UiElement.Type.CLICKABLE && isViewTappable(view)) {
36-
return createUiElement(view);
37-
} else if (targetType == UiElement.Type.SCROLLABLE
38-
&& isViewScrollable(view, isAndroidXAvailable)) {
39-
return createUiElement(view);
40-
}
33+
if (targetType == UiElement.Type.CLICKABLE && isViewTappable(view)) {
34+
return createUiElement(view);
35+
} else if (targetType == UiElement.Type.SCROLLABLE
36+
&& isViewScrollable(view, isAndroidXAvailable)) {
37+
return createUiElement(view);
4138
}
4239
return null;
4340
}
@@ -52,17 +49,6 @@ private UiElement createUiElement(final @NotNull View targetView) {
5249
}
5350
}
5451

55-
private boolean touchWithinBounds(final @NotNull View view, final float x, final float y) {
56-
view.getLocationOnScreen(coordinates);
57-
int vx = coordinates[0];
58-
int vy = coordinates[1];
59-
60-
int w = view.getWidth();
61-
int h = view.getHeight();
62-
63-
return !(x < vx || x > vx + w || y < vy || y > vy + h);
64-
}
65-
6652
private static boolean isViewTappable(final @NotNull View view) {
6753
return view.isClickable() && view.getVisibility() == View.VISIBLE;
6854
}

‎sentry-android-core/src/main/java/io/sentry/android/core/internal/gestures/ViewUtils.java

Lines changed: 33 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,6 @@
77
import io.sentry.android.core.SentryAndroidOptions;
88
import io.sentry.internal.gestures.GestureTargetLocator;
99
import io.sentry.internal.gestures.UiElement;
10-
import io.sentry.util.Objects;
1110
import java.util.LinkedList;
1211
import java.util.Queue;
1312
import org.jetbrains.annotations.ApiStatus;
@@ -17,6 +16,32 @@
1716
@ApiStatus.Internal
1817
public final class ViewUtils {
1918

19+
private static final int[] coordinates = new int[2];
20+
21+
/**
22+
* Verifies if the given touch coordinates are within the bounds of the given view.
23+
*
24+
* @param view the view to check if the touch coordinates are within its bounds
25+
* @param x - the x coordinate of a {@link MotionEvent}
26+
* @param y - the y coordinate of {@link MotionEvent}
27+
* @return true if the touch coordinates are within the bounds of the view, false otherwise
28+
*/
29+
private static boolean touchWithinBounds(
30+
final @Nullable View view, final float x, final float y) {
31+
if (view == null) {
32+
return false;
33+
}
34+
35+
view.getLocationOnScreen(coordinates);
36+
int vx = coordinates[0];
37+
int vy = coordinates[1];
38+
39+
int w = view.getWidth();
40+
int h = view.getHeight();
41+
42+
return !(x < vx || x > vx + w || y < vy || y > vy + h);
43+
}
44+
2045
/**
2146
* Finds a target view, that has been selected/clicked by the given coordinates x and y and the
2247
* given {@code viewTargetSelector}.
@@ -40,7 +65,12 @@ public final class ViewUtils {
4065

4166
@Nullable UiElement target = null;
4267
while (queue.size() > 0) {
43-
final View view = Objects.requireNonNull(queue.poll(), "view is required");
68+
final View view = queue.poll();
69+
70+
if (!touchWithinBounds(view, x, y)) {
71+
// if the touch is not hitting the view, skip traversal of its children
72+
continue;
73+
}
4474

4575
if (view instanceof ViewGroup) {
4676
final ViewGroup viewGroup = (ViewGroup) view;
@@ -54,7 +84,7 @@ public final class ViewUtils {
5484
if (newTarget != null) {
5585
if (targetType == UiElement.Type.CLICKABLE) {
5686
target = newTarget;
57-
} else {
87+
} else if (targetType == UiElement.Type.SCROLLABLE) {
5888
return newTarget;
5989
}
6090
}

‎sentry-android-core/src/test/java/io/sentry/android/core/UserInteractionIntegrationTest.kt

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -140,6 +140,7 @@ class UserInteractionIntegrationTest {
140140
)
141141
)
142142

143+
sut.register(fixture.hub, fixture.options)
143144
sut.onActivityPaused(fixture.activity)
144145

145146
verify(fixture.window).callback = null
@@ -160,6 +161,7 @@ class UserInteractionIntegrationTest {
160161
)
161162
)
162163

164+
sut.register(fixture.hub, fixture.options)
163165
sut.onActivityPaused(fixture.activity)
164166

165167
verify(fixture.window).callback = delegate
@@ -170,8 +172,30 @@ class UserInteractionIntegrationTest {
170172
val callback = mock<SentryWindowCallback>()
171173
val sut = fixture.getSut(callback)
172174

175+
sut.register(fixture.hub, fixture.options)
173176
sut.onActivityPaused(fixture.activity)
174177

175178
verify(callback).stopTracking()
176179
}
180+
181+
@Test
182+
fun `does not instrument if the callback is already ours`() {
183+
val delegate = mock<Window.Callback>()
184+
val context = mock<Context>()
185+
val resources = Fixture.mockResources()
186+
whenever(context.resources).thenReturn(resources)
187+
val existingCallback = SentryWindowCallback(
188+
delegate,
189+
context,
190+
mock(),
191+
mock()
192+
)
193+
val sut = fixture.getSut(existingCallback)
194+
195+
sut.register(fixture.hub, fixture.options)
196+
sut.onActivityResumed(fixture.activity)
197+
198+
val argumentCaptor = argumentCaptor<Window.Callback>()
199+
verify(fixture.window, never()).callback = argumentCaptor.capture()
200+
}
177201
}

‎sentry-android-core/src/test/java/io/sentry/android/core/internal/gestures/SentryGestureListenerClickTest.kt

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -185,7 +185,7 @@ class SentryGestureListenerClickTest {
185185

186186
sut.onSingleTapUp(event)
187187

188-
verify(fixture.scopes, never()).addBreadcrumb(any<Breadcrumb>())
188+
verify(fixture.scopes, never()).addBreadcrumb(any<Breadcrumb>(), anyOrNull())
189189
}
190190

191191
@Test
@@ -214,7 +214,7 @@ class SentryGestureListenerClickTest {
214214

215215
sut.onSingleTapUp(event)
216216

217-
verify(fixture.scopes, never()).addBreadcrumb(any<Breadcrumb>())
217+
verify(fixture.scopes, never()).addBreadcrumb(any<Breadcrumb>(), anyOrNull())
218218
}
219219

220220
@Test
@@ -223,7 +223,7 @@ class SentryGestureListenerClickTest {
223223

224224
val event = mock<MotionEvent>()
225225
val sut = fixture.getSut<LocalView>(event, attachViewsToRoot = false)
226-
fixture.window.mockDecorView<ViewGroup>(event = event, touchWithinBounds = false) {
226+
fixture.window.mockDecorView<ViewGroup>(event = event, touchWithinBounds = true) {
227227
whenever(it.childCount).thenReturn(1)
228228
whenever(it.getChildAt(0)).thenReturn(fixture.target)
229229
}
@@ -244,7 +244,7 @@ class SentryGestureListenerClickTest {
244244

245245
val event = mock<MotionEvent>()
246246
val sut = fixture.getSut<LocalView>(event, attachViewsToRoot = false)
247-
fixture.window.mockDecorView<ViewGroup>(event = event, touchWithinBounds = false) {
247+
fixture.window.mockDecorView<ViewGroup>(event = event, touchWithinBounds = true) {
248248
whenever(it.childCount).thenReturn(1)
249249
whenever(it.getChildAt(0)).thenReturn(fixture.target)
250250
}
@@ -253,4 +253,18 @@ class SentryGestureListenerClickTest {
253253

254254
verify(fixture.scope).propagationContext = any()
255255
}
256+
257+
@Test
258+
fun `if touch is not within view group bounds does not traverse its children`() {
259+
val event = mock<MotionEvent>()
260+
val sut = fixture.getSut<View>(event, attachViewsToRoot = false)
261+
fixture.window.mockDecorView<ViewGroup>(event = event, touchWithinBounds = false) {
262+
whenever(it.childCount).thenReturn(1)
263+
whenever(it.getChildAt(0)).thenReturn(fixture.target)
264+
}
265+
266+
sut.onSingleTapUp(event)
267+
268+
verify(fixture.hub, never()).addBreadcrumb(any<Breadcrumb>(), anyOrNull())
269+
}
256270
}

‎sentry-compose-helper/src/jvmMain/java/io/sentry/compose/gestures/ComposeGestureTargetLocator.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ public ComposeGestureTargetLocator(final @NotNull ILogger logger) {
4242

4343
@Override
4444
public @Nullable UiElement locate(
45-
@NotNull Object root, float x, float y, UiElement.Type targetType) {
45+
@Nullable Object root, float x, float y, UiElement.Type targetType) {
4646

4747
// lazy init composeHelper as it's using some reflection under the hood
4848
if (composeHelper == null) {

‎sentry/api/sentry.api

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4152,6 +4152,7 @@ public final class io/sentry/instrumentation/file/SentryFileOutputStream : java/
41524152
public final class io/sentry/instrumentation/file/SentryFileOutputStream$Factory {
41534153
public fun <init> ()V
41544154
public static fun create (Ljava/io/FileOutputStream;Ljava/io/File;)Ljava/io/FileOutputStream;
4155+
public static fun create (Ljava/io/FileOutputStream;Ljava/io/File;Lio/sentry/IScopes;)Ljava/io/FileOutputStream;
41554156
public static fun create (Ljava/io/FileOutputStream;Ljava/io/File;Z)Ljava/io/FileOutputStream;
41564157
public static fun create (Ljava/io/FileOutputStream;Ljava/io/FileDescriptor;)Ljava/io/FileOutputStream;
41574158
public static fun create (Ljava/io/FileOutputStream;Ljava/lang/String;)Ljava/io/FileOutputStream;

‎sentry/src/main/java/io/sentry/instrumentation/file/SentryFileInputStream.java

Lines changed: 21 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import io.sentry.IScopes;
44
import io.sentry.ISpan;
55
import io.sentry.ScopesAdapter;
6+
import io.sentry.SentryOptions;
67
import java.io.File;
78
import java.io.FileDescriptor;
89
import java.io.FileInputStream;
@@ -127,28 +128,42 @@ public static final class Factory {
127128
public static FileInputStream create(
128129
final @NotNull FileInputStream delegate, final @Nullable String name)
129130
throws FileNotFoundException {
130-
return new SentryFileInputStream(
131-
init(name != null ? new File(name) : null, delegate, ScopesAdapter.getInstance()));
131+
final @NotNull IScopes scopes = ScopesAdapter.getInstance();
132+
return isTracingEnabled(scopes)
133+
? new SentryFileInputStream(init(name != null ? new File(name) : null, delegate, scopes))
134+
: delegate;
132135
}
133136

134137
public static FileInputStream create(
135138
final @NotNull FileInputStream delegate, final @Nullable File file)
136139
throws FileNotFoundException {
137-
return new SentryFileInputStream(init(file, delegate, ScopesAdapter.getInstance()));
140+
final @NotNull IScopes scopes = ScopesAdapter.getInstance();
141+
return isTracingEnabled(scopes)
142+
? new SentryFileInputStream(init(file, delegate, scopes))
143+
: delegate;
138144
}
139145

140146
public static FileInputStream create(
141147
final @NotNull FileInputStream delegate, final @NotNull FileDescriptor descriptor) {
142-
return new SentryFileInputStream(
143-
init(descriptor, delegate, ScopesAdapter.getInstance()), descriptor);
148+
final @NotNull IScopes scopes = ScopesAdapter.getInstance();
149+
return isTracingEnabled(scopes)
150+
? new SentryFileInputStream(init(descriptor, delegate, scopes), descriptor)
151+
: delegate;
144152
}
145153

146154
static FileInputStream create(
147155
final @NotNull FileInputStream delegate,
148156
final @Nullable File file,
149157
final @NotNull IScopes scopes)
150158
throws FileNotFoundException {
151-
return new SentryFileInputStream(init(file, delegate, scopes));
159+
return isTracingEnabled(scopes)
160+
? new SentryFileInputStream(init(file, delegate, scopes))
161+
: delegate;
162+
}
163+
164+
private static boolean isTracingEnabled(final @NotNull IScopes scopes) {
165+
final @NotNull SentryOptions options = scopes.getOptions();
166+
return options.isTracingEnabled();
152167
}
153168
}
154169
}

‎sentry/src/main/java/io/sentry/instrumentation/file/SentryFileOutputStream.java

Lines changed: 46 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import io.sentry.IScopes;
44
import io.sentry.ISpan;
55
import io.sentry.ScopesAdapter;
6+
import io.sentry.SentryOptions;
67
import java.io.File;
78
import java.io.FileDescriptor;
89
import java.io.FileNotFoundException;
@@ -134,33 +135,70 @@ public static final class Factory {
134135
public static FileOutputStream create(
135136
final @NotNull FileOutputStream delegate, final @Nullable String name)
136137
throws FileNotFoundException {
137-
return new SentryFileOutputStream(
138-
init(name != null ? new File(name) : null, false, delegate, ScopesAdapter.getInstance()));
138+
final @NotNull IScopes scopes = ScopesAdapter.getInstance();
139+
return isTracingEnabled(scopes)
140+
? new SentryFileOutputStream(
141+
init(
142+
name != null ? new File(name) : null,
143+
false,
144+
delegate,
145+
ScopesAdapter.getInstance()))
146+
: delegate;
139147
}
140148

141149
public static FileOutputStream create(
142150
final @NotNull FileOutputStream delegate, final @Nullable String name, final boolean append)
143151
throws FileNotFoundException {
144-
return new SentryFileOutputStream(
145-
init(
146-
name != null ? new File(name) : null, append, delegate, ScopesAdapter.getInstance()));
152+
final @NotNull IScopes scopes = ScopesAdapter.getInstance();
153+
return isTracingEnabled(scopes)
154+
? new SentryFileOutputStream(
155+
init(
156+
name != null ? new File(name) : null,
157+
append,
158+
delegate,
159+
ScopesAdapter.getInstance()))
160+
: delegate;
147161
}
148162

149163
public static FileOutputStream create(
150164
final @NotNull FileOutputStream delegate, final @Nullable File file)
151165
throws FileNotFoundException {
152-
return new SentryFileOutputStream(init(file, false, delegate, ScopesAdapter.getInstance()));
166+
final @NotNull IScopes scopes = ScopesAdapter.getInstance();
167+
return isTracingEnabled(scopes)
168+
? new SentryFileOutputStream(init(file, false, delegate, ScopesAdapter.getInstance()))
169+
: delegate;
153170
}
154171

155172
public static FileOutputStream create(
156173
final @NotNull FileOutputStream delegate, final @Nullable File file, final boolean append)
157174
throws FileNotFoundException {
158-
return new SentryFileOutputStream(init(file, append, delegate, ScopesAdapter.getInstance()));
175+
final @NotNull IScopes scopes = ScopesAdapter.getInstance();
176+
return isTracingEnabled(scopes)
177+
? new SentryFileOutputStream(init(file, append, delegate, ScopesAdapter.getInstance()))
178+
: delegate;
159179
}
160180

161181
public static FileOutputStream create(
162182
final @NotNull FileOutputStream delegate, final @NotNull FileDescriptor fdObj) {
163-
return new SentryFileOutputStream(init(fdObj, delegate, ScopesAdapter.getInstance()), fdObj);
183+
final @NotNull IScopes scopes = ScopesAdapter.getInstance();
184+
return isTracingEnabled(scopes)
185+
? new SentryFileOutputStream(init(fdObj, delegate, ScopesAdapter.getInstance()), fdObj)
186+
: delegate;
187+
}
188+
189+
public static FileOutputStream create(
190+
final @NotNull FileOutputStream delegate,
191+
final @Nullable File file,
192+
final @NotNull IScopes scopes)
193+
throws FileNotFoundException {
194+
return isTracingEnabled(scopes)
195+
? new SentryFileOutputStream(init(file, false, delegate, scopes))
196+
: delegate;
197+
}
198+
199+
private static boolean isTracingEnabled(final @NotNull IScopes scopes) {
200+
final @NotNull SentryOptions options = scopes.getOptions();
201+
return options.isTracingEnabled();
164202
}
165203
}
166204
}

0 commit comments

Comments
 (0)