Skip to content

fix(abort): throwIfAborted() actually throws the abort reason#316

Closed
watilde wants to merge 1 commit into
bytecodealliance:mainfrom
watilde:fix/abort-signal-throw-if-aborted
Closed

fix(abort): throwIfAborted() actually throws the abort reason#316
watilde wants to merge 1 commit into
bytecodealliance:mainfrom
watilde:fix/abort-signal-throw-if-aborted

Conversation

@watilde

@watilde watilde commented Jun 27, 2026

Copy link
Copy Markdown

AbortSignal.prototype.throwIfAborted() set a pending exception via JS_SetPendingException() but then returned true. The JSNative contract requires returning false whenever a pending exception is set, so the throw was not propagated: in release builds the method silently returned undefined instead of throwing, and in debug builds it tripped SpiderMonkey's pending-exception assertion.

Return false after setting the exception, matching every other throwing native in this file. Per the DOM spec, throwIfAborted() must throw the signal's abort reason when the signal is aborted.

Add an integration test covering: throwing the given reason, the default AbortError, AbortSignal.abort(reason), and the not-aborted no-op case.

https://dom.spec.whatwg.org/#dom-abortsignal-throwifaborted

AbortSignal.prototype.throwIfAborted() set a pending exception via
JS_SetPendingException() but then returned true. The JSNative contract
requires returning false whenever a pending exception is set, so the
throw was not propagated: in release builds the method silently returned
undefined instead of throwing, and in debug builds it tripped
SpiderMonkey's pending-exception assertion.

Return false after setting the exception, matching every other throwing
native in this file. Per the DOM spec, throwIfAborted() must throw the
signal's abort reason when the signal is aborted.

Add an integration test covering: throwing the given reason, the default
AbortError, AbortSignal.abort(reason), and the not-aborted no-op case.

https://dom.spec.whatwg.org/#dom-abortsignal-throwifaborted

Signed-off-by: Daijiro Wachi <daijiro.wachi@gmail.com>
@tschneidereit

Copy link
Copy Markdown
Member

Hi @watilde, thank you for opening this and the other PRs! As a heads-up, I'm working on a from-scratch new implementation of StarlingMonkey over here. My hope is to get this to a state where it can replace what's here right now in a few weeks.

So depending on your use case, it might make sense to look at that. It's just now getting to a point where it's (barely) ready for contributions.

One reason why improvements here might still be useful is that, at least as currently planned, the new version will only support WASIp3, not p2 as the current one. Another is of course if you need these fixes now :)

In any case, I'll try to look at these soon, unless @andreiltd beats me to it :)

@andreiltd

Copy link
Copy Markdown
Member

Hey, Thanks for patches! I believe this fix is already in other pending PR: #310. I believe that wpt tests that the mentioned PRs enables gives us enough coverage as well.

@watilde

watilde commented Jul 1, 2026

Copy link
Copy Markdown
Author

That's great! Let me close this and watch #310. Thank you!

@watilde watilde closed this Jul 1, 2026
@watilde watilde deleted the fix/abort-signal-throw-if-aborted branch July 1, 2026 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants