Closed Bug 1293881 Opened 3 years ago Closed 3 years ago

Revert SetRequestHeader behavior so it doesn't lowercase incoming header names.

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: wisniewskit, Assigned: wisniewskit)

References

Details

(Keywords: site-compat)

Attachments

(1 file)

In bug 1285036, patch #7 changed the behavior of SetRequestHeader to match the current XHR specification. That is, it lowercased all incoming header names. That bug was intended only as a refactoring bug, plus there's no guarantee that this (behavioral) change will be web-compatible, so it was decided to revert it for now.

Here's a patch which does so. A try run is only showing unrelated intermittents so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=702f15cffc9a
Attachment #8779592 - Flags: review?(bugs)
We need this patch on which all branches? trunk and aurora? Could you set the tracking flags?
I'm still new here, so I'm not sure what tracking flags I would need to set here (or how to properly confirm which branches it might have landed on). But the patch only landed in central 6 days ago, so I don't think it's gone into any branches other than central and inbound. Which flags would I mark in that case?
Flags: needinfo?(bugs)
(In reply to Thomas Wisniewski from comment #2)
> I'm still new here, so I'm not sure what tracking flags I would need to set
> here (or how to properly confirm which branches it might have landed on).

You can always check what code is, e.g., in the mozilla-aurora repo:

https://dxr.mozilla.org/mozilla-aurora/source/devtools/client/netmonitor/test/browser_net_copy_as_curl.js

In this case, the devtools test is in state before your patch, i.e., the patch landed after the merge.
Blocks: 1269102
Thanks! Based on that it hasn't hit aurora yet, and is just in trunk/central. Do any tracking flags need to be set in that case?
I see, it landed to FF50, but was backed out couple of times.
Flags: needinfo?(bugs)
Comment on attachment 8779592 [details] [diff] [review]
make-SetRequestHeader-retain-case-of-header-names.diff

>   bool notAlreadySet = true;
>+  const nsCaseInsensitiveCStringComparator ignoreCase;
>   for (RequestHeader& header : mAuthorRequestHeaders) {
>-    if (header.name.Equals(lowercaseName)) {
>+    if (header.name.Equals(aName, ignoreCase)) {
And this is what the code looked like before the change?
Headers with different cases were treated as the header?
Attachment #8779592 - Flags: review?(bugs) → review+
Yes, that's how it behaved prior to my patches: it treats them case-insensitively, using the precise case from the first time the header is set (that is, setting the same header multiple times will not change the case, it always uses the first variant you set it with).
Alright, I don't see anything worrying on my try run and the patch still applies cleanly today, so I'm requesting check-in.
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4303f0b75997
Revert a behavioral change introduced in bug 1285036 patch 7: header names set by SetRequestHeader should retain their case instead of being lowercased as the current XHR spec specifies. r=jimb
Keywords: checkin-needed
Landed yesterday:
https://hg.mozilla.org/mozilla-central/rev/4303f0b75997
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.