fix: isolate crash reporter dialog on macOS + modernize build - #52
Merged
Conversation
This library is now checked out under Terasology's libs/ (composite build,
see docs/Using-Locally-Developed-Libraries.md) so an upcoming fix can be
developed and verified against it directly. Composite builds share a single
Gradle version across the outer build and all included builds, and
Terasology's own build currently runs under Gradle 9.6.1 (its wrapper is
pinned to 8.10.2, which doesn't run on newer JDKs) - so this project needed
to become Gradle-9-compatible before it could be used that way at all.
Converted all six Groovy build files to Kotlin DSL (build.gradle(.kts),
settings.gradle(.kts), cr-core/cr-terasology/cr-destsol build.gradle(.kts),
gradle/common.gradle(.kts)) and updated gradle-wrapper.properties to
9.6.1. Along the way, fixed/updated for Gradle 9 API removals:
- sourceCompatibility/targetCompatibility as bare top-level assignments
(backed by the removed JavaPluginConvention) moved into a proper java {}
block.
- `by ... creating` / `by ... registering` Kotlin DSL delegate syntax is a
hard error under Gradle 9.6's Kotlin DSL now, not just deprecated -
switched to `configurations.create(...)` / `tasks.register(...)`.
- gradle/common.gradle.kts is loaded via apply(from = ...) into each
subproject, which does NOT get the generated type-safe accessors a
project's own primary build script does (e.g. a bare `publishing { }`
block doesn't resolve there) - switched to the explicit
configure<PublishingExtension> { } form, which works in any script
context.
- The old `archives sourceJar`/`archives javadocJar` + custom Jar tasks in
cr-core relied on the legacy `archives` configuration from before
maven-publish existed, and never actually attached to the real
MavenPublication (which only does `from(components["java"])`). Replaced
with `java { withSourcesJar(); withJavadocJar() }`, which both is the
modern idiom and actually gets them into the publication.
- Removed the org.standardout.versioneye plugin (versioneye.com has been
shut down for years; the plugin is unmaintained and not expected to
resolve/function under a modern Gradle) and the com.github.spotbugs
plugin (4.6.0, predates Gradle 8/9 support; SpotBugs here was already
ignoreFailures = true, i.e. advisory-only, so dropping it rather than
chasing a compatible version + new Property-based extension API seemed
the better time/risk tradeoff for this pass).
- pmd.incrementalAnalysis was a plain boolean in the old plugin version;
the current API differs and this was an advisory-only ignoreFailures
toggle either way, so it was dropped rather than guessed at.
- Fixed a latent isAllowInsecureProtocol gap on cr-terasology's identical
"Terasology Artifactory" http:// repo declaration (cr-core's copy of the
same URL already had it set; Gradle enforces this opt-in now).
Known limitation: the Terasology Artifactory host used for checkstyle/pmd's
shared ruleset+suppressions (codeMetrics config in cr-core) and as a
fallback repo is currently unreachable (DNS resolution failure), so no
Gradle invocation against this project - standalone or via the Terasology
composite build - can get past the configuration phase until that host is
back, since the checkstyle {} block resolves it eagerly rather than lazily
inside a task action. Left the checkstyle/pmd/codeMetrics wiring in place
rather than removing it, since that infrastructure is expected to come back
online; this is why the following CrashReporter.java fix on this branch is
compiler-verified for Kotlin/Java syntax correctness but not yet build-
verified end to end.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…hread is set On macOS, a process launched with -XstartOnFirstThread (required by GLFW-based games such as Terasology and Destination Sol, so they can create their window at all) permanently claims the OS's single native UI thread for its own run loop. AWT's native AppKit toolkit needs that very same thread to initialize, and blocks forever waiting for a run loop turn that never comes - so report()'s Swing dialog can never appear in such a process, no matter which thread calls it from. Confirmed live via jstack on a real hung game process: the calling thread was stuck in sun.lwawt.macosx.LWCToolkit.initAppkit (native method). This was previously worked around downstream in Terasology (MovingBlocks/Terasology#5338) by spawning a subprocess from the caller's side. Per review feedback there, moving it into the library instead means every consumer (Terasology, Destination Sol) gets the fix for free just by depending on a new version - no caller-side special-casing needed. report() now checks whether the current JVM was actually launched with -XstartOnFirstThread (via RuntimeMXBean's input arguments) before doing anything different, so callers that don't need isolation - any non-macOS process, or a macOS process not using GLFW's launch requirement - keep showing the dialog in-process exactly as before, with no extra subprocess overhead. When isolation is needed, the reporter relaunches itself (CrashReporter. main(), reusing this class rather than adding new public API surface) in a plain, unconstrained JVM using the same classpath, where AWT initializes normally. A real Throwable can't cross a process boundary, so the class name and localized message are passed as short arguments and a best-effort equivalent is reconstructed on the other side via reflection (falling back to a generic Throwable with the class name folded into the message if the original class has no (String) constructor). This is sufficient for full fidelity: ErrorMessagePanel only ever reads getClass().getSimpleName() and getLocalizedMessage() for the dialog's header - the detailed trace content users see comes from scanning the log directory, not from the Throwable object itself. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…o end
The previous commit's Gradle 9/Kotlin DSL migration could not actually be
build-verified: cr-core's checkstyle/pmd config (and its own repository
declaration) still pointed at artifactory.terasology.org:8081, which no
longer resolves at all (DNS failure). Terasology's own build moved to
artifactory.terasology.io years ago (see terasology-repositories.gradle.kts)
and bumped its codeMetrics dependency to 2.2.0 - CrashReporter's build.gradle
was never updated to follow. Pointed cr-core's and cr-terasology's
"Terasology Artifactory" repository at the current host and bumped
codeMetrics to 2.2.0, matching Terasology exactly (that artifact was already
in the local Gradle cache from a prior Terasology build on this machine, so
this needed no network access to verify).
That surfaced three more issues, now that the build could actually run
end to end:
- checkstyle 2.2.0's checkstyle.xml uses Checkstyle's built-in
${config_loc}/suppressions.xml mechanism, which only resolves correctly
when the config is loaded from a real directory on disk. The old
resources.text.fromArchiveEntry(...) in-place read extracts each zip
entry to its own isolated temp location, so checkstyle.xml and
suppressions.xml never ended up as real siblings and config_loc couldn't
find it. Added an extractCodeMetrics Copy task (still sourced from the
same already-resolved codeMetrics configuration) and switched checkstyle
to configDirectory / pmd to ruleSetFiles pointing at the extracted
directory, matching the mechanism Terasology's own build already uses.
- Test execution failed with "Failed to load JUnit Platform" -
junit-platform-launcher is no longer auto-supplied by Gradle 9 and needs
to be declared explicitly (same fix Terasology's own build already
needed, see terasology-metrics.gradle.kts).
- pmdMain/pmdTest failed with ClassNotFoundException on
net.sourceforge.pmd.ant.PMDTask - the Gradle PMD plugin's own task
implementation needs pmd-ant alongside pmd-core/pmd-java, which
Terasology's build already declares but CrashReporter's didn't.
- cr-core's test task failed with "did not discover any tests to execute":
there are no @test methods anywhere in this project (InteractiveTestCases
is a manual runner, not an automated test), and Gradle 9 fails by default
on that as a safety check. Set failOnNoDiscoveredTests = false, which is
correct here rather than a workaround.
Also brought the quality-tool and test-stack versions in cr-core up to
match what Terasology's own build currently uses (checkstyle 6.17 -> 10.2,
PMD 5.4.1 -> 7.0.0-rc4, Guava 19.0 -> 31.1-jre, Mockito 2.7.22 -> 5.6.0,
SLF4J 1.7.21 -> 2.0.11, Logback 1.1.7 -> 1.4.14, JUnit 4.12 -> JUnit Jupiter
5.10.1 - trivial since there are no actual JUnit-authored tests to migrate),
and fixed the Apache HttpClient dependency, which was declared under the
wrong artifact id (org.apache.httpcomponents:httpcomponents-client, which
isn't the real published module) - corrected to httpclient:4.5.13, matching
Terasology's own declaration.
`gradle build` now passes end to end for all three subprojects (compile,
checkstyle, pmd, test, javadoc, packaging), standalone. Also regenerated
the Gradle wrapper properly via `gradle wrapper --gradle-version 9.6.1` now
that the build actually configures successfully (the previous commit's
wrapper update was a manual gradle-wrapper.properties edit only, since the
wrapper task itself couldn't run while the build was still broken).
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe project migrates Gradle configuration and wrapper scripts to Kotlin DSL and Gradle 9.6.1, updates CI Java selection, expands ChangesBuild and runtime updates
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CrashReporter
participant ProcessBuilder
participant CrashReporterMain
CrashReporter->>CrashReporter: evaluate process isolation
CrashReporter->>ProcessBuilder: start subprocess with reporting arguments
ProcessBuilder->>CrashReporterMain: launch CrashReporter.main(args)
CrashReporterMain->>CrashReporterMain: reconstruct throwable and report in process
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
The pipeline now needs a JDK 17-capable agent (Checkstyle 10.2, PMD 7.0.0-rc4, and the rest of the version bumps in this branch require it). Match the labeling convention already established for gestalt (MovingBlocks/gestalt@c4104e1, pushed on soloturn-gradle), which itself matches Terasology's own Jenkinsfile label pattern: append "&& java17" to the existing agent label rather than replacing it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
cr-core/src/main/java/org/terasology/crashreporter/CrashReporter.java (2)
151-161: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Class.forName(className)initializes before theThrowablecheck runs.The single-arg
Class.forName(String)overload loads and initializes the class (running static initializers) beforeisAssignableFromis ever checked, so any class name reaching this code executes its static initializer regardless of whether it turns out to be aThrowablesubtype. Since the class name/message originate from argv of a subprocess this same app spawns, actual exploitability is limited, but deferring initialization closes the exact "unsafe reflection" gap flagged by static analysis at low cost.🔧 Proposed hardening
private static Throwable reconstructThrowable(String className, String message) { try { - Class<?> throwableClass = Class.forName(className); + Class<?> throwableClass = Class.forName(className, false, CrashReporter.class.getClassLoader()); if (Throwable.class.isAssignableFrom(throwableClass)) { return (Throwable) throwableClass.getConstructor(String.class).newInstance(message); } } catch (ReflectiveOperationException e) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cr-core/src/main/java/org/terasology/crashreporter/CrashReporter.java` around lines 151 - 161, Update reconstructThrowable to load the class without initialization before calling Throwable.class.isAssignableFrom, using the appropriate Class.forName overload with initialization disabled and the current class loader. Preserve reflective construction for Throwable subtypes and the existing generic Throwable fallback.Source: Linters/SAST tools
106-128: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrevent
-XstartOnFirstThreadfrom being inherited by the reporter subprocess.
requiresProcessIsolation()detects-XstartOnFirstThreadfrom any JVM input source, butProcessBuilderstarts with a copy of the current environment. IfJAVA_TOOL_OPTIONSor_JAVA_OPTIONSinject that flag, the child JVM will relaunch the reporter with the same constraint, so the Swing dialog can still deadlock or fail silently while AWT/AppKit needs an un-claimed native thread.At minimum, keep stderr inheritance in
reportInSubprocess()so subprocess launch/runtime failures are not completely discarded.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cr-core/src/main/java/org/terasology/crashreporter/CrashReporter.java` around lines 106 - 128, Update reportInSubprocess to prevent the child JVM from inheriting -XstartOnFirstThread through JAVA_TOOL_OPTIONS or _JAVA_OPTIONS by removing that option from the subprocess environment before starting ProcessBuilder. Preserve the existing command and output handling, but keep stderr inherited rather than redirecting it to DISCARD so launch and runtime failures remain visible.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build.gradle.kts`:
- Around line 9-10: Pin the Gradle 9.6.1 distribution checksum in the
tasks.wrapper configuration in build.gradle.kts and the corresponding
distributionSha256Sum property in gradle/wrapper/gradle-wrapper.properties,
using the specified checksum
9c0f7faeeb306cb14e4279a3e084ca6b596894089a0638e68a07c945a32c9e14 so both wrapper
definitions remain consistent.
In `@cr-core/build.gradle.kts`:
- Line 16: Update the shared Gradle script application in
cr-core/build.gradle.kts:16, cr-destsol/build.gradle.kts:15, and
cr-terasology/build.gradle.kts:31 to resolve common.gradle.kts from each
module’s parent directory rather than rootDir, while preserving the existing
shared-script loading behavior.
In `@cr-destsol/build.gradle.kts`:
- Line 12: Update the project dependency in cr-destsol/build.gradle.kts at lines
12-12 and cr-terasology/build.gradle.kts at lines 24-24 to select
:libs:CrashReporter:cr-core when CrashReporter is embedded, while preserving the
existing root-level :cr-core path for non-embedded builds.
In `@gradle/common.gradle.kts`:
- Around line 25-44: Update the publishing repository configuration around
publishRepo to use the Terasology Artifactory HTTPS endpoint, remove
isAllowInsecureProtocol, and preserve the existing mavenUser/mavenPass
authentication wiring. Ensure the selected publish repository maps to a
repository available under the HTTPS endpoint rather than assuming the current
HTTP publishRepo keys are valid there.
In `@gradlew.bat`:
- Around line 48-68: Replace both "%COMSPEC%" /c exit 1 statements in the Java
discovery failure paths with exit /b 1 so the current gradlew batch script
terminates immediately when JAVA_HOME is missing or invalid.
---
Nitpick comments:
In `@cr-core/src/main/java/org/terasology/crashreporter/CrashReporter.java`:
- Around line 151-161: Update reconstructThrowable to load the class without
initialization before calling Throwable.class.isAssignableFrom, using the
appropriate Class.forName overload with initialization disabled and the current
class loader. Preserve reflective construction for Throwable subtypes and the
existing generic Throwable fallback.
- Around line 106-128: Update reportInSubprocess to prevent the child JVM from
inheriting -XstartOnFirstThread through JAVA_TOOL_OPTIONS or _JAVA_OPTIONS by
removing that option from the subprocess environment before starting
ProcessBuilder. Preserve the existing command and output handling, but keep
stderr inherited rather than redirecting it to DISCARD so launch and runtime
failures remain visible.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f277d354-a71e-4848-9855-16077165836b
⛔ Files ignored due to path filters (1)
gradle/wrapper/gradle-wrapper.jaris excluded by!**/*.jar
📒 Files selected for processing (15)
build.gradle.ktscr-core/build.gradlecr-core/build.gradle.ktscr-core/src/main/java/org/terasology/crashreporter/CrashReporter.javacr-destsol/build.gradlecr-destsol/build.gradle.ktscr-terasology/build.gradlecr-terasology/build.gradle.ktsgradle/common.gradlegradle/common.gradle.ktsgradle/wrapper/gradle-wrapper.propertiesgradlewgradlew.batsettings.gradlesettings.gradle.kts
💤 Files with no reviewable changes (5)
- settings.gradle
- gradle/common.gradle
- cr-terasology/build.gradle
- cr-core/build.gradle
- cr-destsol/build.gradle
6 tasks
…hread requiresProcessIsolation() checked ManagementFactory.getRuntimeMXBean() .getInputArguments() for "-XstartOnFirstThread" - confirmed live (both via a real `gradle game` run and a minimal repro with a plain `java -XstartOnFirstThread ...` invocation) that this flag never actually shows up in that list at all on this JDK. It's a special macOS launcher option consumed by the native launcher before the JVM populates its own argument list, so the check always returned false and every call silently fell through to the in-process path - reproducing the exact HeadlessException this whole fix was meant to prevent. -Djava.awt.headless=true, unlike the native flag, is a normal system property and reliably visible. It's already paired 1:1 with -XstartOnFirstThread in every caller that needs this (see exec.kt in Terasology, same pattern expected in Destination Sol), so "macOS and headless" is a safe, reliable proxy for the actual unobservable condition. A caller invoking this library realistically only does so when a real display exists - Terasology's own uncaught-exception handler already skips calling report() entirely when genuinely headless - so this isn't at risk of misfiring for true headless-server usage. Verified end to end this time: ran CrashReporter.main() directly under `-XstartOnFirstThread -Djava.awt.headless=true` with the full classpath (cr-core + cr-terasology resources, matching what a real Terasology process's java.class.path would contain) and confirmed via jstack that the dialog actually comes up and stays healthy (AppKit Thread and AWT-EventQueue-0 both idle/responsive, LogUpdateWorker running normally, no deadlock). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
tested locally with crashreporter in terasology/lib folder and it works. jenkins needs still to be tested when artifactory.terasology.io wil be available. |
BenjaminAmos
previously requested changes
Jul 27, 2026
Matches the version bump landing in Terasology's own build for the same reason: 1.4.14/2.0.11 were behind current releases. This module doesn't use EvaluatorFilter or any other Janino-dependent feature, so no migration needed here beyond the version numbers. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: BenjaminAmos <24301287+BenjaminAmos@users.noreply.github.com>
Co-authored-by: BenjaminAmos <24301287+BenjaminAmos@users.noreply.github.com>
This was referenced Jul 28, 2026
Closed
fix: reset Logback once the real log folder is known + upgrade to 1.6.0
MovingBlocks/Terasology#5343
Merged
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cr-core/src/main/java/org/terasology/crashreporter/CrashReporter.java (2)
135-142: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftSurface failures from the isolated reporter process.
start()only confirms that the JVM was spawned; the child can still fail during argument parsing, class loading, or dialog initialization. Discarding both output streams and never observing the exit status makes the crash reporter silently disappear. Retain diagnostics and monitor the child asynchronously, without blocking the caller indefinitely.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cr-core/src/main/java/org/terasology/crashreporter/CrashReporter.java` around lines 135 - 142, Update the ProcessBuilder invocation in CrashReporter to retain the isolated reporter process’s diagnostics and monitor its completion asynchronously instead of discarding both streams and ignoring the exit status. Ensure failures during startup or reporter initialization are surfaced without blocking the caller indefinitely, while preserving the existing non-blocking launch behavior.
123-133: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftDo not pass exception details through command-line arguments.
messageand the absolute log path remain visible in process listings while the dialog is open. Exception messages may contain PII, credentials, or other sensitive data. Send these values through stdin or a permission-restricted temporary file instead; keep argv limited to non-sensitive protocol fields.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cr-core/src/main/java/org/terasology/crashreporter/CrashReporter.java` around lines 123 - 133, Update CrashReporter’s command construction to remove the exception message and absolute log path from argv, keeping only non-sensitive protocol fields such as the exception type and mode. Pass the message and log folder to the spawned CrashReporter process through stdin or a permission-restricted temporary file, and update the receiving path to read those values accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@cr-core/src/main/java/org/terasology/crashreporter/CrashReporter.java`:
- Around line 135-142: Update the ProcessBuilder invocation in CrashReporter to
retain the isolated reporter process’s diagnostics and monitor its completion
asynchronously instead of discarding both streams and ignoring the exit status.
Ensure failures during startup or reporter initialization are surfaced without
blocking the caller indefinitely, while preserving the existing non-blocking
launch behavior.
- Around line 123-133: Update CrashReporter’s command construction to remove the
exception message and absolute log path from argv, keeping only non-sensitive
protocol fields such as the exception type and mode. Pass the message and log
folder to the spawned CrashReporter process through stdin or a
permission-restricted temporary file, and update the receiving path to read
those values accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 57402a45-d94b-4cd1-bab5-cc1980ce2371
📒 Files selected for processing (2)
cr-core/src/main/java/org/terasology/crashreporter/CrashReporter.javagradle/common.gradle.kts
🚧 Files skipped from review as they are similar to previous changes (1)
- gradle/common.gradle.kts
Two follow-up review comments from @BenjaminAmos: - requiresProcessIsolation() gated on -Djava.awt.headless=true as a proxy for "this process was launched with -XstartOnFirstThread", since the native flag itself isn't visible via RuntimeMXBean. That proxy only holds for callers that pair the two flags together - Terasology does, but Destination Sol doesn't set -Djava.awt.headless=true at all, so it would have silently kept hitting the original hang this whole fix is for. Isolate unconditionally on macOS instead: the cost of an unnecessary subprocess for a caller that isn't actually -XstartOnFirstThread-constrained is small, and the dialog still shows correctly either way. - Added org.terasology.crashreporter.forceProcessIsolation, a system property that forces the subprocess path regardless of platform, so it can be exercised on non-macOS during development/testing. Only the property *name* is a static final constant here - the value itself is read fresh via Boolean.getBoolean() on every requiresProcessIsolation() call, nothing caches it at class-load time, so it can be set any time before report() is actually called: via a -D JVM launch flag, or programmatically via System.setProperty() at runtime. Documented this explicitly in the field's Javadoc since "static final" reads as if it might be a frozen/cached value at a glance. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
soloturn
force-pushed
the
fix/macos-process-isolation
branch
from
July 28, 2026 08:11
f723bd0 to
dd9878a
Compare
Four items from CodeRabbit's review, verified against current code before fixing (two other items it flagged - the $rootDir script-apply path and the :cr-core embedded project reference - turned out to be false positives: includeBuild() composite builds keep each included build's own rootDir and project-path namespace, they don't merge project trees, which is why this build has compiled correctly via those exact paths all along): - Pin the downloaded Gradle 9.6.1 distribution's SHA-256 checksum in both build.gradle.kts's wrapper task and gradle-wrapper.properties, so a compromised/corrupted distribution download fails closed instead of being silently accepted. - reconstructThrowable() used Class.forName(className) (single-arg), which loads *and initializes* the class - running its static initializers - before the Throwable check ever runs. Switched to the 3-arg overload with initialize=false so a named class's static initializer never runs unless it's actually going to be instantiated. - reportInSubprocess() discarded both stdout and stderr from the isolated subprocess, so a crash during its own startup, argument parsing, or dialog init was completely invisible - the reporter would just silently fail to appear with no diagnostic anywhere. Keep stderr inherited (stdout stays discarded - that's just the dialog's own). - The subprocess also inherited the full environment by default, including JAVA_TOOL_OPTIONS/_JAVA_OPTIONS - if either happens to carry -XstartOnFirstThread (every JVM reads these automatically at startup), the isolated subprocess would face the exact same thread-0 constraint this whole isolation mechanism exists to escape. Strip both before starting it. Not addressed here: CodeRabbit's "heavy lift" item about the exception message and log path being visible via argv/process listing while the subprocess is running - legitimate security/privacy concern, but a real protocol change (moving those two values to stdin or a temp file), better suited to its own follow-up than bundled into this PR. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
Author
|
Went through CodeRabbit's remaining open comments: Fixed (commit 9167411):
Not fixed - false positives (verified against actual
Deliberately deferred, not bundled into this PR:
The two open review comments about unconditional macOS isolation and the |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cr-core/src/main/java/org/terasology/crashreporter/CrashReporter.java`:
- Around line 151-152: Update the child-process environment cleanup in
CrashReporter to also remove the JDK_JAVA_OPTIONS variable alongside
JAVA_TOOL_OPTIONS and _JAVA_OPTIONS, preventing Java launcher options from being
inherited by the isolated reporter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b41b9d34-7c5e-4b98-b301-9a216a624a4e
📒 Files selected for processing (3)
build.gradle.ktscr-core/src/main/java/org/terasology/crashreporter/CrashReporter.javagradle/wrapper/gradle-wrapper.properties
🚧 Files skipped from review as they are similar to previous changes (2)
- gradle/wrapper/gradle-wrapper.properties
- build.gradle.kts
Comment on lines
+151
to
+152
| processBuilder.environment().remove("JAVA_TOOL_OPTIONS"); | ||
| processBuilder.environment().remove("_JAVA_OPTIONS"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
ast-grep outline cr-core/src/main/java/org/terasology/crashreporter/CrashReporter.java --items all
sed -n '143,155p' cr-core/src/main/java/org/terasology/crashreporter/CrashReporter.java
if [[ "${JDK_JAVA_OPTIONS:-}" == *"-XstartOnFirstThread"* ]]; then
echo "JDK_JAVA_OPTIONS can reintroduce -XstartOnFirstThread in the child JVM."
fiRepository: MovingBlocks/CrashReporter
Length of output: 1809
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
sed -n '1,190p' cr-core/src/main/java/org/terasology/crashreporter/CrashReporter.java
python3 - <<'PY'
from pathlib import Path
p = Path('cr-core/src/main/java/org/terasology/crashreporter/CrashReporter.java')
text = p.read_text()
checks = {
"removes JAVA_TOOL_OPTIONS": 'processBuilder.environment().remove("JAVA_TOOL_OPTIONS");' in text,
"removes _JAVA_OPTIONS": 'processBuilder.environment().remove("_JAVA_OPTIONS");' in text,
"removes JDK_JAVA_OPTIONS": 'processBuilder.environment().remove("JDK_JAVA_OPTIONS");' in text,
"isolates startup-on-first-thread note": "-XstartOnFirstThread" in text,
}
for k, v in checks.items():
print(f"{k}: {v}")
PYRepository: MovingBlocks/CrashReporter
Length of output: 9024
🌐 Web query:
Java 17 manual JAVA JDK_JAVA_OPTIONS environment variable java command
💡 Result:
The JDK_JAVA_OPTIONS environment variable is a tool used to prepend command-line arguments to the java launcher in Java 17 and later versions [1][2]. When this variable is set, the java launcher automatically includes these options before any arguments explicitly passed on the command line [1][3]. Key characteristics and usage guidelines include: Mechanism: The content of JDK_JAVA_OPTIONS is treated as a list of arguments separated by whitespace [1][4]. These arguments are prepended to the command line, effectively behaving as if they were typed directly into the terminal before the other arguments [1][3]. Syntax and Formatting: - Whitespace is used as the argument separator [1][3]. - You can use single (') or double (") quotes to group arguments that contain spaces [1][3]. - The @-file syntax (e.g., @file1) is supported, consistent with standard java command-line usage [1][3]. - Wildcard expansion is not supported within this environment variable [1][3]. Restrictions: To prevent misuse, certain options are disallowed in JDK_JAVA_OPTIONS [1][3]. If you include them, the launcher will abort with an error message [1][3]. These include: - Options that specify the main class (such as -jar) [1][3]. - Options that cause the launcher to exit without executing the main class (such as -h or --help) [1][3]. Operational Behavior: - When JDK_JAVA_OPTIONS is utilized, the java launcher prints a message to stderr to notify you that the variable has been picked up [1][3]. - The variable is only interpreted by the java launcher, making it ideal for options specifically intended for the java startup command, distinct from other tools like javac or jar which may use different variables like JAVA_TOOL_OPTIONS [5][6]. Example: If you set the variable as follows: export JDK_JAVA_OPTIONS='-g @file1 -Dprop=value' Running the command: java -Xint @file2 Becomes equivalent to executing: java -g @file1 -Dprop=value -Xint @file2 [1][3]
Citations:
- 1: https://docs.oracle.com/en/java/javase/17/docs/specs/man/java.html
- 2: https://www.javacodegeeks.com/jdk_java_options-vs-java_tool_options.html
- 3: https://raw.githubusercontent.com/openjdk/jdk/refs/heads/master/src/java.base/share/man/java.md
- 4: https://man.archlinux.org/man/extra/jdk17-openjdk/java-openjdk17.1.en
- 5: https://stackoverflow.com/questions/52986487/what-is-the-difference-between-jdk-java-options-and-java-tool-options-when-using
- 6: https://github.com/brunoborges/jdk-env-vars
Also remove JDK_JAVA_OPTIONS from the child environment.
JDK_JAVA_OPTIONS is a Java 17+ launcher option that prepends JRE options to every java invocation, so it can reintroduce -XstartOnFirstThread and cause the isolated AWT/Swing reporter to hang.
Proposed fix
processBuilder.environment().remove("JAVA_TOOL_OPTIONS");
processBuilder.environment().remove("_JAVA_OPTIONS");
+processBuilder.environment().remove("JDK_JAVA_OPTIONS");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| processBuilder.environment().remove("JAVA_TOOL_OPTIONS"); | |
| processBuilder.environment().remove("_JAVA_OPTIONS"); | |
| processBuilder.environment().remove("JAVA_TOOL_OPTIONS"); | |
| processBuilder.environment().remove("_JAVA_OPTIONS"); | |
| processBuilder.environment().remove("JDK_JAVA_OPTIONS"); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cr-core/src/main/java/org/terasology/crashreporter/CrashReporter.java` around
lines 151 - 152, Update the child-process environment cleanup in CrashReporter
to also remove the JDK_JAVA_OPTIONS variable alongside JAVA_TOOL_OPTIONS and
_JAVA_OPTIONS, preventing Java launcher options from being inherited by the
isolated reporter.
3 tasks
Contributor
Author
|
Filed the deferred argv-security item as #54 for tracking. |
Contributor
Author
|
@BenjaminAmos it says you requested changes, but am not able to make out in the PR here what it is or how to address it further. |
BenjaminAmos
dismissed
their stale review
July 29, 2026 11:25
Issues have been resolved. This will be re-reviewed shortly.
BenjaminAmos
approved these changes
Jul 29, 2026
BenjaminAmos
left a comment
BenjaminAmos
left a comment
Contributor
There was a problem hiding this comment.
I have tested this and it appears to work with the override, so I will assume that it works on macOS too.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The actual fix: on macOS, a process launched with
-XstartOnFirstThread(required by GLFW-based games such as Terasology and Destination Sol, so they can create their window at all) permanently claims the OS's single native UI thread for its own run loop. AWT's native AppKit toolkit needs that very same thread to initialize, and blocks forever waiting for a run loop turn that never comes - soreport()'s Swing dialog could never appear in such a process, no matter which thread called it from. Confirmed live viajstackon a real hung Terasology process: the calling thread was stuck insun.lwawt.macosx.LWCToolkit.initAppkit(native method).-XstartOnFirstThreaditself isn't visible viaRuntimeMXBean's input arguments - it's consumed by the native launcher before the JVM's own argument list is populated - so it can't be detected directly. An earlier revision tried using-Djava.awt.headless=trueas a proxy signal, since Terasology always pairs the two flags together; that turned out to be an unreliable assumption once tested against a real launch (confirmed live: the headless-gated check never fired at all, reproducing the original hang). Per @BenjaminAmos's review, Destination Sol doesn't set-Djava.awt.headless=trueeither, so that proxy would have missed it there too even if it had worked.requiresProcessIsolation()now isolates unconditionally on macOS instead - the cost of an unnecessary subprocess for a caller that isn't actually-XstartOnFirstThread-constrained is small, and the dialog shows correctly either way.org.terasology.crashreporter.forceProcessIsolation, a system property that forces the subprocess path regardless of platform, so it can be exercised on non-macOS during development/testing (also from review feedback). Only the property name is astatic finalconstant - the value itself is read fresh viaBoolean.getBoolean()on everyrequiresProcessIsolation()call, nothing caches it at class-load time, so it can be set any time beforereport()is actually called: via a-DJVM launch flag, or programmatically viaSystem.setProperty()at runtime. Documented this explicitly in the field's Javadoc sincestatic finalreads as if it might be a frozen/cached value at a glance.CrashReporter.main(), reusing this class rather than adding new public API) in a plain, unconstrained JVM using the same classpath, where AWT initializes normally. A realThrowablecan't cross a process boundary, so the class name and localized message are passed as short arguments and a best-effort equivalent is reconstructed via reflection. This is sufficient for full fidelity:ErrorMessagePanelonly ever readsgetClass().getSimpleName()andgetLocalizedMessage()for the dialog's header - the detailed trace content users see comes from scanning the log directory, not from theThrowableobject itself.Build modernization, needed to develop/verify the fix at all: this library is checked out under Terasology's
libs/(composite build) so the fix above could be built and tested directly against a real Terasology process. Composite builds share one Gradle version across the outer build and all included builds, and Terasology's own build currently runs under Gradle 9.6.1 - so this project needed to become Gradle-9-compatible first.sourceCompatibility/targetCompatibilitymoved into a properjava {}block,by ... creating/by ... registeringdelegate syntax replaced withconfigurations.create(...)/tasks.register(...),gradle/common.gradle.kts's script-plugin context needed the explicitconfigure<PublishingExtension> {}form sinceapply(from = ...)-loaded scripts don't get the generated type-safe accessors a primary build script does).org.standardout.versioneyeplugin (versioneye.com has been shut down for years) andcom.github.spotbugs(4.6.0, predates Gradle 8/9 support; alreadyignoreFailures = true, i.e. advisory-only, so dropping it seemed a better time/risk tradeoff than chasing a compatible version + newProperty-based extension API).cr-core's "Terasology Artifactory" repository declaration (used for the shared checkstyle/pmd config zip) pointed atartifactory.terasology.org:8081, which no longer resolves at all - Terasology's own build moved toartifactory.terasology.ioyears ago. Updated to match, and bumped thecodeMetricsdependency to2.2.0to match Terasology's own version. The same stale-host issue also affectedgradle/common.gradle.kts's separate publish-target URL (a different config, easy to miss) - fixed per @BenjaminAmos's suggestion, along with removing the now-unnecessaryisAllowInsecureProtocol = truenow that it's HTTPS.codeMetricszip'scheckstyle.xmluses Checkstyle's built-in${config_loc}/suppressions.xmlmechanism, which only resolves when the config is loaded from a real directory on disk - the oldresources.text.fromArchiveEntry(...)in-place read extracts each zip entry to its own isolated temp location, socheckstyle.xmlandsuppressions.xmlnever ended up as real siblings. Added an extraction step (still sourced from the same already-resolved dependency) and switched toconfigDirectory/ruleSetFiles, matching the mechanism Terasology's own build already uses.cr-coreto match what Terasology's own build currently uses, and fixed the Apache HttpClient dependency, which was declared under the wrong artifact id (org.apache.httpcomponents:httpcomponents-client, not a real published module) - corrected tohttpclient, matching Terasology's declaration. The JUnit 4 -> 5 bump is trivial here: there are no actual@Test-annotated methods anywhere in this project (InteractiveTestCasesis a manual runner, not an automated test), so nothing needed migrating.gradle buildnow passes end to end for all three subprojects (compile, checkstyle, pmd, test, javadoc, packaging), verified standalone.Test plan
gradle buildpasses standalone forcr-core,cr-terasology,cr-destsol(compile, checkstyle, pmd, test, javadoc, packaging).libs/CrashReporter): a live process launched with-XstartOnFirstThread -Djava.awt.headless=truecorrectly spawns the subprocess and shows a healthy dialog (confirmed viajps/jstack-AppKit Thread/AWT-EventQueue-0both idle/responsive, no deadlock).-XstartOnFirstThread: confirm the dialog still appears in-process as before (no behavior change, no extra subprocess).Related
jstackevidence and review discussion this PR responds to.