Hallvord R. M. Steen [:hallvors] Reporter
	Description • 11 years ago

Test cases:
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/setrequestheader-case-insensitive.htm
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/setrequestheader-header-allowed.htm

	Thomas Wisniewski
	Comment 1 • 8 years ago

The test links above have since changed to:

http://w3c-test.org/XMLHttpRequest/setrequestheader-case-insensitive.htm
http://w3c-test.org/XMLHttpRequest/setrequestheader-header-allowed.htm

Here's a patch that allows these tests to pass. These were the changes that had to be made;

1) Remove Content-Transfer-Encoding from the list of forbidden request headers, since it's not on the XHR/Fetch specs' list (just Transfer-Encoding).
2) Allow setting multiple values ("merging") for headers internally considered "singletons", like Authorization.
3) Allow headers that are set to default values (ie User-Agent) to be over-ridden with the user-set headers, rather than merged with them (if they aren't forbidden ones, of course).
4) Fix the XHR logic which keeps track of the user-set headers in case a redirect must be made.
5) Remove an unused hashtable variable, mAlreadySetHeaders.

I'm guessing that some of these changes (1,2) may be controversial, as they will affect more than just XMLHttpRequests. However, if I'm reading the WhatWG specs correctly, the Fetch and WebSockets APIs should also get fixes 1 and 2 (and 3 as well), so this version of the patch still seemed worth pitching.

Also note that Chrome doesn't currently allow the overriding of User-Agent in XHRs, though the spec does not forbid it and hence it fails that test case.

	Thomas Wisniewski
	Comment 2 • 8 years ago

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cc3a3a46b89c&selectedJob=22477374

Here's a follow-up patch to fix the mochitest failure that uncovered.

	Johnny Stenback (:jst)
	Comment 3 • 8 years ago

Comment on attachment 8760556 [details] [diff] [review]
918763-fix-case-multivalue-and-permitted-name-issues-of-request-headers-in-xhrs.diff

Redirecting to sicking as he'd be a better reviewer for this than me.

	Thomas Wisniewski
	Comment 4 • 8 years ago

Rebased and folded into one patch.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 5 • 8 years ago

Comment on attachment 8766847 [details] [diff] [review]
918763-fix-case-multivalue-and-permitted-name-issues-of-request-headers-in-xhrs.diff

Review of attachment 8766847 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsContentUtils.cpp
@@ -7011,5 @@
>  {
>    static const char *kInvalidHeaders[] = {
>      "accept-charset", "accept-encoding", "access-control-request-headers",
>      "access-control-request-method", "connection", "content-length",
> -    "cookie", "cookie2", "content-transfer-encoding", "date", "dnt",

Why remove "content-transfer-encoding"?

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +2947,5 @@
> +        mModifiedRequestHeaders[isAlreadyUserSetIndex].value
> +      );
> +    } else {
> +      RequestHeader reqHeader = {
> +        nsCString(headerOverride ? *headerOverride : header),

Isn't headerOverride always null here?

@@ +2950,5 @@
> +      RequestHeader reqHeader = {
> +        nsCString(headerOverride ? *headerOverride : header),
> +        nsCString(value)
> +      };
> +      mModifiedRequestHeaders.AppendElement(reqHeader);

It might be easier to use two arrays instead. One with names and one with values. That way you can use nsTArray.indexOf to do a case-insensitive search for an already set header.

Up to you.

::: netwerk/protocol/http/nsHttpHeaderArray.cpp
@@ +51,5 @@
>      MOZ_ASSERT(!entry || variety != eVarietyRequestDefault,
>                 "Cannot set default entry which overrides existing entry!");
>      if (!entry) {
>          return SetHeader_internal(header, value, variety);
> +    } else if (merge) {

This change should be reviewed by a necko peer. Maybe :mayhemer.

At the very least I suspect that we need to ensure that when we parse a http-response, that we still don't merge headers which aren't singleton headers.

	Thomas Wisniewski
	Comment 6 • 8 years ago

> Why remove "content-transfer-encoding"?

It's not one of the forbidden headers in the fetch spec (https://fetch.spec.whatwg.org/#forbidden-header-name)

Also the web platform test setrequestheader-header-allowed specifically checks that it isn't blocked.


>This change should be reviewed by a necko peer. Maybe :mayhemer.
>At the very least I suspect that we need to ensure that when we parse a http-response, that we still don't merge headers which aren't singleton headers.

Given that note (and the array suggestion), maybe it would be better to use my seventh patch in bug 1285036 instead? It manages the XHR request headers differently, so the nsHttpHeaderArray code won't need a change. However, it's a bigger patch, because it's attempting to align the algorithms for XHR request headers more closely with the spec (so it covers more WPTs), not just solve this specific ticket's issue.

	Thomas Wisniewski
	Comment 7 • 8 years ago

:baku just r+'d the patch I mentioned in the last comment, so I'll hold off on this one for now. If checkin of that one sticks, then I'll mark this one as obsolete.

	Thomas Wisniewski
	Comment 8 • 8 years ago

The patch in bug 1285036 that I mentioned in the last two comments has landed, fixing these web platform tests, so I'm closing this ticket as a dupe.