Skip to content

Commit ed98d11

Browse files
romtsnromtsn
authored
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 2de45eb commit ed98d11

File tree

13 files changed

+215
-52
lines changed

13 files changed

+215
-52
lines changed

‎CHANGELOG.md‎

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

3+
## Unreleased
4+
5+
### Fixes
6+
7+
- Do not instrument File I/O operations if tracing is disabled ([#4051](https://github.com/getsentry/sentry-java/pull/4051))
8+
- Do not instrument User Interaction multiple times ([#4051](https://github.com/getsentry/sentry-java/pull/4051))
9+
- Speed up view traversal to find touched target in `UserInteractionIntegration` ([#4051](https://github.com/getsentry/sentry-java/pull/4051))
10+
311
## 7.20.0
412

513
### Features

‎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, hub, 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.hub, never()).addBreadcrumb(any<Breadcrumb>())
188+
verify(fixture.hub, never()).addBreadcrumb(any<Breadcrumb>(), anyOrNull())
189189
}
190190

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

215215
sut.onSingleTapUp(event)
216216

217-
verify(fixture.hub, never()).addBreadcrumb(any<Breadcrumb>())
217+
verify(fixture.hub, 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
@@ -39,7 +39,7 @@ public ComposeGestureTargetLocator(final @NotNull ILogger logger) {
3939

4040
@Override
4141
public @Nullable UiElement locate(
42-
@NotNull Object root, float x, float y, UiElement.Type targetType) {
42+
@Nullable Object root, float x, float y, UiElement.Type targetType) {
4343

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

‎sentry/api/sentry.api‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3645,6 +3645,7 @@ public final class io/sentry/instrumentation/file/SentryFileOutputStream : java/
36453645
public final class io/sentry/instrumentation/file/SentryFileOutputStream$Factory {
36463646
public fun <init> ()V
36473647
public static fun create (Ljava/io/FileOutputStream;Ljava/io/File;)Ljava/io/FileOutputStream;
3648+
public static fun create (Ljava/io/FileOutputStream;Ljava/io/File;Lio/sentry/IHub;)Ljava/io/FileOutputStream;
36483649
public static fun create (Ljava/io/FileOutputStream;Ljava/io/File;Z)Ljava/io/FileOutputStream;
36493650
public static fun create (Ljava/io/FileOutputStream;Ljava/io/FileDescriptor;)Ljava/io/FileOutputStream;
36503651
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.HubAdapter;
44
import io.sentry.IHub;
55
import io.sentry.ISpan;
6+
import io.sentry.SentryOptions;
67
import java.io.File;
78
import java.io.FileDescriptor;
89
import java.io.FileInputStream;
@@ -127,26 +128,40 @@ 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, HubAdapter.getInstance()));
131+
final @NotNull IHub hub = HubAdapter.getInstance();
132+
return isTracingEnabled(hub)
133+
? new SentryFileInputStream(init(name != null ? new File(name) : null, delegate, hub))
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, HubAdapter.getInstance()));
140+
final @NotNull IHub hub = HubAdapter.getInstance();
141+
return isTracingEnabled(hub)
142+
? new SentryFileInputStream(init(file, delegate, hub))
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, HubAdapter.getInstance()), descriptor);
148+
final @NotNull IHub hub = HubAdapter.getInstance();
149+
return isTracingEnabled(hub)
150+
? new SentryFileInputStream(init(descriptor, delegate, hub), descriptor)
151+
: delegate;
144152
}
145153

146154
static FileInputStream create(
147155
final @NotNull FileInputStream delegate, final @Nullable File file, final @NotNull IHub hub)
148156
throws FileNotFoundException {
149-
return new SentryFileInputStream(init(file, delegate, hub));
157+
return isTracingEnabled(hub)
158+
? new SentryFileInputStream(init(file, delegate, hub))
159+
: delegate;
160+
}
161+
162+
private static boolean isTracingEnabled(final @NotNull IHub hub) {
163+
final @NotNull SentryOptions options = hub.getOptions();
164+
return options.isTracingEnabled();
150165
}
151166
}
152167
}

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

Lines changed: 38 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
import io.sentry.HubAdapter;
44
import io.sentry.IHub;
55
import io.sentry.ISpan;
6+
import io.sentry.SentryOptions;
67
import java.io.File;
78
import java.io.FileDescriptor;
89
import java.io.FileNotFoundException;
@@ -131,32 +132,62 @@ public static final class Factory {
131132
public static FileOutputStream create(
132133
final @NotNull FileOutputStream delegate, final @Nullable String name)
133134
throws FileNotFoundException {
134-
return new SentryFileOutputStream(
135-
init(name != null ? new File(name) : null, false, delegate, HubAdapter.getInstance()));
135+
final @NotNull IHub hub = HubAdapter.getInstance();
136+
return isTracingEnabled(hub)
137+
? new SentryFileOutputStream(
138+
init(name != null ? new File(name) : null, false, delegate, hub))
139+
: delegate;
136140
}
137141

138142
public static FileOutputStream create(
139143
final @NotNull FileOutputStream delegate, final @Nullable String name, final boolean append)
140144
throws FileNotFoundException {
141-
return new SentryFileOutputStream(
142-
init(name != null ? new File(name) : null, append, delegate, HubAdapter.getInstance()));
145+
final @NotNull IHub hub = HubAdapter.getInstance();
146+
return isTracingEnabled(hub)
147+
? new SentryFileOutputStream(
148+
init(name != null ? new File(name) : null, append, delegate, hub))
149+
: delegate;
143150
}
144151

145152
public static FileOutputStream create(
146153
final @NotNull FileOutputStream delegate, final @Nullable File file)
147154
throws FileNotFoundException {
148-
return new SentryFileOutputStream(init(file, false, delegate, HubAdapter.getInstance()));
155+
final @NotNull IHub hub = HubAdapter.getInstance();
156+
return isTracingEnabled(hub)
157+
? new SentryFileOutputStream(init(file, false, delegate, hub))
158+
: delegate;
149159
}
150160

151161
public static FileOutputStream create(
152162
final @NotNull FileOutputStream delegate, final @Nullable File file, final boolean append)
153163
throws FileNotFoundException {
154-
return new SentryFileOutputStream(init(file, append, delegate, HubAdapter.getInstance()));
164+
final @NotNull IHub hub = HubAdapter.getInstance();
165+
return isTracingEnabled(hub)
166+
? new SentryFileOutputStream(init(file, append, delegate, hub))
167+
: delegate;
155168
}
156169

157170
public static FileOutputStream create(
158171
final @NotNull FileOutputStream delegate, final @NotNull FileDescriptor fdObj) {
159-
return new SentryFileOutputStream(init(fdObj, delegate, HubAdapter.getInstance()), fdObj);
172+
final @NotNull IHub hub = HubAdapter.getInstance();
173+
return isTracingEnabled(hub)
174+
? new SentryFileOutputStream(init(fdObj, delegate, hub), fdObj)
175+
: delegate;
176+
}
177+
178+
public static FileOutputStream create(
179+
final @NotNull FileOutputStream delegate,
180+
final @Nullable File file,
181+
final @NotNull IHub hub)
182+
throws FileNotFoundException {
183+
return isTracingEnabled(hub)
184+
? new SentryFileOutputStream(init(file, false, delegate, hub))
185+
: delegate;
186+
}
187+
188+
private static boolean isTracingEnabled(final @NotNull IHub hub) {
189+
final @NotNull SentryOptions options = hub.getOptions();
190+
return options.isTracingEnabled();
160191
}
161192
}
162193
}

0 commit comments

Comments
 (0)