1793736 - Use AbortSignal.reason in Fetch code

Status: RESOLVED FIXED

(Core :: Networking, enhancement, P3)

Description

[nidhijaju]

Expected results:

In https://github.com/whatwg/fetch/pull/1343, the Http-network fetch and fetch() method have been updated to use AbortSignal's abort reason (or a serialized version of it) when erroring streams and aborting the fetch() call, rather than unconditionally using an AbortError. This change also includes using the abort reason to abort the fetch controller.

There are some related changes in the Service Worker spec in https://github.com/w3c/ServiceWorker/pull/1655. When a service worker intercepts a fetch request, the signal's abort reason is passed to the service worker to abort the fetch controller and signal abort in the Handle Fetch algorithm.

The same change needs to be made to the Firefox implementation.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Core::Networking' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 2

[Sunil Mayya]

Currently the code in dom/fetch/ throw DOM abort error without passing the abort reason. The AbortSignalImpl class which used to pass the abort signal already has a field that could support the reason field.
Assuming this field is always populated by the source of the signal, it should not be too much effort (1-2 weeks) to implement this feature.

However, the code changes are required in the service workers code and we need someone to look into that code to confirm the impacts/efforts.

Comment 3

[Valentin Gosu [:valentin] (he/him)]

However, the code changes are required in the service workers code and we need someone to look into that code to confirm the impacts/efforts.

Andrew, do you think you could assist Sunil with the service worker side of things? Or direct us to someone else? Thanks!

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

:saschanaz has already been involved in some abort signal propagation bug 1740522 (corresponding to https://github.com/w3c/web-locks/pull/86); I've also cc'ed Eden.

Comment 5

[Kagami Rosylight [:saschanaz] (they/them)]

Talked with Eden about this. It seems the main task is to let InterceptedHttpChannel receive an abort reason when being canceled, and the abort information will not be touched by service worker itself in the current implementation. Thus, for now the service worker shouldn't block this work item.

(Please correct me if I understood wrong!)

Comment 6

[Sunil Mayya]

Attachment: Bug 1793736 - Include abort signal reason for fetch. r=#necko

Comment 7

[Sunil Mayya]

Attachment: WIP: Bug 1793736 - Fix crash and abort reason tests

Depends on D161669

Comment 8

[Sunil Mayya]

I have implemented abort signal reason propagation in fetch code. The WPT tests with regard to fetch abort reason is passing.
However, there are still some failures in tests related to service workers. I think the service worker is not able to intercept the abort signal for fetch request. I found the following comment in ServiceWorkerOp.cpp

 /**
   * Step 3: create the public DOM Request object
   * TODO: this Request object should be created with an AbortSignal object
   * which should be aborted if the loading is aborted. See but 1394102.
   */
  RefPtr<Request> request =
      new Request(globalObjectAsSupports, internalRequest.clonePtr(), nullptr);
  MOZ_ASSERT_IF(internalRequest->IsNavigationRequest(),
                request->Redirect() == RequestRedirect::Manual);

From the above comment I think the implementation to follow abort signal is missing.
Hence, we need to resolve Bug 1394102 to fix WPT failures for service workers.

:Saschanaz, Andrea could you please confirm if my observation is correct?

Comment 9

[Kagami Rosylight [:saschanaz] (they/them)]

I don't think :baku works on Fetch/SW anymore. Canceling NI per https://bugzilla.mozilla.org/show_bug.cgi?id=1394102#c30.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/133c9084af25
Include abort signal reason for fetch. r=necko-reviewers,valentin,tschuster

Comment 11

[Atila Butkovits]

Backed out for causing failures at browser_navigationPreload_read_after_respondWith.js.

Backout link: https://hg.mozilla.org/integration/autoland/rev/10865595c6c165011749122af8f920516cc356f9

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&selectedTaskRun=FyuPiZ5bTrWrnUGRyd_ckg.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&revision=133c9084af25c6634dc4f499d3870c509e4b1b25

Failure log: https://treeherder.mozilla.org/logviewer?job_id=397232624&repo=autoland&lineNumber=3873

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fb8072c22d7a
Include abort signal reason for fetch. r=necko-reviewers,valentin,tschuster

Comment 13

[Serban Stanca [:SerbanS]]

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