From 4ee42052164d0c5d3aa2373077aaaedd3707fb1e Mon Sep 17 00:00:00 2001 From: Antonis Lilis Date: Thu, 23 Apr 2026 11:19:45 +0200 Subject: [PATCH 1/5] fix(android): Stop Hermes sampling profiler on React instance teardown (#5441) HermesSamplingProfiler.enable() starts a global timer thread that fires pthread_kill(jsThread, SIGPROF) every ~10ms to sample stacks. Before this change, nothing stopped it when the ReactInstance was torn down (Metro reload, orderly Activity destroy, CodePush swap), so the sampler could call pthread_kill on a pthread that was already joined by RN's teardown sequence, resulting in SIGABRT in libhermes.so at hermes::vm::sampling_profiler::Sampler::platformSuspendVMAndWalkStack. Hermes's own Sampler::disable() synchronously joins the timer thread before returning. Calling it from the module's invalidate() hook closes the window before the JS thread is destroyed. - Override invalidate() in both new-arch and old-arch wrappers to call a cleanup hook on RNSentryModuleImpl. - Defensive HermesSamplingProfiler.disable() before enable() in startProfiling() to flush any leaked registration from a prior run. - Track isProfiling as volatile and snatch the AndroidProfiler reference locally in invalidate() to avoid racing with a concurrent stopProfiling on the JS thread. - Delete the android profiler trace file in invalidate() to avoid leaking it in cacheDir when the profile is being discarded. Upstream race: facebook/hermes#1853. Co-Authored-By: Claude Opus 4.7 (1M context) --- CHANGELOG.md | 1 + .../io/sentry/react/RNSentryModuleImpl.java | 56 ++++++++++++++++++- .../java/io/sentry/react/RNSentryModule.java | 6 ++ .../java/io/sentry/react/RNSentryModule.java | 6 ++ 4 files changed, 68 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5416e06a57..9148857719 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,6 +20,7 @@ - Check `captureReplay` return value in iOS bridge to avoid linking error events to uncaptured replays ([#6008](https://github.com/getsentry/sentry-react-native/pull/6008)) - Report the expected properties file path and any missing keys when using `flavorAware` on Android, instead of failing with an opaque `Illegal null value provided in this collection` error ([#6031](https://github.com/getsentry/sentry-react-native/pull/6031)) +- Stop the Hermes sampling profiler on React instance teardown to prevent `pthread_kill` SIGABRT when the JS thread is torn down with profiling active ([#5441](https://github.com/getsentry/sentry-react-native/issues/5441)) ### Dependencies diff --git a/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java b/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java index c8ee3abcd5..80b2e8a6a6 100644 --- a/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java +++ b/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java @@ -114,7 +114,14 @@ public class RNSentryModuleImpl { */ private int profilingTracesHz = 101; - private AndroidProfiler androidProfiler = null; + // Volatile because invalidate() (NativeModules/teardown thread) can null this concurrently + // with stopProfiling() (JS thread); we need cross-thread visibility of the null write. + private volatile AndroidProfiler androidProfiler = null; + + // Written on the JS thread (start/stopProfiling via @ReactMethod isBlockingSynchronousMethod), + // read on the NativeModules/teardown thread (invalidate). volatile ensures cross-thread + // visibility so invalidate() cannot observe a stale `false` while profiling is active. + private volatile boolean isProfiling = false; private boolean isProguardDebugMetaLoaded = false; private @Nullable String proguardUuid = null; @@ -789,10 +796,15 @@ public WritableMap startProfiling(boolean platformProfilers) { } try { + // Defensive reset: the Hermes sampling profiler is global with no state + // introspection, so flush any leaked registration from a prior run before + // enabling. See https://github.com/facebook/hermes/issues/1853. + HermesSamplingProfiler.disable(); HermesSamplingProfiler.enable(); if (androidProfiler != null) { androidProfiler.start(); } + isProfiling = true; result.putBoolean("started", true); } catch (Throwable e) { // NOPMD - We don't want to crash in any case @@ -812,6 +824,7 @@ public WritableMap stopProfiling() { end = androidProfiler.endAndCollect(false, null); } HermesSamplingProfiler.disable(); + isProfiling = false; output = File.createTempFile( @@ -852,6 +865,47 @@ public WritableMap stopProfiling() { return result; } + /** + * Cleanup hook invoked from the module wrappers when the React instance is destroyed (e.g. Metro + * reload, orderly Activity teardown). Calls HermesSamplingProfiler.disable(), which synchronously + * joins the sampler's timer thread, so no pthread_kill can fire after the JS thread is torn down. + * + *

Correctness depends on RN invoking module invalidate() before joining the JS message queue + * thread. That is the documented contract for both old-arch CatalystInstance shutdown and + * new-arch TurboModule teardown. + * + *

See https://github.com/getsentry/sentry-react-native/issues/5441 and + * https://github.com/facebook/hermes/issues/1853. + */ + public void invalidate() { + if (!isProfiling) { + return; + } + // Flip the flag first to make a re-entrant invalidate() a no-op. Then snatch the + // AndroidProfiler reference and null the field so a concurrent stopProfiling() on the + // JS thread sees null and skips its own endAndCollect() on the same instance. + isProfiling = false; + final AndroidProfiler profiler = androidProfiler; + androidProfiler = null; + + try { + if (profiler != null) { + final AndroidProfiler.ProfileEndData end = profiler.endAndCollect(false, null); + // The profile is being discarded; delete the trace file to avoid leaking in cacheDir. + if (end != null && end.traceFile != null) { + try { + end.traceFile.delete(); + } catch (Throwable ignored) { // NOPMD - File cleanup is best-effort. + } + } + } + HermesSamplingProfiler.disable(); + logger.log(SentryLevel.INFO, "Stopped Hermes sampling profiler on React instance destroy."); + } catch (Throwable e) { // NOPMD - Guards the JNI boundary; does not catch native SIGABRT. + logger.log(SentryLevel.WARNING, "Failed to stop Hermes sampling profiler on teardown: " + e); + } + } + private @Nullable String getProguardUuid() { if (isProguardDebugMetaLoaded) { return proguardUuid; diff --git a/packages/core/android/src/newarch/java/io/sentry/react/RNSentryModule.java b/packages/core/android/src/newarch/java/io/sentry/react/RNSentryModule.java index 5ad03e30ca..82d2622996 100644 --- a/packages/core/android/src/newarch/java/io/sentry/react/RNSentryModule.java +++ b/packages/core/android/src/newarch/java/io/sentry/react/RNSentryModule.java @@ -233,4 +233,10 @@ public void enableShakeDetection() { public void disableShakeDetection() { this.impl.disableShakeDetection(); } + + @Override + public void invalidate() { + this.impl.invalidate(); + super.invalidate(); + } } diff --git a/packages/core/android/src/oldarch/java/io/sentry/react/RNSentryModule.java b/packages/core/android/src/oldarch/java/io/sentry/react/RNSentryModule.java index 6ca2fe2087..099623f056 100644 --- a/packages/core/android/src/oldarch/java/io/sentry/react/RNSentryModule.java +++ b/packages/core/android/src/oldarch/java/io/sentry/react/RNSentryModule.java @@ -233,4 +233,10 @@ public void enableShakeDetection() { public void disableShakeDetection() { this.impl.disableShakeDetection(); } + + @Override + public void invalidate() { + this.impl.invalidate(); + super.invalidate(); + } } From a1ec64b2801507bff004ba31ed1d6e58c29e6702 Mon Sep 17 00:00:00 2001 From: Antonis Lilis Date: Thu, 23 Apr 2026 11:23:31 +0200 Subject: [PATCH 2/5] Fix changelog --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9148857719..a74ddf711c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,7 +20,7 @@ - Check `captureReplay` return value in iOS bridge to avoid linking error events to uncaptured replays ([#6008](https://github.com/getsentry/sentry-react-native/pull/6008)) - Report the expected properties file path and any missing keys when using `flavorAware` on Android, instead of failing with an opaque `Illegal null value provided in this collection` error ([#6031](https://github.com/getsentry/sentry-react-native/pull/6031)) -- Stop the Hermes sampling profiler on React instance teardown to prevent `pthread_kill` SIGABRT when the JS thread is torn down with profiling active ([#5441](https://github.com/getsentry/sentry-react-native/issues/5441)) +- Stop the Hermes sampling profiler on React instance teardown to prevent `pthread_kill` SIGABRT when the JS thread is torn down with profiling active ([#6035](https://github.com/getsentry/sentry-react-native/pull/6035)) ### Dependencies From bb56af0f92f3bbcdca353859fd6c8e1e2288d360 Mon Sep 17 00:00:00 2001 From: Antonis Lilis Date: Thu, 23 Apr 2026 11:34:34 +0200 Subject: [PATCH 3/5] fix(android): Address review: atomic state + startProfiling unwind + stopProfiling TOCTOU Apply review feedback from Sentry Warden and Cursor Bugbot: - Replace volatile fields with AtomicBoolean/AtomicReference so PMD's AvoidUsingVolatile rule passes and to make the cross-thread gating explicit. startProfiling/stopProfiling run on the JS thread; invalidate runs on the NativeModules teardown thread. - startProfiling now sets isProfiling=true immediately after HermesSamplingProfiler.enable() succeeds, before androidProfiler.start(). The catch block unwinds via getAndSet(false) + disable() so we can't leave the Hermes sampler running while the flag reads false. - stopProfiling snapshots androidProfiler.get() into a local, closing the TOCTOU where a concurrent invalidate() could null the field between the null-check and endAndCollect(). - invalidate gates on isProfiling.getAndSet(false) and atomically snatches the profiler reference with androidProfiler.getAndSet(null). Co-Authored-By: Claude Opus 4.7 (1M context) --- .../io/sentry/react/RNSentryModuleImpl.java | 62 ++++++++++++------- 1 file changed, 39 insertions(+), 23 deletions(-) diff --git a/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java b/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java index 80b2e8a6a6..a8e4b83b02 100644 --- a/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java +++ b/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java @@ -77,6 +77,8 @@ import java.util.Map; import java.util.Properties; import java.util.concurrent.CountDownLatch; +import java.util.concurrent.atomic.AtomicBoolean; +import java.util.concurrent.atomic.AtomicReference; import java.util.regex.Pattern; import org.jetbrains.annotations.NotNull; import org.jetbrains.annotations.Nullable; @@ -114,14 +116,15 @@ public class RNSentryModuleImpl { */ private int profilingTracesHz = 101; - // Volatile because invalidate() (NativeModules/teardown thread) can null this concurrently - // with stopProfiling() (JS thread); we need cross-thread visibility of the null write. - private volatile AndroidProfiler androidProfiler = null; + // Atomic because invalidate() (NativeModules/teardown thread) can null the reference + // concurrently with stopProfiling() (JS thread). getAndSet gives us an atomic snapshot-and-clear. + private final AtomicReference androidProfiler = new AtomicReference<>(); - // Written on the JS thread (start/stopProfiling via @ReactMethod isBlockingSynchronousMethod), - // read on the NativeModules/teardown thread (invalidate). volatile ensures cross-thread - // visibility so invalidate() cannot observe a stale `false` while profiling is active. - private volatile boolean isProfiling = false; + // Atomic so that invalidate() can claim cleanup responsibility via compareAndSet/getAndSet. + // startProfiling/stopProfiling run on the JS thread (@ReactMethod isBlockingSynchronousMethod); + // invalidate runs on the NativeModules/teardown thread. Without atomicity, invalidate could + // observe a stale false and skip disabling a running Hermes sampler. + private final AtomicBoolean isProfiling = new AtomicBoolean(false); private boolean isProguardDebugMetaLoaded = false; private @Nullable String proguardUuid = null; @@ -780,18 +783,18 @@ private void initializeAndroidProfiler() { } final String tracesFilesDirPath = getProfilingTracesDirPath(); - androidProfiler = + androidProfiler.set( new AndroidProfiler( tracesFilesDirPath, (int) SECONDS.toMicros(1) / profilingTracesHz, new SentryFrameMetricsCollector(reactApplicationContext, logger, buildInfo), () -> executorService, - logger); + logger)); } public WritableMap startProfiling(boolean platformProfilers) { final WritableMap result = new WritableNativeMap(); - if (androidProfiler == null && platformProfilers) { + if (androidProfiler.get() == null && platformProfilers) { initializeAndroidProfiler(); } @@ -801,13 +804,24 @@ public WritableMap startProfiling(boolean platformProfilers) { // enabling. See https://github.com/facebook/hermes/issues/1853. HermesSamplingProfiler.disable(); HermesSamplingProfiler.enable(); - if (androidProfiler != null) { - androidProfiler.start(); + // Mark profiling active immediately after enable() succeeds so a concurrent + // invalidate() observes the running state even if androidProfiler.start() below throws. + isProfiling.set(true); + final AndroidProfiler profiler = androidProfiler.get(); + if (profiler != null) { + profiler.start(); } - isProfiling = true; result.putBoolean("started", true); } catch (Throwable e) { // NOPMD - We don't want to crash in any case + // Unwind Hermes if we enabled it but failed before returning. getAndSet(false) makes + // this idempotent with a concurrent invalidate()/stopProfiling(). + if (isProfiling.getAndSet(false)) { + try { + HermesSamplingProfiler.disable(); + } catch (Throwable ignored) { // NOPMD - Best-effort unwind. + } + } result.putBoolean("started", false); result.putString("error", e.toString()); } @@ -820,11 +834,14 @@ public WritableMap stopProfiling() { File output = null; try { AndroidProfiler.ProfileEndData end = null; - if (androidProfiler != null) { - end = androidProfiler.endAndCollect(false, null); + // Snapshot the reference locally so a concurrent invalidate() cannot null the field + // between the null-check and the endAndCollect() call. + final AndroidProfiler profiler = androidProfiler.get(); + if (profiler != null) { + end = profiler.endAndCollect(false, null); } HermesSamplingProfiler.disable(); - isProfiling = false; + isProfiling.set(false); output = File.createTempFile( @@ -878,15 +895,14 @@ public WritableMap stopProfiling() { * https://github.com/facebook/hermes/issues/1853. */ public void invalidate() { - if (!isProfiling) { + // Atomic gate: only one caller (invalidate vs stopProfiling vs a re-entrant invalidate) + // wins the right to clean up; the rest no-op. + if (!isProfiling.getAndSet(false)) { return; } - // Flip the flag first to make a re-entrant invalidate() a no-op. Then snatch the - // AndroidProfiler reference and null the field so a concurrent stopProfiling() on the - // JS thread sees null and skips its own endAndCollect() on the same instance. - isProfiling = false; - final AndroidProfiler profiler = androidProfiler; - androidProfiler = null; + // getAndSet(null) atomically snatches the reference so a concurrent stopProfiling() + // sees null and skips its own endAndCollect() on the same instance. + final AndroidProfiler profiler = androidProfiler.getAndSet(null); try { if (profiler != null) { From 1c3a4b9214d51a152c7954a422420b006c4955e7 Mon Sep 17 00:00:00 2001 From: Antonis Lilis Date: Thu, 23 Apr 2026 11:50:27 +0200 Subject: [PATCH 4/5] fix(android): Isolate Hermes disable from Android profiler cleanup in invalidate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address two new review findings: - High severity (Cursor Bugbot): the previous invalidate() shared a single try/catch with endAndCollect(). If endAndCollect threw, the HermesSamplingProfiler.disable() call on the line below was skipped, leaving the sampler's timer thread alive — defeating the entire purpose of invalidate(). Reorder so the crash-critical Hermes disable runs first in its own try/catch; the Android platform profiler cleanup follows in an independent try/catch. - Medium severity (Cursor Bugbot / reviewer): stopProfiling could race with invalidate on the same AndroidProfiler instance (both calling endAndCollect concurrently). Gate stopProfiling on isProfiling.compareAndSet(true, false) and take ownership of the profiler reference via androidProfiler.getAndSet(null), mirroring invalidate's pattern. Also add two small idempotency tests that verify invalidate() is a no-op on a fresh module without crossing the Hermes JNI boundary. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../io/sentry/react/RNSentryProfilingTest.kt | 33 +++++++++++++++++ .../io/sentry/react/RNSentryModuleImpl.java | 36 +++++++++++++------ 2 files changed, 59 insertions(+), 10 deletions(-) create mode 100644 packages/core/RNSentryAndroidTester/app/src/test/java/io/sentry/react/RNSentryProfilingTest.kt diff --git a/packages/core/RNSentryAndroidTester/app/src/test/java/io/sentry/react/RNSentryProfilingTest.kt b/packages/core/RNSentryAndroidTester/app/src/test/java/io/sentry/react/RNSentryProfilingTest.kt new file mode 100644 index 0000000000..09266d18d8 --- /dev/null +++ b/packages/core/RNSentryAndroidTester/app/src/test/java/io/sentry/react/RNSentryProfilingTest.kt @@ -0,0 +1,33 @@ +package io.sentry.react + +import org.junit.Test +import org.junit.runner.RunWith +import org.robolectric.RobolectricTestRunner + +/** + * Tests for the profiling lifecycle on the RNSentryModuleImpl. + * + * Note: the Hermes sampling profiler is a JNI-backed static API and cannot be loaded in a pure + * JVM unit-test environment. These tests exercise only the paths that do not cross the JNI + * boundary — specifically, the invalidate() guard that returns early when profiling is not + * active. If the guard were to regress and call into Hermes, the tests would fail with + * UnsatisfiedLinkError. + */ +@RunWith(RobolectricTestRunner::class) +class RNSentryProfilingTest { + @Test + fun `invalidate is a no-op on a fresh module`() { + val module = Utils.createRNSentryModuleWithMockedContext() + // isProfiling starts false; invalidate() should return early without touching + // HermesSamplingProfiler. If it didn't, the JNI call would throw UnsatisfiedLinkError. + module.invalidate() + } + + @Test + fun `invalidate is idempotent`() { + val module = Utils.createRNSentryModuleWithMockedContext() + module.invalidate() + module.invalidate() + module.invalidate() + } +} diff --git a/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java b/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java index a8e4b83b02..93eb2cea4c 100644 --- a/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java +++ b/packages/core/android/src/main/java/io/sentry/react/RNSentryModuleImpl.java @@ -831,17 +831,22 @@ public WritableMap startProfiling(boolean platformProfilers) { public WritableMap stopProfiling() { final boolean isDebug = ScopesAdapter.getInstance().getOptions().isDebug(); final WritableMap result = new WritableNativeMap(); + // Claim cleanup ownership atomically. If invalidate() got here first, it already + // disabled Hermes and collected the Android profiler; there's nothing to return. + if (!isProfiling.compareAndSet(true, false)) { + result.putString("error", "Profiling not active"); + return result; + } + // Take sole ownership of the AndroidProfiler reference so a concurrent invalidate() + // cannot also call endAndCollect() on the same instance. + final AndroidProfiler profiler = androidProfiler.getAndSet(null); File output = null; try { AndroidProfiler.ProfileEndData end = null; - // Snapshot the reference locally so a concurrent invalidate() cannot null the field - // between the null-check and the endAndCollect() call. - final AndroidProfiler profiler = androidProfiler.get(); if (profiler != null) { end = profiler.endAndCollect(false, null); } HermesSamplingProfiler.disable(); - isProfiling.set(false); output = File.createTempFile( @@ -904,21 +909,32 @@ public void invalidate() { // sees null and skips its own endAndCollect() on the same instance. final AndroidProfiler profiler = androidProfiler.getAndSet(null); + // Crash-critical: disable the Hermes sampler first. disable() synchronously joins the + // timer thread, so once it returns no pthread_kill can fire after the JS thread is + // joined. Isolated in its own try/catch so a failure in the Android profiler cleanup + // below cannot skip this call. try { - if (profiler != null) { + HermesSamplingProfiler.disable(); + logger.log(SentryLevel.INFO, "Stopped Hermes sampling profiler on React instance destroy."); + } catch (Throwable e) { // NOPMD - Guards the JNI boundary; does not catch native SIGABRT. + logger.log(SentryLevel.WARNING, "Failed to stop Hermes sampling profiler on teardown: " + e); + } + + // Android platform profiler is independent of Hermes; a failure here must not leak + // back into the Hermes shutdown above. The profile is being discarded; delete the + // trace file to avoid leaking it in cacheDir. + if (profiler != null) { + try { final AndroidProfiler.ProfileEndData end = profiler.endAndCollect(false, null); - // The profile is being discarded; delete the trace file to avoid leaking in cacheDir. if (end != null && end.traceFile != null) { try { end.traceFile.delete(); } catch (Throwable ignored) { // NOPMD - File cleanup is best-effort. } } + } catch (Throwable e) { // NOPMD - Android profiler cleanup is best-effort on teardown. + logger.log(SentryLevel.WARNING, "AndroidProfiler cleanup failed during invalidate: " + e); } - HermesSamplingProfiler.disable(); - logger.log(SentryLevel.INFO, "Stopped Hermes sampling profiler on React instance destroy."); - } catch (Throwable e) { // NOPMD - Guards the JNI boundary; does not catch native SIGABRT. - logger.log(SentryLevel.WARNING, "Failed to stop Hermes sampling profiler on teardown: " + e); } } From 1e2b54f428bdc8e04b97fa60650374710257a507 Mon Sep 17 00:00:00 2001 From: Antonis Lilis Date: Fri, 24 Apr 2026 10:24:35 +0200 Subject: [PATCH 5/5] Update changelog --- CHANGELOG.md | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ce209666ff..b8350d4fe8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,12 @@ > make sure you follow our [migration guide](https://docs.sentry.io/platforms/react-native/migration/) first. +## Unreleased + +### Fixes + +- Stop the Hermes sampling profiler on React instance teardown to prevent `pthread_kill` SIGABRT when the JS thread is torn down with profiling active ([#6035](https://github.com/getsentry/sentry-react-native/pull/6035)) + ## 8.9.1 ### Features @@ -25,7 +31,6 @@ - Fix `_experiments.enableUnhandledCPPExceptionsV2` being silently ignored on iOS ([#6014](https://github.com/getsentry/sentry-react-native/pull/6014)) - Check `captureReplay` return value in iOS bridge to avoid linking error events to uncaptured replays ([#6008](https://github.com/getsentry/sentry-react-native/pull/6008)) - Report the expected properties file path and any missing keys when using `flavorAware` on Android, instead of failing with an opaque `Illegal null value provided in this collection` error ([#6031](https://github.com/getsentry/sentry-react-native/pull/6031)) -- Stop the Hermes sampling profiler on React instance teardown to prevent `pthread_kill` SIGABRT when the JS thread is torn down with profiling active ([#6035](https://github.com/getsentry/sentry-react-native/pull/6035)) ### Dependencies