Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

### Fixes

- Avoid StrictMode warnings (followup) ([#4809](https://github.com/getsentry/sentry-java/pull/4809))
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if it's worth mentioning that as a separate item, maybe combine it with the line below? Not sure if the danger check likes that though 😅

- Avoid StrictMode warnings ([#4724](https://github.com/getsentry/sentry-java/pull/4724))
- Use logger from options for JVM profiler ([#4771](https://github.com/getsentry/sentry-java/pull/4771))
- Session Replay: Avoid deadlock when pausing replay if no connection ([#4788](https://github.com/getsentry/sentry-java/pull/4788))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,8 @@ private static void readDefaultOptionValues(

if (options.getDistinctId() == null) {
try {
options.setDistinctId(Installation.id(context));
options.setDistinctId(
options.getRuntimeManager().runWithRelaxedPolicy(() -> Installation.id(context)));
} catch (RuntimeException e) {
options.getLogger().log(SentryLevel.ERROR, "Could not generate distinct Id.", e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -607,7 +607,7 @@ private void mergeUser(final @NotNull SentryBaseEvent event) {

private @Nullable String getDeviceId() {
try {
return Installation.id(context);
return options.getRuntimeManager().runWithRelaxedPolicy(() -> Installation.id(context));
} catch (Throwable e) {
options.getLogger().log(SentryLevel.ERROR, "Error getting installationId.", e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,7 @@ private void mergeUser(final @NotNull SentryBaseEvent event) {

// userId should be set even if event is Cached as the userId is static and won't change anyway.
if (user.getId() == null) {
user.setId(Installation.id(context));
user.setId(options.getRuntimeManager().runWithRelaxedPolicy(() -> Installation.id(context)));
}
if (user.getIpAddress() == null && options.isSendDefaultPii()) {
user.setIpAddress(IpAddressUtils.DEFAULT_IP_ADDRESS);
Expand Down Expand Up @@ -336,7 +336,7 @@ private void setAppExtras(final @NotNull App app, final @NotNull Hint hint) {
*/
public @NotNull User getDefaultUser(final @NotNull Context context) {
final @NotNull User user = new User();
user.setId(Installation.id(context));
user.setId(options.getRuntimeManager().runWithRelaxedPolicy(() -> Installation.id(context)));
return user;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -227,18 +227,24 @@ private void setDeviceIO(final @NotNull Device device, final boolean includeDyna
// this way of getting the size of storage might be problematic for storages bigger than 2GB
// check the use of
// https://developer.android.com/reference/java/io/File.html#getFreeSpace%28%29
final @Nullable File internalStorageFile = context.getExternalFilesDir(null);
if (internalStorageFile != null) {
StatFs internalStorageStat = new StatFs(internalStorageFile.getPath());
device.setStorageSize(getTotalInternalStorage(internalStorageStat));
device.setFreeStorage(getUnusedInternalStorage(internalStorageStat));
}

final @Nullable StatFs externalStorageStat = getExternalStorageStat(internalStorageFile);
if (externalStorageStat != null) {
device.setExternalStorageSize(getTotalExternalStorage(externalStorageStat));
device.setExternalFreeStorage(getUnusedExternalStorage(externalStorageStat));
}
options
Copy link
Member

Choose a reason for hiding this comment

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

We do this to have a base of 0 strictmode warnings, right? I'm just wondering if we should keep this as-is, as the collection of these attributes can be turned off via settings anyway.

.getRuntimeManager()
.runWithRelaxedPolicy(
() -> {
final @Nullable File internalStorageFile = context.getExternalFilesDir(null);
if (internalStorageFile != null) {
StatFs internalStorageStat = new StatFs(internalStorageFile.getPath());
device.setStorageSize(getTotalInternalStorage(internalStorageStat));
device.setFreeStorage(getUnusedInternalStorage(internalStorageStat));
}

final @Nullable StatFs externalStorageStat =
getExternalStorageStat(internalStorageFile);
if (externalStorageStat != null) {
device.setExternalStorageSize(getTotalExternalStorage(externalStorageStat));
device.setExternalFreeStorage(getUnusedExternalStorage(externalStorageStat));
}
});

if (device.getConnectionType() == null) {
// wifi, ethernet or cellular, null if none
Expand Down Expand Up @@ -479,7 +485,7 @@ private Long getUnusedExternalStorage(final @NotNull StatFs stat) {
@Nullable
private String getDeviceId() {
try {
return Installation.id(context);
return options.getRuntimeManager().runWithRelaxedPolicy(() -> Installation.id(context));
} catch (Throwable e) {
options.getLogger().log(SentryLevel.ERROR, "Error getting installationId.", e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,8 @@ public static Map<String, Object> serializeScope(
}
if (user.getId() == null) {
try {
user.setId(Installation.id(context));
user.setId(
options.getRuntimeManager().runWithRelaxedPolicy(() -> Installation.id(context)));
} catch (RuntimeException e) {
logger.log(SentryLevel.ERROR, "Could not retrieve installation ID", e);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,9 +131,10 @@ public static boolean hasStartupCrashMarker(final @NotNull SentryOptions options

final File crashMarkerFile = new File(outboxPath, STARTUP_CRASH_MARKER_FILE);
try {
final boolean exists = crashMarkerFile.exists();
final boolean exists =
options.getRuntimeManager().runWithRelaxedPolicy(() -> crashMarkerFile.exists());
if (exists) {
if (!crashMarkerFile.delete()) {
if (!options.getRuntimeManager().runWithRelaxedPolicy(() -> crashMarkerFile.delete())) {
options
.getLogger()
.log(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,13 @@ public <T> T runWithRelaxedPolicy(final @NotNull IRuntimeManagerCallback<T> toRu
StrictMode.setVmPolicy(oldVmPolicy);
}
}

@Override
public void runWithRelaxedPolicy(final @NotNull Runnable toRun) {
runWithRelaxedPolicy(
() -> {
toRun.run();
return null;
});
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ class AndroidRuntimeManagerTest {

// Run the function and assert LAX policies
called =
sut.runWithRelaxedPolicy {
sut.runWithRelaxedPolicy<Boolean> {
assertEquals(
StrictMode.ThreadPolicy.LAX.toString(),
StrictMode.getThreadPolicy().toString(),
Expand All @@ -64,6 +64,37 @@ class AndroidRuntimeManagerTest {
StrictMode.setThreadPolicy(threadPolicy)
StrictMode.setVmPolicy(vmPolicy)

// Run the function and assert LAX policies
try {
sut.runWithRelaxedPolicy<Unit> {
assertEquals(
StrictMode.ThreadPolicy.LAX.toString(),
StrictMode.getThreadPolicy().toString(),
)
assertEquals(StrictMode.VmPolicy.LAX.toString(), StrictMode.getVmPolicy().toString())
called = true
throw Exception("Test exception")
}
} catch (_: Exception) {}
Copy link
Member

Choose a reason for hiding this comment

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

Maybe extend the test to assert that the exception is thrown, something like this:

Suggested change
} catch (_: Exception) {}
} catch (_: Exception) {
exceptionThrown = true
}


// Policies should be reverted back
assertEquals(threadPolicy.toString(), StrictMode.getThreadPolicy().toString())
assertEquals(vmPolicy.toString(), StrictMode.getVmPolicy().toString())

// Ensure the code ran
assertTrue(called)
}

@Test
fun `runWithRelaxedPolicy with Runnable changes policy when running and restores it afterwards even if the code throws`() {
var called = false
val threadPolicy = StrictMode.ThreadPolicy.Builder().detectAll().penaltyDeath().build()
val vmPolicy = StrictMode.VmPolicy.Builder().detectAll().penaltyDeath().build()

// Set and assert the StrictMode policies
StrictMode.setThreadPolicy(threadPolicy)
StrictMode.setVmPolicy(vmPolicy)

// Run the function and assert LAX policies
try {
sut.runWithRelaxedPolicy {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,8 +157,7 @@ internal fun SentryEnvelope.describeForTest(): String {
val deserialized =
JsonSerializer(SentryOptions())
.deserialize(item.data.inputStream().reader(), SentryEvent::class.java)!!
descr +=
"Event (${deserialized.eventId}) - message: ${deserialized.message!!.formatted} -- "
descr += "Event (${deserialized.eventId}) - message: ${deserialized.message?.formatted} -- "
}
SentryItemType.Transaction -> {
val deserialized =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import io.sentry.android.core.AndroidLogger
import io.sentry.android.core.CurrentActivityHolder
import io.sentry.android.core.NdkIntegration
import io.sentry.android.core.SentryAndroidOptions
import io.sentry.assertEnvelopeEvent
import io.sentry.assertEnvelopeTransaction
import io.sentry.protocol.SentryTransaction
import java.util.concurrent.CountDownLatch
Expand Down Expand Up @@ -256,10 +257,49 @@ class SdkInitTests : BaseUiTest() {
@Test
fun initNotThrowStrictMode() {
StrictMode.setThreadPolicy(StrictMode.ThreadPolicy.Builder().detectAll().penaltyDeath().build())
StrictMode.setVmPolicy(StrictMode.VmPolicy.Builder().detectAll().penaltyDeath().build())
StrictMode.setVmPolicy(
StrictMode.VmPolicy.Builder()
.detectActivityLeaks()
// .detectCleartextNetwork() <- mockWebServer is on http, not https
.detectContentUriWithoutPermission()
.detectCredentialProtectedWhileLocked()
.detectFileUriExposure()
.detectImplicitDirectBoot()
.detectIncorrectContextUse()
.detectLeakedRegistrationObjects()
.detectLeakedSqlLiteObjects()
// .detectNonSdkApiUsage() <- thrown by leakCanary
// .detectUnsafeIntentLaunch() <- fails CI with java.lang.NoSuchMethodError
// .detectUntaggedSockets() <- thrown by mockWebServer
.penaltyDeath()
.build()
)
initSentry(true) { it.tracesSampleRate = 1.0 }
val sampleScenario = launchActivity<EmptyActivity>()
initSentry()
relayIdlingResource.increment()
relayIdlingResource.increment()
Sentry.captureException(Exception("test"))
sampleScenario.moveToState(Lifecycle.State.DESTROYED)

// Avoid interferences with other tests and assertion logic
StrictMode.setThreadPolicy(StrictMode.ThreadPolicy.LAX)
StrictMode.setVmPolicy(StrictMode.VmPolicy.LAX)

relay.assert {
findEnvelope {
assertEnvelopeEvent(it.items.toList()).exceptions!!.any { it.value == "test" }
}
.assert {
it.assertEvent()
it.assertNoOtherItems()
}
findEnvelope { assertEnvelopeTransaction(it.items.toList()).transaction == "EmptyActivity" }
.assert {
it.assertTransaction()
it.assertNoOtherItems()
}
assertNoOtherEnvelopes()
}
}

private fun assertDefaultIntegrations() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ package io.sentry.uitest.android.mockservers

import io.sentry.ProfilingTraceData
import io.sentry.SentryEnvelope
import io.sentry.SentryEvent
import io.sentry.android.core.AndroidLogger
import io.sentry.assertEnvelopeEvent
import io.sentry.assertEnvelopeItem
import io.sentry.assertEnvelopeProfile
import io.sentry.assertEnvelopeTransaction
Expand All @@ -27,6 +29,16 @@ class EnvelopeAsserter(val envelope: SentryEnvelope, val response: MockResponse)
return item
}

/**
* Asserts a transaction exists and returns the first one. It is then removed from internal list
* of unasserted items.
*/
fun assertEvent(): SentryEvent =
assertEnvelopeEvent(unassertedItems, AndroidLogger()) { index, item ->
unassertedItems.removeAt(index)
return item
}

/**
* Asserts a transaction exists and returns the first one. It is then removed from internal list
* of unasserted items.
Expand Down
2 changes: 2 additions & 0 deletions sentry-test-support/api/sentry-test-support.api
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
public final class io/sentry/AssertionsKt {
public static final fun assertEnvelopeEvent (Ljava/util/List;Lio/sentry/ILogger;Lkotlin/jvm/functions/Function2;)Lio/sentry/SentryEvent;
public static synthetic fun assertEnvelopeEvent$default (Ljava/util/List;Lio/sentry/ILogger;Lkotlin/jvm/functions/Function2;ILjava/lang/Object;)Lio/sentry/SentryEvent;
public static final fun assertEnvelopeFeedback (Ljava/util/List;Lio/sentry/ILogger;Lkotlin/jvm/functions/Function2;)Lio/sentry/SentryEvent;
public static synthetic fun assertEnvelopeFeedback$default (Ljava/util/List;Lio/sentry/ILogger;Lkotlin/jvm/functions/Function2;ILjava/lang/Object;)Lio/sentry/SentryEvent;
public static final fun assertEnvelopeProfile (Ljava/util/List;Lio/sentry/ILogger;Lkotlin/jvm/functions/Function2;)Lio/sentry/ProfilingTraceData;
Expand Down
10 changes: 10 additions & 0 deletions sentry-test-support/src/main/kotlin/io/sentry/Assertions.kt
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,16 @@ inline fun <reified T> assertEnvelopeItem(
return item.second
}

/**
* Asserts a transaction exists in [items] and returns the first one. Otherwise it throws an
* [AssertionError].
*/
inline fun assertEnvelopeEvent(
items: List<SentryEnvelopeItem>,
logger: ILogger = NoOpLogger.getInstance(),
predicate: (index: Int, item: SentryEvent) -> Unit = { _, _ -> },
): SentryEvent = assertEnvelopeItem(items, SentryItemType.Event, logger, predicate)

/**
* Asserts a transaction exists in [items] and returns the first one. Otherwise it throws an
* [AssertionError].
Expand Down
2 changes: 2 additions & 0 deletions sentry/api/sentry.api
Original file line number Diff line number Diff line change
Expand Up @@ -7297,6 +7297,7 @@ public final class io/sentry/util/UrlUtils$UrlDetails {

public abstract interface class io/sentry/util/runtime/IRuntimeManager {
public abstract fun runWithRelaxedPolicy (Lio/sentry/util/runtime/IRuntimeManager$IRuntimeManagerCallback;)Ljava/lang/Object;
public abstract fun runWithRelaxedPolicy (Ljava/lang/Runnable;)V
}

public abstract interface class io/sentry/util/runtime/IRuntimeManager$IRuntimeManagerCallback {
Expand All @@ -7306,6 +7307,7 @@ public abstract interface class io/sentry/util/runtime/IRuntimeManager$IRuntimeM
public final class io/sentry/util/runtime/NeutralRuntimeManager : io/sentry/util/runtime/IRuntimeManager {
public fun <init> ()V
public fun runWithRelaxedPolicy (Lio/sentry/util/runtime/IRuntimeManager$IRuntimeManagerCallback;)Ljava/lang/Object;
public fun runWithRelaxedPolicy (Ljava/lang/Runnable;)V
}

public abstract interface class io/sentry/util/thread/IThreadChecker {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@
public interface IRuntimeManager {
<T> T runWithRelaxedPolicy(final @NotNull IRuntimeManagerCallback<T> toRun);

void runWithRelaxedPolicy(final @NotNull Runnable toRun);

interface IRuntimeManagerCallback<T> {
T run();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,4 +9,9 @@ public final class NeutralRuntimeManager implements IRuntimeManager {
public <T> T runWithRelaxedPolicy(final @NotNull IRuntimeManagerCallback<T> toRun) {
return toRun.run();
}

@Override
public void runWithRelaxedPolicy(final @NotNull Runnable toRun) {
toRun.run();
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a test to ensure if an exception is thrown it's properly propagated and not silently swallowed?

Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,17 @@ class NeutralRuntimeManagerTest {
fun `runWithRelaxedPolicy runs the code`() {
var called = false

called = sut.runWithRelaxedPolicy { true }
called = sut.runWithRelaxedPolicy<Boolean> { true }

// Ensure the code ran
assertTrue(called)
}

@Test
fun `runWithRelaxedPolicy with runnable runs the code`() {
var called = false

sut.runWithRelaxedPolicy { called = true }

// Ensure the code ran
assertTrue(called)
Expand Down
Loading