fix: free line number tables while class pins held

[jbachorik]

What does this PR do?:
Fixes SIGSEGV in Recording::cleanupUnreferencedMethods by moving the cleanup call inside finishChunk(), before GetLoadedClasses pins are released via DeleteLocalRef.

Motivation:
PROF-14545: Production crash (SIGSEGV) in Recording::cleanupUnreferencedMethods, first seen in dd-trace-java 1.56.1 after PR #327 introduced method-map cleanup.

Root cause: cleanupUnreferencedMethods() was called after finishChunk() returned, at which point the local references from GetLoadedClasses had already been released. On JVM implementations that free JVMTI-allocated line number table memory when a class is unloaded, the jvmti->Deallocate(_ptr) call inside SharedLineNumberTable::~SharedLineNumberTable() would access freed memory → SIGSEGV.

finishChunk() calls GetLoadedClasses at its start, which pins all loaded classes in memory for the duration. By moving cleanup to run before DeleteLocalRef releases those pins, we guarantee that Deallocate is called while the corresponding class is still loaded.

Additional Notes:
The fix is a 2-line change: finishChunk() gains a do_cleanup=false optional parameter, and switchChunk() passes do_cleanup=true instead of calling cleanupUnreferencedMethods() separately.

How to test the change?:
Added CleanupAfterClassUnloadTest which reproduces the crash scenario:

1. A dynamically-loaded class with line number tables appears in CPU profiling stack traces (profiler calls GetLineNumberTable and stores the table in method_map)
2. All references to the class and its class loader are dropped; System.gc() is called until the class is confirmed unloaded via WeakReference
3. AGE_THRESHOLD+1 dump cycles age the method out of method_map and trigger cleanup
4. The profiler must survive without SIGSEGV — verified by asserting it produced JFR output

Without the fix, this test will crash on JVM implementations that free JVMTI-allocated memory on class unload. With the fix, it reliably passes.

For Datadog employees:

• This PR doesn't touch any of that.
• JIRA: PROF-14545

[dd-octo-sts]

CI Test Results

Run: #25502881382 | Commit: 32538ed | Duration: 33m 29s (longest job)

✅ All 32 test jobs passed

Status Overview

JDK	glibc-aarch64/debug	glibc-amd64/debug	musl-aarch64/debug	musl-amd64/debug
8	-	✅	-	-
8-ibm	-	✅	-	-
8-j9	✅	✅	-	-
8-librca	-	-	✅	✅
8-orcl	-	✅	-	-
11	-	✅	-	-
11-j9	✅	✅	-	-
11-librca	-	-	✅	✅
17	✅	✅	-	-
17-graal	✅	✅	-	-
17-j9	✅	✅	-	-
17-librca	-	-	✅	✅
21	✅	✅	-	-
21-graal	✅	✅	-	-
21-librca	-	-	✅	✅
25	✅	✅	-	-
25-graal	✅	✅	-	-
25-librca	-	-	✅	✅

Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled

Summary: Total: 32 | Passed: 32 | Failed: 0

---

Updated: 2026-05-07 15:21:32 UTC

[jbachorik]

@codex review

[chatgpt-codex-connector]

💡 Codex Review

java-profiler/ddprof-test/src/test/java/com/datadoghq/profiler/memleak/CleanupAfterClassUnloadTest.java

Lines 94 to 98 in e8c5a9a

	long deadline = System.currentTimeMillis() + 8_000;
	while (loaderRef.get() != null && System.currentTimeMillis() < deadline) {
	System.gc();
	Thread.sleep(30);
	}

Assert the class actually unloads

In runs where this loader is not cleared before the 8-second deadline, the loop silently falls through and the test continues with the dynamic class still loaded. That means the later dump cycles exercise cleanup while the class is still pinned/live, so the regression can pass without reproducing the class-unload scenario it is meant to guard; please assert or use an assumption that loaderRef.get() == null before aging out the method.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[jbachorik]

Addressed — added assumeTrue(loaderRef.get() == null, …) immediately after the GC poll loop so the test skips rather than silently continuing when class unloading does not occur within the 8 s window. Using assumeTrue (vs assertTrue) because the CI matrix spans JVMs (J9, Graal, musl variants) where class unloading is non-deterministic — a hard failure would be noise, not signal.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b8f17cf30f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Stop the manually restarted profiler on abort

When this test aborts at the class-unload assumption or throws anywhere after the manual profiler.execute(...), the finally block only deletes files and leaves the profiler running. Because stopProfiler() was called before the manual start, AbstractProfilerTest.stopped remains true, so the @AfterEach cleanup also skips profiler.stop(). This can leave an active recording that makes subsequent tests fail or observe the wrong profiler state; the manual start should be paired with a stop in this finally path.

Useful? React with 👍 / 👎.