fix: Remove dispatch queue metadata collection to fix crash #3522

indragiek · 2023-12-30T00:46:20Z

📜 Description

This PR removes the collection of dispatch queue metadata when profiling, and uses main as the hard-coded thread name for the main thread (in place of the queue name com.apple.main-thread) for consistency with other platforms.

💡 Motivation and Context

We have crashes in production from trying to read ThreadHandle::dispatchQueueAddress when profiling: #3503

This appears to be a race condition with queue lifetime that is not possible to fully fix. There are similar problems encountered by other projects trying to use the same API:

💚 How did you test it?

Updated the tests, checked that the hardcoded main thread name is set.

📝 Checklist

You have to check all boxes before merging:

I reviewed the submitted code.
I added tests to verify the changes.
No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
I updated the docs if needed.
Review from the native team if needed.
No breaking change or entry added to the changelog.
No breaking change for hybrid SDKs or communicated to hybrid SDKs.

🔮 Next steps

…y/sentry-cocoa into indragiek/remove-queue-metadata

codecov · 2023-12-30T00:59:55Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1734d1b) 89.241% compared to head (ff52c12) 89.258%.

Additional details and impacted files

@@              Coverage Diff              @@
##              main     #3522       +/-   ##
=============================================
+ Coverage   89.241%   89.258%   +0.016%     
=============================================
  Files          529       529               
  Lines        57780     57718       -62     
  Branches     20687     20659       -28     
=============================================
- Hits         51564     51518       -46     
+ Misses        5300      5288       -12     
+ Partials       916       912        -4

Files	Coverage Δ
SentryTestUtils/SentryProfilerMocks.mm	`100.000% <ø> (ø)`
...tryTestUtils/SentryProfilerMocksSwiftCompatible.mm	`100.000% <100.000%> (ø)`
Sources/Sentry/Profiling/SentryProfilerState.mm	`98.373% <100.000%> (-0.052%)`	⬇️
Sources/Sentry/SentryBacktrace.cpp	`88.172% <100.000%> (+1.564%)`	⬆️
Sources/Sentry/SentryThreadHandle.cpp	`70.121% <ø> (+0.891%)`	⬆️
Sources/Sentry/SentryThreadMetadataCache.cpp	`87.096% <ø> (-4.208%)`	⬇️
...urces/Sentry/include/SentryThreadMetadataCache.hpp	`100.000% <ø> (ø)`
...SentryProfilerTests/SentryProfilerSwiftTests.swift	`97.739% <100.000%> (-0.033%)`	⬇️
Tests/SentryProfilerTests/SentryProfilerTests.mm	`98.159% <100.000%> (-0.045%)`	⬇️
...tryProfilerTests/SentryThreadMetadataCacheTests.mm	`93.939% <ø> (-1.061%)`	⬇️

... and 9 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1734d1b...ff52c12. Read the comment docs.

github-actions · 2023-12-30T01:02:29Z

Performance metrics 🚀

	Plain	With Sentry	Diff
Startup time	1234.96 ms	1248.09 ms	13.13 ms
Size	21.58 KiB	417.86 KiB	396.28 KiB

Baseline results on branch: main

Startup times

Revision	Plain	With Sentry	Diff
`c0ff306`	1219.24 ms	1243.96 ms	24.72 ms
`9883c0f`	1207.59 ms	1223.14 ms	15.55 ms
`313b1d9`	1240.18 ms	1258.44 ms	18.26 ms
`034be1c`	1222.67 ms	1236.22 ms	13.55 ms
`156e771`	1228.06 ms	1242.64 ms	14.58 ms
`e2abb0d`	1235.08 ms	1257.00 ms	21.92 ms
`216bdf9`	1199.84 ms	1209.53 ms	9.69 ms
`b9a9ffd`	1221.18 ms	1235.37 ms	14.19 ms
`a9103fe`	1221.49 ms	1243.33 ms	21.84 ms
`3f6c30b`	1252.98 ms	1266.22 ms	13.24 ms

App size

Revision	Plain	With Sentry	Diff
`c0ff306`	20.76 KiB	434.65 KiB	413.89 KiB
`9883c0f`	22.85 KiB	405.47 KiB	382.62 KiB
`313b1d9`	22.85 KiB	414.11 KiB	391.26 KiB
`034be1c`	20.76 KiB	436.66 KiB	415.90 KiB
`156e771`	20.76 KiB	419.70 KiB	398.94 KiB
`e2abb0d`	20.76 KiB	434.72 KiB	413.96 KiB
`216bdf9`	21.58 KiB	418.13 KiB	396.54 KiB
`b9a9ffd`	21.58 KiB	418.43 KiB	396.85 KiB
`a9103fe`	20.76 KiB	426.95 KiB	406.19 KiB
`3f6c30b`	22.85 KiB	408.88 KiB	386.03 KiB

Previous results on branch: indragiek/remove-queue-metadata

Startup times

Revision	Plain	With Sentry	Diff
`5e6b38e`	1194.29 ms	1209.20 ms	14.92 ms

App size

Revision	Plain	With Sentry	Diff
`5e6b38e`	21.58 KiB	417.17 KiB	395.58 KiB

philipphofmann

Good catch, thanks @indragiek 👍

philipphofmann · 2024-01-02T10:23:21Z

Sources/Sentry/Profiling/SentryProfilerState.mm

-        if (!backtrace.threadMetadata.name.empty() && metadata[@"name"] == nil) {
-            metadata[@"name"] =
-                [NSString stringWithUTF8String:backtrace.threadMetadata.name.c_str()];
+        if (metadata[@"name"] == nil) {


m: I think we shoud first check if backtrace.threadMetadata.threadID == self->_mainThreadID. Otherwise, the main thread could have different names depending on the platform. Or is this the desired behavior?

This looks like it's checking if it's already been set previously, as we reuse the metadata dictionary. The check you mention does happen before we actually go to set the value on line 107 below in this hunk.

Yeah @armcknight is right, that was the intent here

armcknight

LGTM

armcknight · 2024-01-08T19:24:11Z

Sources/Sentry/Profiling/SentryProfilerState.mm

+        if ([NSThread isMainThread]) {
+            [self cacheMainThreadID];
+        } else {
+            dispatch_async(dispatch_get_main_queue(), ^{ [self cacheMainThreadID]; });
+        }


We have a wrapper for this, i pushed commit 9fbdeb6 to use it

armcknight · 2024-01-08T19:25:09Z

Tests/SentryProfilerTests/SentryProfilerSwiftTests.swift

-            }
-        } else {
-            XCTAssertFalse(try XCTUnwrap(try XCTUnwrap(queueMetadata.first?.value)["label"] as? String).isEmpty)
+            XCTAssertFalse(try threadMetadata.values.compactMap { $0["name"] }.filter { try XCTUnwrap($0 as? String) == "main" }.isEmpty)


Nice, this probably should have been there before, but thanks for adding it 👍🏻

armcknight · 2024-01-08T23:16:03Z

I logged the thread sanitizer warning in a new issue, I don't think it's related.

* Remove dispatch queue metadata collection * Hardcode "main" as the name of the main thread * Format code * Update CHANGELOG.md * use thread wrapper main thread dispatch --------- Co-authored-by: Sentry Github Bot <[email protected]> Co-authored-by: Andrew McKnight <[email protected]>

indragiek added 2 commits December 28, 2023 12:43

Remove dispatch queue metadata collection

bb6f737

Hardcode "main" as the name of the main thread

2dca470

indragiek requested review from philipphofmann, brustolin and armcknight as code owners December 30, 2023 00:46

indragiek mentioned this pull request Dec 30, 2023

EXC_BAD_ACCESS in sentry::profiling::backtrace #3223

Open

getsentry-bot and others added 3 commits December 30, 2023 00:47

Format code

8a19bbe

Update CHANGELOG.md

db05a92

Merge branch 'indragiek/remove-queue-metadata' of github.com:getsentr…

e6cc5f9

…y/sentry-cocoa into indragiek/remove-queue-metadata

philipphofmann approved these changes Jan 2, 2024

View reviewed changes

philipphofmann mentioned this pull request Jan 3, 2024

-[SentryProfilerState appendBacktrace:]_block_invoke #3510

Closed

use thread wrapper main thread dispatch

9fbdeb6

armcknight approved these changes Jan 8, 2024

View reviewed changes

Merge branch 'main' into indragiek/remove-queue-metadata

ff52c12

armcknight mentioned this pull request Jan 8, 2024

Fix potential data race in SentryTracerTests.testAddingSpansOnDifferentThread_WhileFinishing_DoesNotCrash (TSAN warning) #3550

Closed

indragiek merged commit 94d8eb3 into main Jan 9, 2024
69 of 70 checks passed

indragiek deleted the indragiek/remove-queue-metadata branch January 9, 2024 02:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Remove dispatch queue metadata collection to fix crash #3522

fix: Remove dispatch queue metadata collection to fix crash #3522

indragiek commented Dec 30, 2023

codecov bot commented Dec 30, 2023 •

edited

Loading

github-actions bot commented Dec 30, 2023 •

edited

Loading

Baseline results on branch: main

Startup times

App size

Previous results on branch: indragiek/remove-queue-metadata

Startup times

App size

philipphofmann left a comment

philipphofmann Jan 2, 2024

armcknight Jan 8, 2024

indragiek Jan 9, 2024

armcknight left a comment

armcknight Jan 8, 2024 •

edited

Loading

armcknight Jan 8, 2024

armcknight commented Jan 8, 2024

fix: Remove dispatch queue metadata collection to fix crash #3522

fix: Remove dispatch queue metadata collection to fix crash #3522

Conversation

indragiek commented Dec 30, 2023

📜 Description

💡 Motivation and Context

💚 How did you test it?

📝 Checklist

🔮 Next steps

codecov bot commented Dec 30, 2023 • edited Loading

Codecov Report

github-actions bot commented Dec 30, 2023 • edited Loading

Performance metrics 🚀

Baseline results on branch: main

Startup times

App size

Previous results on branch: indragiek/remove-queue-metadata

Startup times

App size

philipphofmann left a comment

Choose a reason for hiding this comment

philipphofmann Jan 2, 2024

Choose a reason for hiding this comment

armcknight Jan 8, 2024

Choose a reason for hiding this comment

indragiek Jan 9, 2024

Choose a reason for hiding this comment

armcknight left a comment

Choose a reason for hiding this comment

armcknight Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

armcknight Jan 8, 2024

Choose a reason for hiding this comment

armcknight commented Jan 8, 2024

codecov bot commented Dec 30, 2023 •

edited

Loading

github-actions bot commented Dec 30, 2023 •

edited

Loading

armcknight Jan 8, 2024 •

edited

Loading