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

There's an 100-thread limit in crash reports #4006

Open
dnguyen032123 opened this issue May 22, 2024 · 2 comments
Open

There's an 100-thread limit in crash reports #4006

dnguyen032123 opened this issue May 22, 2024 · 2 comments

Comments

@dnguyen032123
Copy link

dnguyen032123 commented May 22, 2024

Platform

macOS

Environment

Production

Installed

Manually

Version

8.22.1

Xcode Version

15.1

Did it work on previous versions?

No response

Steps to Reproduce

The code below tries to make the thread 100 crash. Apple's crash report shows that thread 108 (some additional threads were automatically created by Cocoa, I think) crashed indeed. However, the crash report on the Sentry server doesn't show the crashed thread (108); thread 0 is shown by default instead. Also, the Threads dropdown only contains threads 0 to 99, so the crashed thread is not even in the dropdown. I checked the local .json crash report in the SentryCrash folder; it doesn't seem to have the crashed thread either. Thus, I think there's a 100-thread limit. Could we increase it or at least show the crashed thread at the top of the dropdown?

#include <thread>
#include <chrono>

- (void)applicationDidFinishLaunching:(NSNotification *)aNotification {
    const int numThreads = 101;
    std::vector<std::thread> threads;
    for (int i = 0; i < numThreads; ++i)
    {
        if (i == numThreads - 1)
        {
            threads.emplace_back([]() {
                int* x = nullptr;
                *x = 10;
            });
        }
        else
        {
            threads.emplace_back([]() {
                std::this_thread::sleep_for(std::chrono::seconds(1));
            });
        }
    }

    for (auto& worker : threads)
    {
        worker.join();
    }
}

Expected Result

The crashed thread (108) is at the top of the Threads dropdown, and its backtrace is also shown.

Actual Result

The thread 0 is at the top of the Threads dropdown, and its backtrace is shown instead.
Screenshot_2024-05-21_at_1 15 00_PM
test_sentry_with_memmove-report-000000009c800000.json

Are you willing to submit a PR?

No response

@dnguyen032123 dnguyen032123 changed the title There's a 100-thread limit in crash reports There's an 100-thread limit in crash reports May 22, 2024
@brustolin
Copy link
Contributor

brustolin commented May 23, 2024

Thanks @dnguyen032123 for reaching out.

I believe this is a hard limit defined in here:

We need to have a limit because it's not possible to allocate more memory during crash handling. We will investigate whether it is okay to raise this limit.

@philipphofmann
Copy link
Member

Yes, indeed, we can't allocate memory when we're crashing, but we can change our approach. When we receive a signal or a mach message we get the thread fill the thread list of the SentryCrashMachineContext here

static inline bool
getThreadList(SentryCrashMachineContext *context)
{
const task_t thisTask = mach_task_self();
SentryCrashLOG_DEBUG("Getting thread list");
kern_return_t kr;
thread_act_array_t threads;
mach_msg_type_number_t actualThreadCount;
if ((kr = task_threads(thisTask, &threads, &actualThreadCount)) != KERN_SUCCESS) {
SentryCrashLOG_ERROR("task_threads: %s", mach_error_string(kr));
return false;
}
SentryCrashLOG_TRACE("Got %d threads", context->threadCount);
int threadCount = (int)actualThreadCount;
int maxThreadCount = sizeof(context->allThreads) / sizeof(context->allThreads[0]);
if (threadCount > maxThreadCount) {
SentryCrashLOG_ERROR(
"Thread count %d is higher than maximum of %d", threadCount, maxThreadCount);
threadCount = maxThreadCount;
}
for (int i = 0; i < threadCount; i++) {
context->allThreads[i] = threads[i];
}
context->threadCount = threadCount;
for (mach_msg_type_number_t i = 0; i < actualThreadCount; i++) {
mach_port_deallocate(thisTask, context->allThreads[i]);
}
vm_deallocate(thisTask, (vm_address_t)threads, sizeof(thread_t) * actualThreadCount);
return true;
}

Then we use the information from the SentryCrashMachineContext to write the list of threads here:

/** Write information about a thread to the report.
*
* @param writer The writer.
*
* @param key The object key, if needed.
*
* @param crash The crash handler context.
*
* @param machineContext The context whose thread to write about.
*
* @param shouldWriteNotableAddresses If true, write any notable addresses
* found.
*/
static void
writeThread(const SentryCrashReportWriter *const writer, const char *const key,
const SentryCrash_MonitorContext *const crash,
const struct SentryCrashMachineContext *const machineContext, const int threadIndex,
const bool shouldWriteNotableAddresses)
{
bool isCrashedThread = sentrycrashmc_isCrashedContext(machineContext);
SentryCrashThread thread = sentrycrashmc_getThreadFromContext(machineContext);
SentryCrashLOG_DEBUG(
"Writing thread %x (index %d). is crashed: %d", thread, threadIndex, isCrashedThread);
SentryCrashStackCursor stackCursor;
bool hasBacktrace = getStackCursor(crash, machineContext, &stackCursor);
writer->beginObject(writer, key);
{
if (hasBacktrace) {
writeBacktrace(writer, SentryCrashField_Backtrace, &stackCursor);
}
if (sentrycrashmc_canHaveCPUState(machineContext)) {
writeRegisters(writer, SentryCrashField_Registers, machineContext);
}
writer->addIntegerElement(writer, SentryCrashField_Index, threadIndex);
const char *name = sentrycrashccd_getThreadName(thread);
if (name != NULL) {
writer->addStringElement(writer, SentryCrashField_Name, name);
}
name = sentrycrashccd_getQueueName(thread);
if (name != NULL) {
writer->addStringElement(writer, SentryCrashField_DispatchQueue, name);
}
writer->addBooleanElement(writer, SentryCrashField_Crashed, isCrashedThread);
writer->addBooleanElement(
writer, SentryCrashField_CurrentThread, thread == sentrycrashthread_self());
if (isCrashedThread) {
writeStackContents(
writer, SentryCrashField_Stack, machineContext, stackCursor.state.hasGivenUp);
if (shouldWriteNotableAddresses) {
writeNotableAddresses(writer, SentryCrashField_NotableAddresses, machineContext);
}
}
}
writer->endContainer(writer);
}

getThreadList already stores all threads in a thread_act_array_t when we're crashing. So instead of storing the thread_act_array_t into a fixed-sized thread_t array with a limit of 100, maybe we could directly use the thread_act_array_t when writing the crash report. A simple workaround would be to increase the number to 200 or something, cause thread_t allocates between 4 and 8 bytes if I'm not mistaken. The memory overhead could be acceptable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: No status
Status: Needs Discussion
Development

No branches or pull requests

3 participants