-
-
Notifications
You must be signed in to change notification settings - Fork 465
[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
Changes from 9 commits
d7e2ffc
fd2bc62
7561fe4
0870f09
e2cb750
4ffe903
41b11a9
40cbfea
0a924a0
e524d07
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -85,41 +85,20 @@ 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); | ||
| } | ||
| executeInBackground(() -> captureConfigurationChangedBreadcrumb(newConfig)); | ||
|
||
| } | ||
|
|
||
| @Override | ||
| public void onLowMemory() { | ||
| createLowMemoryBreadcrumb(null); | ||
| executeInBackground(() -> captureLowMemoryBreadcrumb(null)); | ||
| } | ||
|
|
||
| @Override | ||
| public void onTrimMemory(final int level) { | ||
| createLowMemoryBreadcrumb(level); | ||
| executeInBackground(() -> captureLowMemoryBreadcrumb(level)); | ||
| } | ||
|
|
||
| private void createLowMemoryBreadcrumb(final @Nullable Integer level) { | ||
| private void captureLowMemoryBreadcrumb(final @Nullable Integer level) { | ||
| if (hub != null) { | ||
| final Breadcrumb breadcrumb = new Breadcrumb(); | ||
| if (level != null) { | ||
|
|
@@ -147,4 +126,41 @@ private void createLowMemoryBreadcrumb(final @Nullable Integer level) { | |
| hub.addBreadcrumb(breadcrumb); | ||
| } | ||
| } | ||
|
|
||
| private void captureConfigurationChangedBreadcrumb(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(); | ||
| 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 |
|---|---|---|
|
|
@@ -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; | ||
|
|
@@ -90,7 +89,6 @@ private void startSession() { | |
| if (lastUpdatedSession == 0L | ||
| || (lastUpdatedSession + sessionIntervalMillis) <= currentTimeMillis) { | ||
| if (enableSessionTracking) { | ||
| addSessionBreadcrumb("start"); | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💯 |
||
| hub.startSession(); | ||
| } | ||
| hub.getOptions().getReplayController().start(); | ||
|
|
@@ -125,7 +123,6 @@ private void scheduleEndSession() { | |
| @Override | ||
| public void run() { | ||
| if (enableSessionTracking) { | ||
| addSessionBreadcrumb("end"); | ||
| hub.endSession(); | ||
| } | ||
| hub.getOptions().getReplayController().stop(); | ||
|
|
@@ -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() { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,12 +31,12 @@ | |
| public final class NetworkBreadcrumbsIntegration implements Integration, Closeable { | ||
|
|
||
| private final @NotNull Context context; | ||
|
|
||
| private final @NotNull BuildInfoProvider buildInfoProvider; | ||
|
|
||
| private final @NotNull ILogger logger; | ||
| private volatile boolean isClosed; | ||
| private @Nullable SentryOptions options; | ||
|
|
||
| @TestOnly @Nullable NetworkBreadcrumbsNetworkCallback networkCallback; | ||
| @TestOnly @Nullable volatile NetworkBreadcrumbsNetworkCallback networkCallback; | ||
|
|
||
| public NetworkBreadcrumbsIntegration( | ||
| final @NotNull Context context, | ||
|
|
@@ -63,40 +63,71 @@ 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) { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I've changed this to |
||
| 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); | ||
|
|
||
| // The specific error is logged in the ConnectivityChecker method | ||
| if (!registered) { | ||
| networkCallback = null; | ||
| logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration not installed."); | ||
| return; | ||
| try { | ||
| options | ||
| .getExecutorService() | ||
| .submit( | ||
| new Runnable() { | ||
| @Override | ||
| public void run() { | ||
| // in case integration is closed before the task is executed, simply return | ||
| final @Nullable NetworkBreadcrumbsNetworkCallback callback = networkCallback; | ||
| if (isClosed || callback == null) { | ||
|
||
| networkCallback = null; | ||
| return; | ||
| } | ||
|
|
||
| final boolean registered = | ||
| AndroidConnectionStatusProvider.registerNetworkCallback( | ||
| context, logger, buildInfoProvider, callback); | ||
| 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( | ||
| () -> { | ||
| final @Nullable NetworkBreadcrumbsNetworkCallback callback = networkCallback; | ||
| if (callback != null) { | ||
| AndroidConnectionStatusProvider.unregisterNetworkCallback( | ||
| context, logger, buildInfoProvider, callback); | ||
| logger.log(SentryLevel.DEBUG, "NetworkBreadcrumbsIntegration removed."); | ||
| } | ||
| networkCallback = null; | ||
| }); | ||
| } catch (Throwable t) { | ||
| logger.log(SentryLevel.ERROR, "Error submitting NetworkBreadcrumbsIntegration task.", t); | ||
| } | ||
| networkCallback = null; | ||
| } | ||
|
|
||
| @SuppressLint("ObsoleteSdkInt") | ||
|
|
||
There was a problem hiding this comment.
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.