Skip to content

Commit ba1bfc7

Browse files
supervacuussupervacuus
committed
apply review+sync feedback
1 parent d7d5447 commit ba1bfc7

6 files changed

Lines changed: 107 additions & 12 deletions

File tree

‎.gitignore‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,5 @@
11
.DS_Store
2+
.java-version
23
.idea/
34
.gradle/
45
.run/

‎sentry-android-core/api/sentry-android-core.api‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -517,7 +517,7 @@ public final class io/sentry/android/core/TombstoneIntegration$TombstoneHint : i
517517
}
518518

519519
public class io/sentry/android/core/TombstoneIntegration$TombstonePolicy : io/sentry/android/core/ApplicationExitInfoHistoryDispatcher$ApplicationExitInfoPolicy {
520-
public fun <init> (Lio/sentry/android/core/SentryAndroidOptions;)V
520+
public fun <init> (Lio/sentry/android/core/SentryAndroidOptions;Landroid/content/Context;)V
521521
public fun buildReport (Landroid/app/ApplicationExitInfo;Z)Lio/sentry/android/core/ApplicationExitInfoHistoryDispatcher$Report;
522522
public fun getLabel ()Ljava/lang/String;
523523
public fun getLastReportedTimestamp ()Ljava/lang/Long;

‎sentry-android-core/src/main/java/io/sentry/android/core/TombstoneIntegration.java‎

Lines changed: 42 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,11 +17,16 @@
1717
import io.sentry.android.core.ApplicationExitInfoHistoryDispatcher.ApplicationExitInfoPolicy;
1818
import io.sentry.android.core.NativeEventCollector.NativeEventData;
1919
import io.sentry.android.core.cache.AndroidEnvelopeCache;
20+
import io.sentry.android.core.internal.tombstone.NativeExceptionMechanism;
2021
import io.sentry.android.core.internal.tombstone.TombstoneParser;
2122
import io.sentry.hints.Backfillable;
2223
import io.sentry.hints.BlockingFlushHint;
2324
import io.sentry.hints.NativeCrashExit;
25+
import io.sentry.protocol.DebugMeta;
26+
import io.sentry.protocol.Mechanism;
27+
import io.sentry.protocol.SentryException;
2428
import io.sentry.protocol.SentryId;
29+
import io.sentry.protocol.SentryThread;
2530
import io.sentry.transport.CurrentDateProvider;
2631
import io.sentry.transport.ICurrentDateProvider;
2732
import io.sentry.util.HintUtils;
@@ -32,6 +37,7 @@
3237
import java.nio.charset.StandardCharsets;
3338
import java.time.Instant;
3439
import java.time.format.DateTimeFormatter;
40+
import java.util.List;
3541
import org.jetbrains.annotations.ApiStatus;
3642
import org.jetbrains.annotations.NotNull;
3743
import org.jetbrains.annotations.Nullable;
@@ -85,7 +91,7 @@ public void register(@NotNull IScopes scopes, @NotNull SentryOptions options) {
8591
scopes,
8692
this.options,
8793
dateProvider,
88-
new TombstonePolicy(this.options)));
94+
new TombstonePolicy(this.options, this.context)));
8995
} catch (Throwable e) {
9096
options.getLogger().log(SentryLevel.DEBUG, "Failed to start tombstone processor.", e);
9197
}
@@ -106,10 +112,12 @@ public static class TombstonePolicy implements ApplicationExitInfoPolicy {
106112

107113
private final @NotNull SentryAndroidOptions options;
108114
private final @NotNull NativeEventCollector nativeEventCollector;
115+
@NotNull private final Context context;
109116

110-
public TombstonePolicy(final @NotNull SentryAndroidOptions options) {
117+
public TombstonePolicy(final @NotNull SentryAndroidOptions options, @NotNull Context context) {
111118
this.options = options;
112119
this.nativeEventCollector = new NativeEventCollector(options);
120+
this.context = context;
113121
}
114122

115123
@Override
@@ -151,7 +159,12 @@ public boolean shouldReportHistorical() {
151159
return null;
152160
}
153161

154-
try (final TombstoneParser parser = new TombstoneParser(tombstoneInputStream)) {
162+
try (final TombstoneParser parser =
163+
new TombstoneParser(
164+
tombstoneInputStream,
165+
this.options.getInAppIncludes(),
166+
this.options.getInAppExcludes(),
167+
this.context.getApplicationInfo().nativeLibraryDir)) {
155168
event = parser.parse();
156169
}
157170
} catch (Throwable e) {
@@ -206,11 +219,33 @@ public boolean shouldReportHistorical() {
206219
return new ApplicationExitInfoHistoryDispatcher.Report(event, hint, tombstoneHint);
207220
}
208221

209-
private SentryEvent mergeNativeCrashes(
222+
private SentryEvent mergeNativeCrashes(
210223
final @NotNull SentryEvent nativeEvent, final @NotNull SentryEvent tombstoneEvent) {
211-
nativeEvent.setExceptions(tombstoneEvent.getExceptions());
212-
nativeEvent.setDebugMeta(tombstoneEvent.getDebugMeta());
213-
nativeEvent.setThreads(tombstoneEvent.getThreads());
224+
// we take the event data verbatim from the Native SDK and only apply tombstone data where we
225+
// are sure that it will improve the outcome:
226+
// * context from the Native SDK will be closer to what users want than any backfilling
227+
// * the Native SDK only tracks the crashing thread (vs. tombstone dumps all)
228+
// * even for the crashing we expect a much better stack-trace (+ symbolication)
229+
// * tombstone adds additional exception meta-data to signal handler content
230+
// * we add debug-meta for consistency since the Native SDK caches memory maps early
231+
@Nullable List<SentryException> tombstoneExceptions = tombstoneEvent.getExceptions();
232+
@Nullable DebugMeta tombstoneDebugMeta = tombstoneEvent.getDebugMeta();
233+
@Nullable List<SentryThread> tombstoneThreads = tombstoneEvent.getThreads();
234+
if (tombstoneExceptions != null
235+
&& !tombstoneExceptions.isEmpty()
236+
&& tombstoneDebugMeta != null
237+
&& tombstoneThreads != null) {
238+
// native crashes don't nest, we always expect one level.
239+
SentryException exception = tombstoneExceptions.get(0);
240+
@Nullable Mechanism mechanism = exception.getMechanism();
241+
if (mechanism != null) {
242+
mechanism.setType(NativeExceptionMechanism.TOMBSTONE_MERGED.getValue());
243+
}
244+
nativeEvent.setExceptions(tombstoneExceptions);
245+
nativeEvent.setDebugMeta(tombstoneDebugMeta);
246+
nativeEvent.setThreads(tombstoneThreads);
247+
}
248+
214249
return nativeEvent;
215250
}
216251

‎sentry-android-core/src/main/java/io/sentry/android/core/internal/tombstone/TombstoneParser.java‎

Lines changed: 38 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -21,18 +21,30 @@
2121
import java.util.Locale;
2222
import java.util.Map;
2323
import java.util.Objects;
24+
import org.jetbrains.annotations.NotNull;
2425

2526
public class TombstoneParser implements Closeable {
2627

2728
private final InputStream tombstoneStream;
29+
@NotNull private final List<String> inAppIncludes;
30+
@NotNull private final List<String> inAppExcludes;
31+
// TODO: in theory can be null, but practically not for native crashes
32+
private final String nativeLibraryDir;
2833
private final Map<String, String> excTypeValueMap = new HashMap<>();
2934

3035
private static String formatHex(long value) {
3136
return String.format("0x%x", value);
3237
}
3338

34-
public TombstoneParser(@NonNull final InputStream tombstoneStream) {
39+
public TombstoneParser(
40+
@NonNull final InputStream tombstoneStream,
41+
@NotNull List<String> inAppIncludes,
42+
@NotNull List<String> inAppExcludes,
43+
String nativeLibraryDir) {
3544
this.tombstoneStream = tombstoneStream;
45+
this.inAppIncludes = inAppIncludes;
46+
this.inAppExcludes = inAppExcludes;
47+
this.nativeLibraryDir = nativeLibraryDir;
3648

3749
// keep the current signal type -> value mapping for compatibility
3850
excTypeValueMap.put("SIGILL", "IllegalInstruction");
@@ -91,14 +103,38 @@ private List<SentryThread> createThreads(
91103
}
92104

93105
@NonNull
94-
private static SentryStackTrace createStackTrace(@NonNull final TombstoneProtos.Thread thread) {
106+
private SentryStackTrace createStackTrace(@NonNull final TombstoneProtos.Thread thread) {
95107
final List<SentryStackFrame> frames = new ArrayList<>();
96108

97109
for (TombstoneProtos.BacktraceFrame frame : thread.getCurrentBacktraceList()) {
110+
if (frame.getFileName().endsWith("libart.so")) {
111+
// We ignore all ART frames for time being because they aren't actionable for app developers
112+
continue;
113+
}
98114
final SentryStackFrame stackFrame = new SentryStackFrame();
99115
stackFrame.setPackage(frame.getFileName());
100116
stackFrame.setFunction(frame.getFunctionName());
101117
stackFrame.setInstructionAddr(formatHex(frame.getPc()));
118+
119+
// TODO: is this the right order?
120+
boolean inApp = false;
121+
for (String inclusion : this.inAppIncludes) {
122+
if (frame.getFunctionName().startsWith(inclusion)) {
123+
inApp = true;
124+
break;
125+
}
126+
}
127+
128+
for (String exclusion : this.inAppExcludes) {
129+
if (frame.getFunctionName().startsWith(exclusion)) {
130+
inApp = false;
131+
break;
132+
}
133+
}
134+
135+
inApp = inApp || frame.getFileName().startsWith(this.nativeLibraryDir);
136+
137+
stackFrame.setInApp(inApp);
102138
frames.add(0, stackFrame);
103139
}
104140

‎sentry-android-core/src/test/java/io/sentry/android/core/ApplicationExitIntegrationTestBase.kt‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -54,6 +54,9 @@ abstract class ApplicationExitIntegrationTestBase<THint : Any> {
5454
@BeforeTest
5555
fun `set up`() {
5656
val context = ApplicationProvider.getApplicationContext<Context>()
57+
// the integration test app has no native library and as such we have to inject one here
58+
context.applicationInfo.nativeLibraryDir =
59+
"/data/app/~~YtXYvdWm5vDHUWYCmVLG_Q==/io.sentry.samples.android-Q2_nG8SyOi4X_6hGGDGE2Q==/lib/arm64"
5760
fixture.init(context)
5861
}
5962

‎sentry-android-core/src/test/java/io/sentry/android/core/internal/tombstone/TombstoneParserTest.kt‎

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -46,11 +46,16 @@ class TombstoneParserTest {
4646
"x28",
4747
)
4848

49+
val inAppIncludes = arrayListOf("io.sentry.samples.android")
50+
val inAppExcludes = arrayListOf<String>()
51+
val nativeLibraryDir =
52+
"/data/app/~~YtXYvdWm5vDHUWYCmVLG_Q==/io.sentry.samples.android-Q2_nG8SyOi4X_6hGGDGE2Q==/lib/arm64"
53+
4954
@Test
5055
fun `parses a snapshot tombstone into Event`() {
5156
val tombstoneStream =
5257
GZIPInputStream(TombstoneParserTest::class.java.getResourceAsStream("/tombstone.pb.gz"))
53-
val parser = TombstoneParser(tombstoneStream)
58+
val parser = TombstoneParser(tombstoneStream, inAppIncludes, inAppExcludes, nativeLibraryDir)
5459
val event = parser.parse()
5560

5661
// top-level data
@@ -93,6 +98,15 @@ class TombstoneParserTest {
9398
assertNotNull(frame.function)
9499
assertNotNull(frame.`package`)
95100
assertNotNull(frame.instructionAddr)
101+
102+
if (thread.id == crashedThreadId) {
103+
if (frame.isInApp!!) {
104+
assert(
105+
frame.function!!.startsWith(inAppIncludes[0]) ||
106+
frame.filename!!.startsWith(nativeLibraryDir)
107+
)
108+
}
109+
}
96110
}
97111

98112
assert(thread.stacktrace!!.registers!!.keys.containsAll(expectedRegisters))
@@ -160,7 +174,13 @@ class TombstoneParserTest {
160174
)
161175
.build()
162176

163-
val parser = TombstoneParser(ByteArrayInputStream(tombstone.toByteArray()))
177+
val parser =
178+
TombstoneParser(
179+
ByteArrayInputStream(tombstone.toByteArray()),
180+
inAppIncludes,
181+
inAppExcludes,
182+
nativeLibraryDir,
183+
)
164184
val event = parser.parse()
165185

166186
val images = event.debugMeta!!.images!!

0 commit comments

Comments
 (0)