Define how to handle bad content encoding

[MoritzKn]

I recognized some inconsistencies in the way different fetch implementations handle bad content encoding and couldn't find a clear definition of how to handle it in the spec.

I created a web-platform-tests to track the different behaviors:

Test case	Chrome	Edge	Firefox	Safari	Github Fetch Polyfill
Fetching a resource with bad content decoding should still resolve	Ok	Ok	Ok	Ok	rejects with TypeError ("TypeError: Network request failed")
Consuming the body of a resource with bad content decoding with arrayBuffer() should reject	Ok	Ok	Ok	resolves with an empty ArrayBuffer 1	Ok
arrayBuffer() rejects with error	TypeError	TypeError	AbortError	/	TypeError
Consuming the body of a resource with bad content decoding with blob() should reject	Ok	Ok	Ok	resolves with an empty Blob 1	Ok
blob() rejects with error	TypeError	TypeError	AbortError	/	TypeError
Consuming the body of a resource with bad content decoding with formData() should reject	OK	OK	OK	Ok	OK
formData() rejects with error	TypeError	TypeError	AbortError	"Not implemented"	TypeError
Consuming the body of a resource with bad content decoding with json() should reject	Ok	Ok	Ok	Ok 2	Ok
json() rejects with error	TypeError	TypeError	AbortError	SyntaxError	TypeError
Consuming the body of a resource with bad content decoding with text() should reject	OK	OK	OK	resolves with an empty string 1	OK
text() rejects with error	TypeError	TypeError	AbortError	/	TypeError

¹ In Safari the Promise returned by fetching a resource with bad content encoding only fulfills if it get executed immediately during the load. Otherwise it will never fulfill.
² Only rejects because an empty string isn't valid JSON ("Syntax Error: (DOM Exception 12): The string did not match the expected pattern.").

[TimothyGu]

Seems like the relevant spec section is this:

HTTP-network fetch

12. Run these substeps in parallel:

    1. …

       1. If one or more bytes have been transmitted from response’s message body, then:

          4. Set bytes to the result of handling content codings given codings and bytes.

          5. Enqueue a Uint8Array object wrapping an ArrayBuffer containing bytes to stream. If that threw an exception, terminate the ongoing fetch, and error stream with that exception.

       2. Otherwise, if the bytes transmission for response’s message body is done normally and stream is readable, then close stream and abort these in-parallel steps.

    2. …

    3. …

    4. Otherwise, if [the fetch has not been aborted and] stream is readable, error stream with a TypeError.

I guess there are a few things of interest here:

1. the algorithm is treating “handling content codings” will always succeed, which is not the case in practice

2. any TCP/TLS transmission errors are not accounted for, which may be because of the following TODO

   The exact layering between Fetch and HTTP still needs to be sorted through and therefore response represents both a response and an HTTP response here.

   On the other hand, we could reasonably at least specify the exact error type to throw in case of such transmission errors.

The error handling of the specific body consumption methods (text(), json(), etc.) seem to be pretty solid through the ReadableStream’s error mechanism (see “read all bytes” which those methods call upon through “consume body”).

[annevk]

Thanks for creating a test, that's great!

Safari and the polyfill seem like the most consistent. That is, if fetch() resolves, so should arrayBuffer(). But from those the polyfill seems like the best since if you transmit bytes and don't get them, you want that signaled somehow.

cc @tyoshino @wanderview @travisleithead @youennf

[TimothyGu]

I'd prefer the majority behavior (response fulfills but body consumption fails). This specific WPT has the broken content-encoding right from the beginning, but in practice the broken content-encoding may not happen until we are already megabytes into the response. If we don't buffer that much there's no way it can be discovered at fetch() time.

[annevk]

Aah I see. Yes, that makes sense. Thanks!

[MoritzKn]

I think there is also value in getting the response of a request with a corrupted body. For example you could check the status code or headers. I think failing because of a corrupted body before actually using the body would be patronizing.

[Mouvedia]

Why didn't you add the test to https://github.com/web-platform-tests/wpt/tree/master/fetch/api/basic ?
It's missing from http://w3c-test.org/fetch/api/basic/

[annevk]

Because in web-platform-tests/wpt#10542 I decided it should be elsewhere.

[Mouvedia]

@annevk I see that but in web-platform-tests/wpt@9d8baef you removed bad-content-encoding.html and didn't add it back.

cf http://w3c-test.org/fetch/content-encoding/

[annevk]

How is fetch/content-encoding/bad-gzip-body.any.js different in terms of coverage?

[Mouvedia]

@annevk I must be missing your point. I was under the impression that all tests had a corresponding html page. I managed to do my own, but it sure would have been more convenient to have the html page handy.

[sideshowbarker]

How is fetch/content-encoding/bad-gzip-body.any.js different in terms of coverage?

I was under the impression that all tests had a corresponding html page.

There is a corresponding HTML page (several corresponding HTML pages actually) — but instead of being under version control, the HTML is generated by the (self-hosted) web server for the test suite:

http://web-platform-tests.live/fetch/content-encoding/bad-gzip-body.any.html

See the related documentation here:

https://web-platform-tests.org/writing-tests/testharness.html#multi-global-tests