Reject aborted operations with the abort reason #176
Conversation
| 3. If <code>|options|.{{CredentialRequestOptions/signal}}</code> is [=AbortSignal/aborted=], | ||
| then return [=a promise rejected with=] | ||
| <code>|options|.{{CredentialRequestOptions/signal}}</code>'s [=AbortSignal/abort reason=]. |
There was a problem hiding this comment.
This seems fine, but we are not also addressing the final requirement enumerated by DOM's "3.3. Using AbortController and AbortSignal objects in APIs" which is:
Use the abort algorithms mechanism to observe changes to the AbortSignal object and do so in a manner that does not lead to clashes with other observers.
I'm not sure in practice this requirement definitely applies to this credman spec.
If it does apply, then following the example in the latter DOM spec section leads me to guess that if we add these abort steps to options.signal:
1. Set `abortedFlag` to true. // cause the overall alg to stop
2. Reject p with signal’s abort reason.
...and then also create abortedFlag in the main synchronous portion of our "Request a Credential" alg, and then check in a couple/few places in the in-parallel portion of the "Request a Credential" alg whether our abortedFlag is true, and if so then say "abort these steps" (we wouldn't need to do the promise rejection because the "abort steps" we added take care of it).
Lacking a more detailed example I am just sort of guessing about this. I've queried the DOM folks about it in any case.
There was a problem hiding this comment.
I worried about this, but I think it's covered by the use of options.signal in [[Create]] and [[DiscoverFromExternalSource]], as suggested by https://w3c.github.io/webappsec-credential-management/#implementation-extension.
There was a problem hiding this comment.
Okay, so if we decide to handle the signal being aborted in [[Create]] and [[DiscoveredFromExternalSource]], I think the changes on this PR are enough.
I don't think we need an abortedFlag, we can instead check if signal is aborted.
There was a problem hiding this comment.
I think this is good as-is in that it makes the correct improvements to the existing spec.
The existing spec has an existing problem, which is that it accesses main-thread-specific IDL objects like AbortSignals and Promises from "in parallel" sections. To address this you'd need to make changes like what @equalsJeffH is proposing, where instead of sharing an AbortSignal across threads (bad), you instead share a boolean abortedFlag variable (fine).
https://html.spec.whatwg.org/#event-loop-for-spec-authors contains more details on this general issue.
But this cross-thread-access problem exists in a lot of places in the spec, I believe. (E.g., just following a few links, "Collect Credentials from the credential store" is run from in-parallel, but it specifically states "Assert: r is a list of interface objects", which should be impossible as interface objects are a main-thread-only concept.) So I don't think fixing it needs to block this PR.
Reject aborted operations with the "abort reason" instead of AbortError.
Fixes #175.
Preview | Diff