Closed Bug 918763 Opened 6 years ago Closed 3 years ago

[XHR2] Does not concatenate multiple header values


(Core :: DOM: Core & HTML, defect, P3)






(Reporter: hsteen, Unassigned)





(3 obsolete files)

The test links above have since changed to:

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.
Attachment #8760556 - Flags: review?(jst)
Try run:

Here's a follow-up patch to fix the mochitest failure that uncovered.
Comment on attachment 8760556 [details] [diff] [review]

Redirecting to sicking as he'd be a better reviewer for this than me.
Attachment #8760556 - Flags: review?(jst) → review?(jonas)
Rebased and folded into one patch.
Attachment #8760556 - Attachment is obsolete: true
Attachment #8763098 - Attachment is obsolete: true
Attachment #8760556 - Flags: review?(jonas)
Attachment #8766847 - Flags: review?(jonas)
Comment on attachment 8766847 [details] [diff] [review]

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.
Attachment #8766847 - Flags: review?(jonas) → review+
> Why remove "content-transfer-encoding"?

It's not one of the forbidden headers in the fetch spec (

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.
: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.
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.
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1285036
Attachment #8766847 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.