Skip to content

Commit

Permalink
Fix iOS "Arithmetic Overflow" (#1874)
Browse files Browse the repository at this point in the history
* guard against arithmetic overflow

* Fix issue where transaction was finished multiple times

* add changelog entry

* update test expectations

* test that didPop does not call finsh transaction multiple times
  • Loading branch information
denrase authored Feb 14, 2024
1 parent 6f57f15 commit 2966d88
Show file tree
Hide file tree
Showing 4 changed files with 34 additions and 9 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@

- Remove Flutter dependency from Drift integration ([#1867](https://github.com/getsentry/sentry-dart/pull/1867))
- Remove dead code, cold start bool is now always present ([#1861](https://github.com/getsentry/sentry-dart/pull/1861))
- Fix iOS "Arithmetic Overflow" ([#1874](https://github.com/getsentry/sentry-dart/pull/1874))

### Dependencies

Expand Down
7 changes: 3 additions & 4 deletions flutter/ios/Classes/SentryFlutterPluginApple.swift
Original file line number Diff line number Diff line change
Expand Up @@ -430,10 +430,9 @@ public class SentryFlutterPluginApple: NSObject, FlutterPlugin {
}

let currentFrames = PrivateSentrySDKOnly.currentScreenFrames

let total = currentFrames.total - totalFrames
let frozen = currentFrames.frozen - frozenFrames
let slow = currentFrames.slow - slowFrames
let total = max(Int(currentFrames.total) - Int(totalFrames), 0)
let frozen = max(Int(currentFrames.frozen) - Int(frozenFrames), 0)
let slow = max(Int(currentFrames.slow) - Int(slowFrames), 0)

if total <= 0 && frozen <= 0 && slow <= 0 {
result(nil)
Expand Down
10 changes: 8 additions & 2 deletions flutter/lib/src/navigation/sentry_navigator_observer.dart
Original file line number Diff line number Diff line change
Expand Up @@ -207,6 +207,7 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> {
autoFinishAfter: _autoFinishAfter,
trimEnd: true,
onFinish: (transaction) async {
_transaction = null;
final nativeFrames = await _native
?.endNativeFramesCollection(transaction.context.traceId);
if (nativeFrames != null) {
Expand Down Expand Up @@ -241,8 +242,13 @@ class SentryNavigatorObserver extends RouteObserver<PageRoute<dynamic>> {
}

Future<void> _finishTransaction() async {
_transaction?.status ??= SpanStatus.ok();
await _transaction?.finish();
final transaction = _transaction;
_transaction = null;
if (transaction == null || transaction.finished) {
return;
}
transaction.status ??= SpanStatus.ok();
await transaction.finish();
}
}

Expand Down
25 changes: 22 additions & 3 deletions flutter/test/sentry_navigator_observer_test.dart
Original file line number Diff line number Diff line change
Expand Up @@ -261,7 +261,7 @@ void main() {
final secondRoute = route(RouteSettings(name: 'Second Route'));

final hub = _MockHub();
final span = getMockSentryTracer();
final span = getMockSentryTracer(finished: false);
when(span.context).thenReturn(SentrySpanContext(operation: 'op'));
when(span.status).thenReturn(null);
_whenAnyStart(hub, span);
Expand All @@ -279,7 +279,7 @@ void main() {
final currentRoute = route(RouteSettings(name: 'Current Route'));

final hub = _MockHub();
final span = getMockSentryTracer();
final span = getMockSentryTracer(finished: false);
when(span.context).thenReturn(SentrySpanContext(operation: 'op'));
when(span.status).thenReturn(null);
_whenAnyStart(hub, span);
Expand All @@ -293,6 +293,24 @@ void main() {
verify(span.finish());
});

test('multiple didPop only finish transaction once', () {
final currentRoute = route(RouteSettings(name: 'Current Route'));

final hub = _MockHub();
final span = getMockSentryTracer(finished: false);
when(span.context).thenReturn(SentrySpanContext(operation: 'op'));
when(span.status).thenReturn(null);
_whenAnyStart(hub, span);

final sut = fixture.getSut(hub: hub);

sut.didPush(currentRoute, null);
sut.didPop(currentRoute, null);
sut.didPop(currentRoute, null);

verify(span.finish()).called(1);
});

test('didPop re-starts previous', () {
final previousRoute = route(RouteSettings(name: 'Previous Route'));
final currentRoute = route(RouteSettings(name: 'Current Route'));
Expand Down Expand Up @@ -833,9 +851,10 @@ class _MockHub extends MockHub {
}
}

ISentrySpan getMockSentryTracer({String? name}) {
ISentrySpan getMockSentryTracer({String? name, bool? finished}) {
final tracer = MockSentryTracer();
when(tracer.name).thenReturn(name ?? 'name');
when(tracer.finished).thenReturn(finished ?? true);
return tracer;
}

Expand Down

0 comments on commit 2966d88

Please sign in to comment.