sentry-cocoa icon indicating copy to clipboard operation
sentry-cocoa copied to clipboard

There's an 100-thread limit in crash reports

Open dnguyen032123 opened this issue 1 year ago • 2 comments

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 avatar May 22 '24 19:05 dnguyen032123

Thanks @dnguyen032123 for reaching out.

I believe this is a hard limit defined in here: https://github.com/getsentry/sentry-cocoa/blob/cf972098baef6f2042eee18549c33451fd983731/Sources/SentryCrash/Recording/Tools/SentryCrashMachineContext_Apple.h#L47

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.

brustolin avatar May 23 '24 08:05 brustolin

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 https://github.com/getsentry/sentry-cocoa/blob/cf972098baef6f2042eee18549c33451fd983731/Sources/SentryCrash/Recording/Tools/SentryCrashMachineContext.c#L60-L92

Then we use the information from the SentryCrashMachineContext to write the list of threads here: https://github.com/getsentry/sentry-cocoa/blob/cf972098baef6f2042eee18549c33451fd983731/Sources/SentryCrash/Recording/SentryCrashReport.c#L1058-L1115

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.

philipphofmann avatar May 23 '24 14:05 philipphofmann

I met the same issue. Do you have any plan to raise the limit to 200 threads? @philipphofmann Or could you at lease make stacktrace of crashed thread be collected?

ShiBody avatar Dec 19 '24 06:12 ShiBody

We don't have an ETA yet, @ShiBody.

philipphofmann avatar Dec 19 '24 13:12 philipphofmann

@philipphofmann I do think it is an ungent requirement to make sure crashed thread is included in event.threads. Actually most of threads are useless. What we really care about is the crashed thread. Now we may also lose crashed thread even we expand limit to 200.

ShiBody avatar Dec 20 '24 02:12 ShiBody