Skip to content

[QA] Fix potential ANRs due to default integrations #3778

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 10 commits into from
Oct 9, 2024
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
- If you're using code obfuscation, adjust your proguard-rules accordingly, so your custom view class name is not minified
- Fix ensure Application Context is used even when SDK is initialized via Activity Context ([#3669](https://github.com/getsentry/sentry-java/pull/3669))
- Fix potential ANRs due to `Calendar.getInstance` usage in Breadcrumbs constructor ([#3736](https://github.com/getsentry/sentry-java/pull/3736))
- Fix potential ANRs due to default integrations ([#3778](https://github.com/getsentry/sentry-java/pull/3778))
- Lazily initialize heavy `SentryOptions` members to avoid ANRs on app start ([#3749](https://github.com/getsentry/sentry-java/pull/3749))

*Breaking changes*:
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -382,7 +382,7 @@ public synchronized void onActivityCreated(

firstActivityCreated = true;

if (fullyDisplayedReporter != null) {
if (performanceEnabled && ttfdSpan != null && fullyDisplayedReporter != null) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've added this, as otherwise we'd add a new listener inside FullyDisplayedReporter without ever clearing that list.

fullyDisplayedReporter.registerFullyDrawnListener(() -> onFullFrameDrawn(ttfdSpan));
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -85,43 +85,25 @@ public void close() throws IOException {
@SuppressWarnings("deprecation")
@Override
public void onConfigurationChanged(@NotNull Configuration newConfig) {
if (hub != null) {
final Device.DeviceOrientation deviceOrientation =
DeviceOrientations.getOrientation(context.getResources().getConfiguration().orientation);

String orientation;
if (deviceOrientation != null) {
orientation = deviceOrientation.name().toLowerCase(Locale.ROOT);
} else {
orientation = "undefined";
}

final Breadcrumb breadcrumb = new Breadcrumb();
breadcrumb.setType("navigation");
breadcrumb.setCategory("device.orientation");
breadcrumb.setData("position", orientation);
breadcrumb.setLevel(SentryLevel.INFO);

final Hint hint = new Hint();
hint.set(ANDROID_CONFIGURATION, newConfig);

hub.addBreadcrumb(breadcrumb, hint);
}
final long now = System.currentTimeMillis();
executeInBackground(() -> captureConfigurationChangedBreadcrumb(now, newConfig));
}

@Override
public void onLowMemory() {
createLowMemoryBreadcrumb(null);
final long now = System.currentTimeMillis();
executeInBackground(() -> captureLowMemoryBreadcrumb(now, null));
}

@Override
public void onTrimMemory(final int level) {
createLowMemoryBreadcrumb(level);
final long now = System.currentTimeMillis();
executeInBackground(() -> captureLowMemoryBreadcrumb(now, level));
}

private void createLowMemoryBreadcrumb(final @Nullable Integer level) {
private void captureLowMemoryBreadcrumb(final long timeMs, final @Nullable Integer level) {
if (hub != null) {
final Breadcrumb breadcrumb = new Breadcrumb();
final Breadcrumb breadcrumb = new Breadcrumb(timeMs);
if (level != null) {
// only add breadcrumb if TRIM_MEMORY_BACKGROUND, TRIM_MEMORY_MODERATE or
// TRIM_MEMORY_COMPLETE.
Expand All @@ -147,4 +129,42 @@ private void createLowMemoryBreadcrumb(final @Nullable Integer level) {
hub.addBreadcrumb(breadcrumb);
}
}

private void captureConfigurationChangedBreadcrumb(
final long timeMs, final @NotNull Configuration newConfig) {
if (hub != null) {
final Device.DeviceOrientation deviceOrientation =
DeviceOrientations.getOrientation(context.getResources().getConfiguration().orientation);

String orientation;
if (deviceOrientation != null) {
orientation = deviceOrientation.name().toLowerCase(Locale.ROOT);
} else {
orientation = "undefined";
}

final Breadcrumb breadcrumb = new Breadcrumb(timeMs);
breadcrumb.setType("navigation");
breadcrumb.setCategory("device.orientation");
breadcrumb.setData("position", orientation);
breadcrumb.setLevel(SentryLevel.INFO);

final Hint hint = new Hint();
hint.set(ANDROID_CONFIGURATION, newConfig);

hub.addBreadcrumb(breadcrumb, hint);
}
}

private void executeInBackground(final @NotNull Runnable runnable) {
if (options != null) {
try {
options.getExecutorService().submit(runnable);
} catch (Throwable t) {
options
.getLogger()
.log(SentryLevel.ERROR, t, "Failed to submit app components breadcrumb task");
}
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@
import io.sentry.IHub;
import io.sentry.SentryLevel;
import io.sentry.Session;
import io.sentry.android.core.internal.util.BreadcrumbFactory;
import io.sentry.transport.CurrentDateProvider;
import io.sentry.transport.ICurrentDateProvider;
import java.util.Timer;
Expand Down Expand Up @@ -90,7 +89,6 @@ private void startSession() {
if (lastUpdatedSession == 0L
|| (lastUpdatedSession + sessionIntervalMillis) <= currentTimeMillis) {
if (enableSessionTracking) {
addSessionBreadcrumb("start");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯

hub.startSession();
}
hub.getOptions().getReplayController().start();
Expand Down Expand Up @@ -125,7 +123,6 @@ private void scheduleEndSession() {
@Override
public void run() {
if (enableSessionTracking) {
addSessionBreadcrumb("end");
hub.endSession();
}
hub.getOptions().getReplayController().stop();
Expand Down Expand Up @@ -157,11 +154,6 @@ private void addAppBreadcrumb(final @NotNull String state) {
}
}

private void addSessionBreadcrumb(final @NotNull String state) {
final Breadcrumb breadcrumb = BreadcrumbFactory.forSession(state);
hub.addBreadcrumb(breadcrumb);
}

@TestOnly
@Nullable
TimerTask getTimerTask() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,13 @@
public final class NetworkBreadcrumbsIntegration implements Integration, Closeable {

private final @NotNull Context context;

private final @NotNull BuildInfoProvider buildInfoProvider;

private final @NotNull ILogger logger;
private final @NotNull Object lock = new Object();
private volatile boolean isClosed;
private @Nullable SentryOptions options;

@TestOnly @Nullable NetworkBreadcrumbsNetworkCallback networkCallback;
@TestOnly @Nullable volatile NetworkBreadcrumbsNetworkCallback networkCallback;

public NetworkBreadcrumbsIntegration(
final @NotNull Context context,
Expand All @@ -63,40 +64,74 @@ public void register(final @NotNull IHub hub, final @NotNull SentryOptions optio
"NetworkBreadcrumbsIntegration enabled: %s",
androidOptions.isEnableNetworkEventBreadcrumbs());

this.options = options;

if (androidOptions.isEnableNetworkEventBreadcrumbs()) {

// The specific error is logged in the ConnectivityChecker method
if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.LOLLIPOP) {
networkCallback = null;
logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration requires Android 5+");
if (buildInfoProvider.getSdkInfoVersion() < Build.VERSION_CODES.N) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed this to N, as our AndroidConnectionStatusProvider.registerNetworkCallback itself only supports >= N

logger.log(SentryLevel.DEBUG, "NetworkCallbacks need Android N+.");
return;
}

networkCallback =
new NetworkBreadcrumbsNetworkCallback(hub, buildInfoProvider, options.getDateProvider());
final boolean registered =
AndroidConnectionStatusProvider.registerNetworkCallback(
context, logger, buildInfoProvider, networkCallback);
try {
options
.getExecutorService()
.submit(
new Runnable() {
@Override
public void run() {
// in case integration is closed before the task is executed, simply return
if (isClosed) {
return;
}

// The specific error is logged in the ConnectivityChecker method
if (!registered) {
networkCallback = null;
logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration not installed.");
return;
synchronized (lock) {
networkCallback =
new NetworkBreadcrumbsNetworkCallback(
hub, buildInfoProvider, options.getDateProvider());

final boolean registered =
AndroidConnectionStatusProvider.registerNetworkCallback(
context, logger, buildInfoProvider, networkCallback);
if (registered) {
logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration installed.");
addIntegrationToSdkVersion(getClass());
} else {
logger.log(
SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration not installed.");
// The specific error is logged by AndroidConnectionStatusProvider
}
}
}
});
} catch (Throwable t) {
logger.log(SentryLevel.ERROR, "Error submitting NetworkBreadcrumbsIntegration task.", t);
}
logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration installed.");
addIntegrationToSdkVersion(getClass());
}
}

@Override
public void close() throws IOException {
if (networkCallback != null) {
AndroidConnectionStatusProvider.unregisterNetworkCallback(
context, logger, buildInfoProvider, networkCallback);
logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration remove.");
isClosed = true;

try {
Objects.requireNonNull(options, "Options is required")
.getExecutorService()
.submit(
() -> {
synchronized (lock) {
if (networkCallback != null) {
AndroidConnectionStatusProvider.unregisterNetworkCallback(
context, logger, buildInfoProvider, networkCallback);
logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration removed.");
}
networkCallback = null;
}
});
} catch (Throwable t) {
logger.log(SentryLevel.ERROR, "Error submitting NetworkBreadcrumbsIntegration task.", t);
}
networkCallback = null;
}

@SuppressLint("ObsoleteSdkInt")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@
import io.sentry.SentryLevel;
import io.sentry.SentryOptions;
import io.sentry.Session;
import io.sentry.android.core.internal.util.BreadcrumbFactory;
import io.sentry.android.core.performance.AppStartMetrics;
import io.sentry.android.core.performance.TimeSpan;
import io.sentry.android.fragment.FragmentLifecycleIntegration;
Expand Down Expand Up @@ -173,7 +172,6 @@ public static synchronized void init(
}
});
if (!sessionStarted.get()) {
hub.addBreadcrumb(BreadcrumbFactory.forSession("session.start"));
hub.startSession();
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,7 @@ static final class SystemEventsBroadcastReceiver extends BroadcastReceiver {
private static final long DEBOUNCE_WAIT_TIME_MS = 60 * 1000;
private final @NotNull IHub hub;
private final @NotNull SentryAndroidOptions options;
private final @NotNull Debouncer debouncer =
private final @NotNull Debouncer batteryChangedDebouncer =
new Debouncer(AndroidCurrentDateProvider.getInstance(), DEBOUNCE_WAIT_TIME_MS, 0);

SystemEventsBroadcastReceiver(
Expand All @@ -221,19 +221,43 @@ static final class SystemEventsBroadcastReceiver extends BroadcastReceiver {
}

@Override
public void onReceive(Context context, Intent intent) {
final boolean shouldDebounce = debouncer.checkForDebounce();
final String action = intent.getAction();
public void onReceive(final Context context, final @NotNull Intent intent) {
final @Nullable String action = intent.getAction();
final boolean isBatteryChanged = ACTION_BATTERY_CHANGED.equals(action);
if (isBatteryChanged && shouldDebounce) {
// aligning with iOS which only captures battery status changes every minute at maximum

// aligning with iOS which only captures battery status changes every minute at maximum
if (isBatteryChanged && batteryChangedDebouncer.checkForDebounce()) {
return;
}

final Breadcrumb breadcrumb = new Breadcrumb();
final long now = System.currentTimeMillis();
try {
options
.getExecutorService()
.submit(
() -> {
final Breadcrumb breadcrumb =
createBreadcrumb(now, intent, action, isBatteryChanged);
final Hint hint = new Hint();
hint.set(ANDROID_INTENT, intent);
hub.addBreadcrumb(breadcrumb, hint);
});
} catch (Throwable t) {
options
.getLogger()
.log(SentryLevel.ERROR, t, "Failed to submit system event breadcrumb action.");
}
}

private @NotNull Breadcrumb createBreadcrumb(
final long timeMs,
final @NotNull Intent intent,
final @Nullable String action,
boolean isBatteryChanged) {
final Breadcrumb breadcrumb = new Breadcrumb(timeMs);
breadcrumb.setType("system");
breadcrumb.setCategory("device.event");
String shortAction = StringUtils.getStringAfterDot(action);
final String shortAction = StringUtils.getStringAfterDot(action);
if (shortAction != null) {
breadcrumb.setData("action", shortAction);
}
Expand Down Expand Up @@ -273,11 +297,7 @@ public void onReceive(Context context, Intent intent) {
}
}
breadcrumb.setLevel(SentryLevel.INFO);

final Hint hint = new Hint();
hint.set(ANDROID_INTENT, intent);

hub.addBreadcrumb(breadcrumb, hint);
return breadcrumb;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,6 @@ class ActivityLifecycleIntegrationTest {
}
val bundle = mock<Bundle>()
val activityFramesTracker = mock<ActivityFramesTracker>()
val fullyDisplayedReporter = FullyDisplayedReporter.getInstance()
val transactionFinishedCallback = mock<TransactionFinishedCallback>()
lateinit var shadowActivityManager: ShadowActivityManager

Expand Down Expand Up @@ -619,11 +618,30 @@ class ActivityLifecycleIntegrationTest {
sut.onActivityCreated(activity, mock())
val ttfdSpan = sut.ttfdSpanMap[activity]
sut.ttidSpanMap.values.first().finish()
fixture.fullyDisplayedReporter.reportFullyDrawn()
fixture.options.fullyDisplayedReporter.reportFullyDrawn()
assertTrue(ttfdSpan!!.isFinished)
assertNotEquals(SpanStatus.CANCELLED, ttfdSpan.status)
}

@Test
fun `if ttfd is disabled, no listener is registered for FullyDisplayedReporter`() {
val ttfdReporter = mock<FullyDisplayedReporter>()

val sut = fixture.getSut()
fixture.options.apply {
tracesSampleRate = 1.0
isEnableTimeToFullDisplayTracing = false
fullyDisplayedReporter = ttfdReporter
}

sut.register(fixture.hub, fixture.options)

val activity = mock<Activity>()
sut.onActivityCreated(activity, mock())

verify(ttfdReporter, never()).registerFullyDrawnListener(any())
}

@Test
fun `App start is Cold when savedInstanceState is null`() {
val sut = fixture.getSut()
Expand Down
Loading
Loading