fix(abort): throwIfAborted() actually throws the abort reason#316
fix(abort): throwIfAborted() actually throws the abort reason#316watilde wants to merge 1 commit into
Conversation
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>
|
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 :) |
|
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. |
|
That's great! Let me close this and watch #310. Thank you! |
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