1280189 - [Fetch] "NetworkError when attempting to fetch resource" exception when redirecting

Status: NEW

(Core :: DOM: Networking, defect, P3)

Description

[Christian Hersevoort]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.84 Safari/537.36

Steps to reproduce:

I created a simple script to reproduce this issue: https://github.com/ChristianHersevoort/fetch-firefox-issue

What it does is:
1. Start a Fetch-request 
2. Open a link or redirect to another page before fetch finishes





Actual results:

Get exception "NetworkError when attempting to fetch resource" on the fetch.catch() handler


Expected results:

The canceled fetch-request should not throw an exception

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

We purposely abort network when the page is navigated.

Are you saying the network should continue or we should just not reject the promise?  Do you see different behavior with XHR, for example?

Comment 2

[Patrick McManus [:mcmanus]]

is the sw the right component for fetch issues?

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Close.  Just DOM unless the site is actually using service workers.  I don't think it is in this case.

Comment 4

[Christian Hersevoort]

My initial assumption was a wrong. This issue is about the timing of the exception and not specifically about fetch. 

Let me reiterate:

What it does is:
1. Start async request (fetch or XHR)
2. Open a link / Redirect to another page
3. Fetch or XHR gets rejected
3b: Page shows error to the user (because a promise was rejected)
4. New page is opened

Expected results:
1. Start async request (fetch or XHR)
2. Open a link / Redirect to another page
3. New page is opened
4. Fetch or XHR gets rejected

Note: The XHR should be rejected AFTER the new 'old' page is closed, therefore it won't show the user an error message. From a developer point of view it's not possible to distinguish between an actual error or and expected redirect.

I've updated the example (https://github.com/ChristianHersevoort/fetch-firefox-issue). For clarity I've added an jQuery ajax (XHR) example as well. The jQuery example has the same bug.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Olli, what do you think?  Are we aborting our nsILoadGroup too early?  Should we be waiting for the new document to become active?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Per spec, the old document's network stuff gets stopped before the fetch of the new document _starts_.  See https://html.spec.whatwg.org/multipage/browsers.html#navigate step 11 which calls https://html.spec.whatwg.org/multipage/browsers.html#abort-a-document which in step 2 does:

  Cancel any instances of the fetch algorithm in the context of this Document, discarding any tasks queued
  for them, and discarding any further data received from the network for them.

So if there is a rejection that's going to happen here, it would happen before the fetch of the new document starts.  So the "expected results" from comment 4 are definitely wrong.  The only possibly spec-compliant things are the "what it does" list or the same thing without step 3/3b if the fetch being canceled is not supposed to reject the promise.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Oh, and at least at one point Chrome did _not_ implement the spec correctly in terms of aborting the document, as I understand.  I don't know whether they do now.

Last I checked, Safari did implement the spec correctly, but neither Safari nor WebKit nightly implements fetch, so it's hard to test this there.

Comment 8

[Adrian Aichner [:anaran]]

I suspect this issue is causing my progressive web app to no longer work offline.

It used to in nightly on android and linux, now both don't.

I am still investigating and would appreciate any guidance in working around this.

I am basically using a pattern like
https://github.com/GoogleChrome/sw-toolbox/blob/master/lib/strategies/fastest.js

But now I get the error described here instead of a fallback to the cached file when network is down.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Can you provide a link to your site or a demo?  Does this reproduce in developer edition or beta?

Comment 10

[Adrian Aichner [:anaran]]

(In reply to Ben Kelly [:bkelly] from comment #9)
> Can you provide a link to your site or a demo?  Does this reproduce in
> developer edition or beta?

My app is fairly involved and not public.

I saw this breakage in firefox for android 48.0b10 as well.

One other thing I noticed meanwhile is that offline access does not seem to work while devtools are open in that tab. I toggle devtools off with Ctrl+Shift+I and I can you the cached app offline.

I went from the "fastest" strategy I had used before to the approach used by
https://serviceworke.rs/offline-fallback_service-worker_doc.html
with caching added in the then clause:

this.addEventListener('fetch', function(event) {
  var request = event.request;
  var url = request.url;
  event.respondWith(
    fetch(event.request).then(function(response) {
      if (request.method == 'GET' && response && successResponses.test(response.status) &&
          request.url.match(this.registration.scope) &&
          (response.type == 'basic' || /\.(js|png|ttf|woff|woff2)/i.test(request.url) ||
           /fonts\.googleapis\.com/i.test(request.url))){
        caches.open(version).then(function(cache) {
          console.log(`put ${event.request.url} in ${version}`);
          cache.put(event.request, response.clone());
        });
      }
      return response;
    }).catch(err => {
      console.log(JSON.stringify(err, Object.getOwnPropertyNames(Error.prototype), 2));
      return caches.open(version).then(function(cache){
        console.log(`return cache.match(${url}) since fetch fails`);
        return cache.match(url);
      }).catch(err => {
        console.log(JSON.stringify(err, Object.getOwnPropertyNames(Error.prototype), 2));
      });
    })
  );
});

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Can you try to make a minimal test case?  Its really hard to investigate without being able to reproduce the error.  The linked SW demo works fine for me.

Also, are you creating iframes or anything?  This bug is about windows that are being closed.  Do you have such windows that are being closed?

Comment 12

[Adrian Aichner [:anaran]]

(In reply to Ben Kelly [:bkelly] from comment #11)
> Can you try to make a minimal test case?  Its really hard to investigate

I'll see whether I can file new coherent bugs, be it devtools or fetch implementation or whatever I may find.

Perhaps I find a public testcase which exposes the problem I was seeing.

> without being able to reproduce the error.  The linked SW demo works fine
> for me.

Yes, that works for me too and is what I based my reworked code on.

> 
> Also, are you creating iframes or anything?  This bug is about windows that
> are being closed.  Do you have such windows that are being closed?

No iframes, no closing windows.

I only found this bug based on the matching error type and message.

Comment 13

[johan.sigfrids]

I am also seeing this, and it is quite irritating. Fetch requests error out with "TypeError: NetworkError when attempting to fetch resource" whenever the user navigates away while there is an ongoing async fetch request. 

Since I am showing a an error message on NetworkError to users this means the user gets flashed with an error message every time they navigate if there is an ongoing fetch request in the background. Beyond flashing an incorrect error message to the user my sentry.io logs fill up with false NetworkError reports. 

Neither Chrome nor Safari does this. 

I am not sure what the ideal solution would be, but throwing an NetworkError when the user cancels the request is just wrong. At the very least it should do something that lets me distinguish an actual error from an canceled network request.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

Johan, can you work around the issue by having a small delay between fetch() rejection and showing the error?  Maybe you could suppress the visual error if you get pagehide event?  Not sure on the timing you are seeing.

Anne, is there anything in the spec AFAIK that would require the browser not to reject fetch() promises when the window navigates?

Comment 15

[Anne (:annevk)]

AFAYK I guess?

We added a keepalive member that you can set to true. I suspect the problem here is comment 7. That the timing when requests are aborted is off between Firefox and other browsers.

Comment 16

[johan.sigfrids]

Problem with pagehide is that it fires after the promise has been rejected and the error show. The promise is also rejected with NetworkError if the user simply hits stop to stop loading the page, in which case pagehide is never fired.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Now that we have abortable fetch we could possibly reject these fetch() calls with AbortError instead of NetworkError.  (This might already happen in FF 57+.) This would let you ignore the AbortError specifically, but report network problems.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

I tested and we do fire AbortError when a fetch() is aborted due to navigation.  I did:

1. open this site in FF57: https://fetch-abort-demo.glitch.me/
2. start fetching
3. open web console and run: window.location = 'http://example.com'
4. observe that the AbortError rejection is displayed briefly before the new page is shown

Johan, does this help with your issue?

Comment 19

[johan.sigfrids]

Yes, throwing a distinguishable AbortError on navigation does solve the problem for me.

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Thanks!  I'm going to close this for now then since the AbortError logic is landed and will be shipping in FF57.

Comment 21

[Christian Hersevoort]

This bug doesn't seem to be fixed.

I combined your example with mine and it's still broken. https://firefox-fetch-issue.glitch.me/

I made 3 cases:
1) Slow fetch + redirect to => Not working: Error flash + NetworkError
2) Slow fetch with try-catch + redirect => No error flash, but I still get a NetworkError in the console
3) Slow fetch with try-catch using async await => Error flash + NetworkError

Am I doing something wrong? I've tested it on FF57 and FF58.

The jQuery example also doesn't work in Firefox. (it works fine in Chrome)

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

Andrea, do you have any thoughts on comment 21?  I'm not going to have time to look at this.

Comment 23

[Stephen]

I believe I am still seeing this issue as well on Firefox 68. Here's my small repro: https://jsfiddle.net/jc4wku5a/

When I watch the debugger when executing this fiddle, I get:

starting
navigating away
caught TypeError: "NetworkError when attempting to fetch resource." TypeError

I see a TypeError instead of an AbortError

Comment 24

[Boris Zbarsky [:bzbarsky]]

So is the issue in comment 23 just that the rejection is with a TypeError, not an AbortError?

Comment 25

[Stephen]

(In reply to Boris Zbarsky [:bzbarsky, bz on IRC] from comment #24)

So is the issue in comment 23 just that the rejection is with a TypeError, not an AbortError?

Yes. comment 18 sounds like there was a change in FF57 that would cause an AbortError. Today in FF68, running the repro from comment 18 and my repro in comment 23 result in a TypeError, not AbortError.

Comment 26

[Boris Zbarsky [:bzbarsky]]

OK. Andrew, do we want to just mutate this to do the "fire AbortError" thing, or should that be a separate bug (and mark this one invalid, since we do in fact want to terminate fetches on navigation)?

I should note that calling window.stop() while fetching on https://fetch-abort-demo.glitch.me/ throws a TypeError in Chrome and Safari, so we may need to file bugs on them too... It does look to me like the Fetch spec says to reject with an AbortError DOMException in that case, right?

Comment 27

[Boris Zbarsky [:bzbarsky]]

Possibly-relevant spots are the hits on https://searchfox.org/mozilla-central/search?q=MSG_FETCH_FAILED&case=true in the tree and the caller at https://searchfox.org/mozilla-central/rev/053579099d936e26393ac10b809b14fb5841c0f0/dom/base/BodyStream.cpp#313 which is what I think the demo at https://fetch-abort-demo.glitch.me/ hits.

Comment 28

[Anne (:annevk)]

I think so, yes. Jake designed this and I thought he also added tests for this scenario, but maybe not.

Comment 29

[Rafael H]

In Firefox 94.0.2, reloading or clicking a link while a fetch request is being processed still throws a NetworkError. To reproduce, carry out any slow fetch and reload before it is complete.

Comment 30

[rafi-d]

Any progress with this here? I am getting this error a lot now (FF 100.01... )

Comment 31

[steven.woelk]

We have trouble with this too. At least this request could be marked as 'aborted' not as an 'error' to handle ist correctly.

Comment 32

[salman.mazhar]

Hi, Is this still being worked on or is there a work around to handles NetworkError for this case only? Our monitoring system is logging a lot of NetworkError errors because of this and we don't have a clear way of handling them without missing on the actual NetworkErrors.

Comment 33

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

Redirect a needinfo that is pending on an inactive user to the triage owner.
:jesup, since the bug has recent activity, could you have a look please?

For more information, please visit BugBot documentation.

Comment 34

[Randell Jesup [:jesup] (needinfo me)]

This still seems like it's something that we're following the spec on, and chrome & safari aren't, correct Olli?

Is there work to do here that would help with regards to AbortError? (see comment 25)

Comment 35

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

Attachment: 1280189.html

Attaching the repro in comment #23 as a static file.

The error is from mPromise->MaybeRejectWithTypeError<MSG_FETCH_FAILED>(); where the stack is:

xul.dll!mozilla::dom::MainThreadFetchResolver::OnResponseAvailableInternal(mozilla::SafeRefPtr<mozilla::dom::InternalResponse> aResponse) Line 761 (c:\Users\sasch\Documents\GitHub\gecko-dev\dom\fetch\Fetch.cpp:761)
xul.dll!mozilla::dom::FetchDriverObserver::OnResponseAvailable(mozilla::SafeRefPtr<mozilla::dom::InternalResponse> aResponse) Line 346 (c:\Users\sasch\Documents\GitHub\gecko-dev\dom\fetch\Fetch.cpp:346)
xul.dll!mozilla::dom::FetchDriver::FailWithNetworkError(nsresult rv) Line 943 (c:\Users\sasch\Documents\GitHub\gecko-dev\dom\fetch\FetchDriver.cpp:943)
xul.dll!mozilla::dom::FetchDriver::OnStartRequest(nsIRequest * aRequest) Line 979 (c:\Users\sasch\Documents\GitHub\gecko-dev\dom\fetch\FetchDriver.cpp:979)
xul.dll!nsCORSListenerProxy::OnStartRequest(nsIRequest * aRequest) Line 512 (c:\Users\sasch\Documents\GitHub\gecko-dev\netwerk\protocol\http\nsCORSListenerProxy.cpp:512)
xul.dll!mozilla::net::HttpChannelChild::DoNotifyListener() Line 1252 (c:\Users\sasch\Documents\GitHub\gecko-dev\netwerk\protocol\http\HttpChannelChild.cpp:1252)
xul.dll!mozilla::net::HttpAsyncAborter<mozilla::net::HttpChannelChild>::HandleAsyncAbort() Line 1119 (c:\Users\sasch\Documents\GitHub\gecko-dev\obj-x86_64-pc-windows-msvc\dist\include\mozilla\net\HttpBaseChannel.h:1119)
xul.dll!mozilla::net::HttpChannelChild::HandleAsyncAbort() Line 1135 (c:\Users\sasch\Documents\GitHub\gecko-dev\netwerk\protocol\http\HttpChannelChild.cpp:1135)
xul.dll!mozilla::net::HttpChannelChild::FailedAsyncOpen(const nsresult & status) Line 1158 (c:\Users\sasch\Documents\GitHub\gecko-dev\netwerk\protocol\http\HttpChannelChild.cpp:1158)
xul.dll!mozilla::net::HttpChannelChild::RecvFailedAsyncOpen::<lambda_24>::operator()() Line 1127 (c:\Users\sasch\Documents\GitHub\gecko-dev\netwerk\protocol\http\HttpChannelChild.cpp:1127)
xul.dll!std::invoke<`lambda at C:/Users/sasch/Documents/GitHub/gecko-dev/netwerk/protocol/http/HttpChannelChild.cpp:1125:13' &>(mozilla::net::HttpChannelChild::RecvFailedAsyncOpen::<lambda_24> & _Obj) Line 1524 (c:\Users\sasch\.mozbuild\vs\VC\Tools\MSVC\14.29.30133\include\type_traits:1524)
xul.dll!std::_Invoker_ret<void,1>::_Call<`lambda at C:/Users/sasch/Documents/GitHub/gecko-dev/netwerk/protocol/http/HttpChannelChild.cpp:1125:13' &>(mozilla::net::HttpChannelChild::RecvFailedAsyncOpen::<lambda_24> & _Func) Line 652 (c:\Users\sasch\.mozbuild\vs\VC\Tools\MSVC\14.29.30133\include\functional:652)
xul.dll!std::_Func_impl_no_alloc<`lambda at C:/Users/sasch/Documents/GitHub/gecko-dev/netwerk/protocol/http/HttpChannelChild.cpp:1125:13',void>::_Do_call() Line 822 (c:\Users\sasch\.mozbuild\vs\VC\Tools\MSVC\14.29.30133\include\functional:822)
xul.dll!std::_Func_class<void>::operator()() Line 869 (c:\Users\sasch\.mozbuild\vs\VC\Tools\MSVC\14.29.30133\include\functional:869)
xul.dll!mozilla::net::ChannelFunctionEvent::Run() Line 55 (c:\Users\sasch\Documents\GitHub\gecko-dev\obj-x86_64-pc-windows-msvc\dist\include\mozilla\net\ChannelEventQueue.h:55)
xul.dll!mozilla::net::ChannelEventQueue::RunOrEnqueue(mozilla::net::ChannelEvent * aCallback, bool aAssertionWhenNotQueued) Line 248 (c:\Users\sasch\Documents\GitHub\gecko-dev\obj-x86_64-pc-windows-msvc\dist\include\mozilla\net\ChannelEventQueue.h:248)
xul.dll!mozilla::net::HttpChannelChild::RecvFailedAsyncOpen(const nsresult & aStatus) Line 1124 (c:\Users\sasch\Documents\GitHub\gecko-dev\netwerk\protocol\http\HttpChannelChild.cpp:1124)
xul.dll!mozilla::net::PHttpChannelChild::OnMessageReceived(const IPC::Message & msg__) Line 532 (c:\Users\sasch\Documents\GitHub\gecko-dev\obj-x86_64-pc-windows-msvc\ipc\ipdl\PHttpChannelChild.cpp:532)
xul.dll!mozilla::dom::PContentChild::OnMessageReceived(const IPC::Message & msg__) Line 8818 (c:\Users\sasch\Documents\GitHub\gecko-dev\obj-x86_64-pc-windows-msvc\ipc\ipdl\PContentChild.cpp:8818)
xul.dll!mozilla::dom::ContentChild::OnMessageReceived(const IPC::Message & aMsg) Line 3765 (c:\Users\sasch\Documents\GitHub\gecko-dev\dom\ipc\ContentChild.cpp:3765)
xul.dll!mozilla::ipc::MessageChannel::DispatchAsyncMessage(mozilla::ipc::ActorLifecycleProxy * aProxy, const IPC::Message & aMsg) Line 1811 (c:\Users\sasch\Documents\GitHub\gecko-dev\ipc\glue\MessageChannel.cpp:1811)
...

And the spec says in step 12.3:

If response is a network error, then reject p with a TypeError and abort these steps.

So the error being TypeError is probably fine. But I think the core issue is that we get an error when Blink does not, rather than the type of the error?

Comment 36

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

Also the spec:

The user agent may terminate an ongoing fetch if that termination is not observable through script.

(Example) The user agent cannot terminate the fetch because the termination can be observed through the promise.

So we incorrectly expose the termination as a form of error.

Comment 37

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

(In reply to Kagami [:saschanaz] from comment #36)

So we incorrectly expose the termination as a form of error.

I guess that suggests that FetchDriver should specially handle NS_BINDING_ABORTED to eat the error and potentially neuter the promise if we think this is from coming from some variation of the load group canceling everything (also happens in the LoadGroup destructor, or maybe there are other ways for it to happen too?).

Comment 38

[Randell Jesup [:jesup] (needinfo me)]

Typo sent this to neck-next I think