Many tests in /cookies/http-state consistently time out on Taskcluster but not on BuildBot

[Hexcles]

https://staging.wpt.fyi/results/cookies/http-state?sha=b040cb2a07&product=firefox[experimental,taskcluster]&product=firefox[buildbot,experimental]&diff=true
https://staging.wpt.fyi/results/cookies/http-state?sha=b040cb2a07&product=chrome[experimental,taskcluster]&product=chrome[buildbot,experimental]&diff=true

Presumably, the tests are a bit slow. I'll try to give them long timeout.

[Hexcles]

Looks like the root cause is a Unicode error from wptserve when there are non-ASCII characters in headers (cookies in this case).

To reproduce:

1. Start wptserve and open localhost:8000
2. Set a cookie with non-ASCII characters

document.cookie="foo=你好"

3. Refresh the page

Traceback:

ERROR:web-platform-tests:Traceback (most recent call last):
  File "/usr/local/google/home/robertma/github/wpt/tools/wptserve/wptserve/server.py", line 544, in handle_one_request
    self.finish_handling_h1(request_line_is_valid)
  File "/usr/local/google/home/robertma/github/wpt/tools/wptserve/wptserve/server.py", line 229, in finish_handling_h1
    request = Request(self)
  File "/usr/local/google/home/robertma/github/wpt/tools/wptserve/wptserve/request.py", line 280, in __init__
    int(self.headers.get("Content-Length", 0)))
  File "/usr/local/google/home/robertma/github/wpt/tools/wptserve/wptserve/request.py", line 332, in headers
    self._headers = RequestHeaders(self.raw_headers)
  File "/usr/local/google/home/robertma/github/wpt/tools/wptserve/wptserve/request.py", line 385, in __init__
    headers = [maybedecode(items[header])]
  File "/usr/local/google/home/robertma/github/wpt/tools/wptserve/wptserve/request.py", line 363, in maybedecode
    return s.decode("ascii")
UnicodeDecodeError: 'ascii' codec can't decode byte 0xe4 in position 4: ordinal not in range(128)

cc @jgraham @gsnedders

[Hexcles]

Is there a spec/RFC that says what encoding HTTP headers should use? UTF-8?

[gsnedders]

A recipient MUST parse an HTTP message as a sequence of octets in an encoding that is a superset of US-ASCII [USASCII].

and

Historically, HTTP has allowed field content with text in the ISO-8859-1 charset [ISO-8859-1], supporting other charsets only through use of [RFC2047] encoding. In practice, most HTTP header field values use only a subset of the US-ASCII charset [USASCII]. Newly defined header fields SHOULD limit their field values to US-ASCII octets. A recipient SHOULD treat other octets in field content (obs-text) as opaque data.

So, uh, not really.

[Hexcles]

Okay... so, what do we do? I mean, hard crashing the server and indefinitely hanging the test are probably not we want in any case...

Why does wptserve need to decode all header strings?

[gsnedders]

Okay... so, what do we do? I mean, hard crashing the server and indefinitely hanging the test are probably not we want in any case...

I agree. 🙂

Why does wptserve need to decode all header strings?

That would be my thought. Is there any reason not to just give a bunch of bytes to the handler?

[gsnedders]

@annevk @mnot thoughts?

[annevk]

Looking at blame this is @FHorschig and @mikewest.

[gsnedders]

@annevk this isn't about that specific test, it's about the wptserve behaviour

[gsnedders]

@Hexcles actually is this just a regression from #11769? (cc/ @Ms2ger)

[Hexcles]

@gsnedders yes. I can confirm reverting that change fixes the problem.

[annevk]

Ah sorry, yes, either passing on bytes or applying https://infra.spec.whatwg.org/#isomorphic-encode.

[jgraham]

Not decoding stuff seems reasonable but it's going to need a full test run to find all the cases it breaks. Does someone have time to work on that?

[annevk]

It's probably more reasonable to do isomorphic decoding since consumers likely want strings anyway as they're easier to operate on.

[gsnedders]

@jgraham pretty sure this is just a regression from #11769 and we didn't use to decode at all?

@annevk we should probably expose the raw bytes somewhere, given there are cases of other encoding being used, no?

[annevk]

@gsnedders we could, but I don't think it's needed (there's no information loss with isomorphic). If you really wanted to reinterpret you could, but for tests that's typically not what you ought to be doing (you want exact comparisons on the return values).

[Hexcles]

Given that this is a regression and we don't run wptserve with Python3 in any critical places, shall we revert #11769 for now?

[gsnedders]

@Hexcles yeah, that was my kinda implicit suggestion before

any objection @Ms2ger?

[foolip]

This regression is blocking https://chromium-review.googlesource.com/c/chromium/src/+/1249141. Gecko automatically imported the change in mozilla/gecko-dev@0ad26e7 although I don't know if also broke the tests.

Per https://web-platform-tests.org/appendix/reverting.html I think reverting this back in #13204 (comment) would have been reasonable. Is #13246 the alternative fix? That's not a tiny change, can we revert first and combine the two commits for relanding?

[gsnedders]

#13248 is a PR to revert it.

[foolip]

OK, I've reopened and approved. @Ms2ger, can you take a look?