Steps to reproduce:

Explainer: https://github.com/shaseley/abort-signal-any

Spec PR: https://github.com/whatwg/dom/pull/1152

Web platform tests PR (landed): https://github.com/web-platform-tests/wpt/pull/37434 ( renamed to remove 'tentative' in https://github.com/web-platform-tests/wpt/pull/39785)

Can you mentor me on this issue, I would like to pick this up and solve it

Hello Kagami, are you the right person to ask for some starting points for this issue?

Note: I labelled this as good-first-bug simply based on Olli's comment "it should be rather small thing to add". If there's a different opinion, happy to know!

Took a bit to read the spec change. The obvious part of tasks are:

1. Update dom/webidl/AbortSignal.webidl to match https://dom.spec.whatwg.org/#interface-AbortSignal (i.e. add _any())
2. Add AbortSignal::Any() in dom/abort/AbortSignal.h (and in the corresponding cpp file) that follows https://dom.spec.whatwg.org/#create-a-dependent-abort-signal.

For the less obvious part, I don't think we need to implement the whole bunch of changes that removes the follow algorithm, our AbortFollower setup is already a bit special as it also corresponds to "add an algorithm to an AbortSignal" steps. I think we should be able to:

1. Replace AbortFollower::mFollowingSignal to an array of signals (nsTArray<WeakPtr<AbortSignalImpl>> mSourceSignals, renaming to better match the spec)
2. Modify AbortFollower::Follow to not call Unfollow() and just add the signal to mSourceSignals.
   • which probably is not a straighforward job, we should check whether there's any existing case that this unfollow is actually happening.
3. Just iterate over signals and call Follow() inside the new AbortSignal::Any().

This way we don't have to replace callers of AbortFollower::Follow().

Tom, do you want to add some opinion here?

I don't have anything substantial to add, mostly because I have already paged out everything related to signals.

I found the special handling of "dependent" signals curious though, do we even have that in Gecko?

I think that basically corresponds to AbortSignalImpl::mFollowers, except there now is a differentiation between non-abortsignal followers and abortsignal ones. Sounds like it affects the order.

Edit: This is the old spec before any(): https://whatpr.org/dom/1152/fb0bf26.html#abortsignal-signal-abort

Isn't dependent signals a new thing, added when any() was added to the spec.
The spec pr was rather simple: https://github.com/whatwg/dom/pull/1152/files
We could and should try to match that as much as possible.

abortsignal-follow used abort algorithm before, now it's done via "dependant signals", which are now aborted after running all the abort algorithms of the "source signal", hence the order change. I'm not sure this is covered in WPT.

(In reply to Kagami [:saschanaz] (they/them) from comment #3)

1. Replace AbortFollower::mFollowingSignal to an array of signals (nsTArray<WeakPtr<AbortSignalImpl>> mSourceSignals, renaming to better match the spec)

I tried implementing this and found two problems:

1. The order of abort algorithms, then dispatch event, then abort dependent signals is covered in WPT. If we implement dependant signals as abort algorithms / AbortFollower, we cannot follow the spec around SignalAbort step 6 and fail the test.
2. AbortFollower::Signal() needs to be replaced using mSourceSignals. Most usages, like here, can be replaced with the first aborted signal in mSourceSignals. But I'm unsure what to do about Signal() here within fetch.

Maybe we can first implement dependant signals according to spec using mSourceSignals and mDependentSignals in AbortSignalImpl and in a follow up remove the follow algorithm.
One difficulty with this might be that for SignalAbort, steps 1-4 belong in AbortSignalImpl, step 5 in AbortSignal and step 6 again in AbortSignalImpl. Merging both classes or having AbortSignalImpl do the event handling too seems like much work. Something like a template method or step 6 being a separate method would make the code less clear.

On a second thought, I don't know why we separate AbortSignalImpl and AbortSignal, but dependent signals and step 6 could probably done by AbortSignal.

(In reply to Vincent Hilla [:vhilla] from comment #8)

1. The order of abort algorithms, then dispatch event, then abort dependent signals is covered in WPT. If we implement dependant signals as abort algorithms / AbortFollower, we cannot follow the spec around SignalAbort step 6 and fail the test.

Maybe we can first implement dependant signals according to spec using mSourceSignals and mDependentSignals in AbortSignalImpl and in a follow up remove the follow algorithm.

Nice that it's in WPT! There should be two sets to align the behavior, yes. (See comment #7)

2. AbortFollower::Signal() needs to be replaced using mSourceSignals. Most usages, like here, can be replaced with the first aborted signal in mSourceSignals. But I'm unsure what to do about Signal() here within fetch.

My impression for the use of AbortSignal in Fetch is that it doesn't strictly follow what the spec does, this needs some investigation. Removing the good first bug tag because of this. Perhaps as a separate prior work, though.

One difficulty with this might be that for SignalAbort, steps 1-4 belong in AbortSignalImpl, step 5 in AbortSignal and step 6 again in AbortSignalImpl. Merging both classes or having AbortSignalImpl do the event handling too seems like much work. Something like a template method or step 6 being a separate method would make the code less clear.

Yeah, the split happened for a reason (see bug 1478101) but that's Fetch specific, perhaps we could do something in Fetch to remove this hack?

This is now in Safari TP.

Hey Vincent, assigning this to you since you've had WIPs already.

Release Note Request (optional, but appreciated)
[Why is this notable]: New web API (AbortSignal.any)
[Affects Firefox for Android]: Yes
[Suggested wording]: Added support for AbortSignal.any
[Links (documentation, blog post, etc)]: https://developer.mozilla.org/en-US/docs/Web/API/AbortSignal/any_static

https://hg.mozilla.org/mozilla-central/rev/d27c32ae0908

Added to Fx124 nightly notes https://www.mozilla.org/en-US/firefox/124.0a1/releasenotes/ and the draft for the final release notes.

FF124 MDN docs work for this done in https://github.com/mdn/content/issues/32342. Since this is already documented, was just an MDN release note and update to the compatibility data.