Skip to content

[wasm][coreclr] Update contract of SystemJS_ExecuteFinalizationCallback#125121

Closed
radekdoulik wants to merge 1 commit intodotnet:mainfrom
radekdoulik:clr-wasm-update-SystemJS_ExecuteFinalizationCallback-contract
Closed

[wasm][coreclr] Update contract of SystemJS_ExecuteFinalizationCallback#125121
radekdoulik wants to merge 1 commit intodotnet:mainfrom
radekdoulik:clr-wasm-update-SystemJS_ExecuteFinalizationCallback-contract

Conversation

@radekdoulik
Copy link
Member

After ManagedThreadBase::KickOff call was enabled again, the contract needs same treatment as in FinalizerThread::FinalizerThreadStart

After `ManagedThreadBase::KickOff` call was enabled again, the contract
needs same treatment as `FinalizerThread::FinalizerThreadStart`
@radekdoulik radekdoulik added this to the Future milestone Mar 3, 2026
@radekdoulik radekdoulik added the arch-wasm WebAssembly architecture label Mar 3, 2026
Copilot AI review requested due to automatic review settings March 3, 2026 14:34
@dotnet-policy-service
Copy link
Contributor

Tagging subscribers to 'arch-wasm': @lewing, @pavelsavara
See info in area-owners.md if you want to be subscribed.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates the CoreCLR finalizer callback contract for Browser/WASM so it matches the exception-trap behavior used by FinalizerThread::FinalizerThreadStart, now that ManagedThreadBase::KickOff is used again in this path.

Changes:

  • Adjust SystemJS_ExecuteFinalizationCallback’s CONTRACTL to be NOTHROW on TARGET_UNIX and THROWS otherwise, aligning with the existing finalizer thread entrypoint contract.

Comment on lines +52 to +56
#ifdef TARGET_UNIX // because UNINSTALL_UNHANDLED_MANAGED_EXCEPTION_TRAP is catching exceptions only on Unix. On Windows OS is handling that.
NOTHROW;
#else // TARGET_UNIX
THROWS;
#endif // TARGET_UNIX
Copy link

Copilot AI Mar 3, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In TARGET_BROWSER builds, TARGET_UNIX is always defined by the build (see clrdefinitions.cmake), so this #ifdef TARGET_UNIX/#else in the contract is effectively dead code and the inline comment about Windows is misleading in this context. Consider simplifying the contract to a single NOTHROW; (or otherwise keying off whether UNINSTALL_UNHANDLED_MANAGED_EXCEPTION_TRAP is active) to avoid an unreachable branch and reduce confusion.

Suggested change
#ifdef TARGET_UNIX // because UNINSTALL_UNHANDLED_MANAGED_EXCEPTION_TRAP is catching exceptions only on Unix. On Windows OS is handling that.
NOTHROW;
#else // TARGET_UNIX
THROWS;
#endif // TARGET_UNIX
NOTHROW;

Copilot uses AI. Check for mistakes.
@radekdoulik
Copy link
Member Author

nm, copilot is right, we don't need this change

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants