Skip to content
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

[PLAT-4707] Record thread information for unhandled JS errors #766

Merged
merged 6 commits into from
Jul 31, 2020
Merged
Show file tree
Hide file tree
Changes from 2 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
3 changes: 1 addition & 2 deletions Bugsnag/BugsnagCrashSentry.m
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,7 @@ - (void)install:(BugsnagConfiguration *)config
ksCrash.introspectMemory = YES;
ksCrash.onCrash = onCrash;
ksCrash.maxStoredReports = BSG_MAX_STORED_REPORTS;
ksCrash.threadTracingEnabled = (int) config.sendThreads;


fractalwrench marked this conversation as resolved.
Show resolved Hide resolved
// User reported events are *always* handled
BSG_KSCrashType crashTypes = BSG_KSCrashTypeUserReported;

Expand Down
14 changes: 10 additions & 4 deletions Bugsnag/Client/BugsnagClient.m
Original file line number Diff line number Diff line change
Expand Up @@ -975,7 +975,10 @@ - (void)notify:(NSException *)exception
* 4. -[BSG_KSCrash captureThreads:depth:]
*/
int depth = (int)(BSGNotifierStackFrameCount);
NSArray *threads = [[BSG_KSCrash sharedInstance] captureThreads:exception depth:depth];
NSArray *threads = [[BSG_KSCrash sharedInstance] captureThreads:exception
depth:depth
unhandled:false
sendThreads:self.configuration.sendThreads];
NSArray *errors = @[[self generateError:exception threads:threads]];

BugsnagMetadata *metadata = [self.metadata deepCopy];
Expand Down Expand Up @@ -1564,14 +1567,17 @@ - (NSArray *)collectBreadcrumbs {
return data;
}

- (NSArray *)collectThreads {
- (NSArray *)collectThreads:(BOOL)unhandled {
// discard the following
// 1. [BugsnagReactNative getPayloadInfo:resolve:reject:]
// 2. [BugsnagClient collectThreads:]
// 3. [BSG_KSCrash captureThreads:]
// 3. [BSG_KSCrash captureThreads:depth:unhandled:]
int depth = 3;
NSException *exc = [NSException exceptionWithName:@"Bugsnag" reason:@"" userInfo:nil];
NSArray<BugsnagThread *> *threads = [[BSG_KSCrash sharedInstance] captureThreads:exc depth:depth];
NSArray<BugsnagThread *> *threads = [[BSG_KSCrash sharedInstance] captureThreads:exc
depth:depth
unhandled:unhandled
sendThreads:self.configuration.sendThreads];
return [BugsnagThread serializeThreads:threads];
}

Expand Down
5 changes: 4 additions & 1 deletion Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.h
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,10 @@
* @param depth the number of frames to discard from the main thread's stacktrace
* @return an array of BugsnagThread
fractalwrench marked this conversation as resolved.
Show resolved Hide resolved
*/
- (NSArray<BugsnagThread *> *)captureThreads:(NSException *)exc depth:(int)depth;
- (NSArray<BugsnagThread *> *)captureThreads:(NSException *)exc
depth:(int)depth
unhandled:(BOOL)unhandled
sendThreads:(BSGThreadSendPolicy)sendThreads;
fractalwrench marked this conversation as resolved.
Show resolved Hide resolved

/**
* Collects information about the application's foreground state (duration in foreground/background)
Expand Down
21 changes: 16 additions & 5 deletions Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrash.m
Original file line number Diff line number Diff line change
Expand Up @@ -38,9 +38,10 @@
#import "BSG_KSLogger.h"
#import "BugsnagThread.h"
#import "BSGSerialization.h"
#import "BugsnagErrorReportSink.h"
#import "Bugsnag.h"
#import "BugsnagCollections.h"
#import "BSG_KSCrashReportFields.h"
#import "Private.h"

fractalwrench marked this conversation as resolved.
Show resolved Hide resolved
#if BSG_HAS_UIKIT
#import <UIKit/UIKit.h>
Expand Down Expand Up @@ -147,8 +148,13 @@ - (id)initWithReportFilesDirectory:(NSString *)reportFilesDirectory {
self.maxStoredReports = 5;

self.reportWhenDebuggerIsAttached = NO;
self.threadTracingEnabled = BSGThreadSendPolicyAlways;
self.writeBinaryImagesForUserReported = YES;

// overridden elsewhere for handled errors, so we can assume that this only
// applies to unhandled errors
BSGThreadSendPolicy sendThreads = [Bugsnag configuration].sendThreads;
self.threadTracingEnabled = sendThreads == BSGThreadSendPolicyAlways
|| sendThreads == BSGThreadSendPolicyUnhandledOnly;
}
return self;
}
Expand Down Expand Up @@ -199,7 +205,7 @@ - (void)setReportWhenDebuggerIsAttached:(BOOL)reportWhenDebuggerIsAttached {
bsg_kscrash_setReportWhenDebuggerIsAttached(reportWhenDebuggerIsAttached);
}

- (void)setThreadTracingEnabled:(int)threadTracingEnabled {
- (void)setThreadTracingEnabled:(bool)threadTracingEnabled {
_threadTracingEnabled = threadTracingEnabled;
bsg_kscrash_setThreadTracingEnabled(threadTracingEnabled);
}
Expand Down Expand Up @@ -282,7 +288,10 @@ - (void)sendAllReports {
}];
}

- (NSArray<BugsnagThread *> *)captureThreads:(NSException *)exc depth:(int)depth {
- (NSArray<BugsnagThread *> *)captureThreads:(NSException *)exc
depth:(int)depth
unhandled:(BOOL)unhandled
sendThreads:(BSGThreadSendPolicy)sendThreads {
NSArray *addresses = [exc callStackReturnAddresses];
int numFrames = (int) [addresses count];
uintptr_t *callstack;
Expand All @@ -308,7 +317,9 @@ - (void)sendAllReports {
}
}

char *trace = bsg_kscrash_captureThreadTrace(depth, numFrames, callstack);
bool recordAllThreads = sendThreads == BSGThreadSendPolicyAlways
|| (sendThreads == BSGThreadSendPolicyUnhandledOnly && unhandled);
char *trace = bsg_kscrash_captureThreadTrace(depth, numFrames, callstack, recordAllThreads);
free(callstack);
NSDictionary *json = BSGDeserializeJson(trace);
free(trace);
Expand Down
11 changes: 6 additions & 5 deletions Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.c
Original file line number Diff line number Diff line change
Expand Up @@ -213,11 +213,11 @@ void bsg_kscrash_setReportWhenDebuggerIsAttached(
reportWhenDebuggerIsAttached;
}

void bsg_kscrash_setThreadTracingEnabled(int threadTracingEnabled) {
void bsg_kscrash_setThreadTracingEnabled(bool threadTracingEnabled) {
crashContext()->crash.threadTracingEnabled = threadTracingEnabled;
}

char *bsg_kscrash_captureThreadTrace(int discardDepth, int frameCount, uintptr_t *callstack) {
char *bsg_kscrash_captureThreadTrace(int discardDepth, int frameCount, uintptr_t *callstack, const bool recordAllThreads) {
BSG_KSCrash_Context *context = crashContext();

// populate context with pre-recorded stacktrace/thread info
Expand All @@ -227,19 +227,20 @@ char *bsg_kscrash_captureThreadTrace(int discardDepth, int frameCount, uintptr_t
context->crash.userException.discardDepth = discardDepth;
context->crash.offendingThread = bsg_ksmachthread_self();
context->crash.crashType = BSG_KSCrashTypeUserReported;
context->crash.threadTracingEnabled = recordAllThreads;

// No need to gather notable addresses for handled errors
context->config.introspectionRules.enabled = false;

// Only suspend threads if tracing is set to always
// (to ensure trace is captured at the same point in time)
if (context->crash.threadTracingEnabled == 0) {
if (context->crash.threadTracingEnabled) {
bsg_kscrashsentry_suspend_threads_user();
}

char *trace = bsg_kscrw_i_captureThreadTrace(context);

if (context->crash.threadTracingEnabled == 0) {
if (context->crash.threadTracingEnabled) {
bsg_kscrashsentry_resume_threads_user(false);
}
return trace;
Expand Down
4 changes: 2 additions & 2 deletions Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashC.h
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ void bsg_kscrash_reportUserException(const char *name, const char *reason,
void bsg_kscrash_setReportWhenDebuggerIsAttached(
bool reportWhenDebuggerIsAttached);

void bsg_kscrash_setThreadTracingEnabled(int threadTracingEnabled);
void bsg_kscrash_setThreadTracingEnabled(bool threadTracingEnabled);

void bsg_kscrash_setWriteBinaryImagesForUserReported(
bool writeBinaryImagesForUserReported);
Expand All @@ -171,7 +171,7 @@ BSG_KSCrash_Context *crashContext(void);
*
* @return a trace of all the threads as a JSON string.
*/
char *bsg_kscrash_captureThreadTrace(int discardDepth, int frameCount, uintptr_t *callstack);
char *bsg_kscrash_captureThreadTrace(int discardDepth, int frameCount, uintptr_t *callstack, const bool recordAllThreads);

#ifdef __cplusplus
}
Expand Down
17 changes: 7 additions & 10 deletions Bugsnag/KSCrash/Source/KSCrash/Recording/BSG_KSCrashReport.c
Original file line number Diff line number Diff line change
Expand Up @@ -627,7 +627,7 @@ void bsg_kscrw_i_writeMemoryContents(
const BSG_KSCrashReportWriter *const writer, const char *const key,
const uintptr_t address, int *limit);

void bsg_kscrw_i_writeTraceInfo(const BSG_KSCrash_Context *crashContext,
void bsg_kscrw_i_writeTraceInfo(const BSG_KSCrash_Context *crashContext,
const BSG_KSCrashReportWriter *writer);

bool bsg_kscrw_i_exceedsBufferLen(const size_t length);
Expand Down Expand Up @@ -1034,6 +1034,7 @@ void bsg_kscrw_i_writeThread(const BSG_KSCrashReportWriter *const writer,
* @param writeNotableAddresses whether notable addresses should be written
* so additional information about the error can be extracted
* only the main thread's stacktrace is serialized.
* @param recordAllThreads whether all threads should be recorded, or just the current one
fractalwrench marked this conversation as resolved.
Show resolved Hide resolved
*/
void bsg_kscrw_i_writeAllThreads(const BSG_KSCrashReportWriter *const writer,
const char *const key,
Expand All @@ -1048,17 +1049,13 @@ void bsg_kscrw_i_writeAllThreads(const BSG_KSCrashReportWriter *const writer,
BSG_KSLOG_ERROR("task_threads: %s", mach_error_string(kr));
return;
}

bool recordAllThreads = crash->threadTracingEnabled == 0 // Always
|| (crash->threadTracingEnabled == 1 // Unhandled Only
&& crash->crashType != BSG_KSCrashTypeUserReported);

// Fetch info for all threads.
writer->beginArray(writer, key);
{
for (mach_msg_type_number_t i = 0; i < numThreads; i++) {
thread_t thread = threads[i];
if (recordAllThreads || thread == crash->offendingThread) {
if (crash->threadTracingEnabled || thread == crash->offendingThread) {
bsg_kscrw_i_writeThread(writer, NULL, crash, thread, (int) i, writeNotableAddresses);
}
}
Expand Down Expand Up @@ -1702,17 +1699,17 @@ char *bsg_kscrw_i_captureThreadTrace(const BSG_KSCrash_Context *crashContext) {

void bsg_kscrw_i_writeTraceInfo(const BSG_KSCrash_Context *crashContext,
const BSG_KSCrashReportWriter *writer) {
bool unhandledCrash = crashContext->crash.crashType != BSG_KSCrashTypeUserReported;
const BSG_KSCrash_SentryContext *crash = &crashContext->crash;

// Don't write the binary images for user reported crashes to improve performance
if (crashContext->crash.writeBinaryImagesForUserReported == true || unhandledCrash) {
if (crash->writeBinaryImagesForUserReported == true || crashContext->crash.crashType != BSG_KSCrashTypeUserReported) {
fractalwrench marked this conversation as resolved.
Show resolved Hide resolved
bsg_kscrw_i_writeBinaryImages(writer, BSG_KSCrashField_BinaryImages);
}
writer->beginObject(writer, BSG_KSCrashField_Crash);
{
bsg_kscrw_i_writeAllThreads(writer, BSG_KSCrashField_Threads, &crashContext->crash,
bsg_kscrw_i_writeAllThreads(writer, BSG_KSCrashField_Threads, crash,
crashContext->config.introspectionRules.enabled);
bsg_kscrw_i_writeError(writer, BSG_KSCrashField_Error,&crashContext->crash);
bsg_kscrw_i_writeError(writer, BSG_KSCrashField_Error,crash);
}
writer->endContainer(writer);
}
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,7 @@ void bsg_kscrashsentry_resumeThreads(void) {

void bsg_kscrashsentry_clearContext(BSG_KSCrash_SentryContext *context) {
void (*onCrash)(void *) = context->onCrash;
int threadTracingEnabled = context->threadTracingEnabled;
bool threadTracingEnabled = context->threadTracingEnabled;
bool reportWhenDebuggerIsAttached = context->reportWhenDebuggerIsAttached;
bool writeBinaryImagesForUserReported =
context->writeBinaryImagesForUserReported;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ typedef struct BSG_KSCrash_SentryContext {
* The methodology used for tracing threads.
* The value will be equal to an enum value from BSGThreadSendPolicy
fractalwrench marked this conversation as resolved.
Show resolved Hide resolved
*/
int threadTracingEnabled;
bool threadTracingEnabled;

/** If true, will record binary images. */
bool writeBinaryImagesForUserReported;
Expand Down
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,10 @@ Changelog
## TBD

### Bug fixes

* Record thread information for unhandled JS errors
[#766](https://github.com/bugsnag/bugsnag-cocoa/pull/766)

* Respect bundle version set from config
[#762](https://github.com/bugsnag/bugsnag-cocoa/pull/762)

Expand Down
3 changes: 2 additions & 1 deletion Tests/BugsnagClientMirrorTest.m
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,8 @@ - (void)setUp {
@"context @16@0:8",
@"collectAppWithState @16@0:8",
@"collectBreadcrumbs @16@0:8",
@"collectThreads @16@0:8",
@"collectThreads: @20@0:8B16",
@"collectThreads: @20@0:8c16",
@"collectDeviceWithState @16@0:8",
@"extraRuntimeInfo @16@0:8",
@"setExtraRuntimeInfo: v24@0:8@16",
Expand Down