Remove default pref-off for lowercase header names in XHR.getAllResponseHeaders()

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P2
normal
RESOLVED FIXED
2 years ago
a month ago

People

(Reporter: shawnjohnjr, Assigned: twisniewski)

Tracking

(Depends on 1 bug, Blocks 2 bugs, {addon-compat, dev-doc-complete, site-compat})

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(1 attachment)

Follow-up per Bug 1370485 Comment 24:

"I think you're right. We might at some point hit compatibility issues of the opposite nature due to Chrome and Safari lowercasing, but waiting with lowercasing until Firefox 58's Nightly arrives seems good. And to be clear, I think we should keep the preference available throughout Firefox 58 so we can easily revert if something breaks. We can remove the preference itself in Firefox 59 or 60 or so."

Updated

2 years ago
Blocks: xhr
Priority: -- → P2
Target Milestone: --- → Future
Assignee: nobody → shuang
Target Milestone: Future → mozilla58
Assignee: shuang → nobody
Assignee: nobody → shawnjohnjr
Shawn! /me waves o/
(Assignee)

Comment 2

7 months ago
Hi Shawn, I was wondering if you're still planning on pushing this one across the finish line? (I wouldn't mind doing so if you're busy).
Flags: needinfo?(shawnjohnjr)
I can't make any try push, it seems my account suspended :(
Reset assignee.
Assignee: shawnjohnjr → nobody
Flags: needinfo?(shawnjohnjr)

Comment 4

7 months ago
Shawn, apparently that happens automatically after a period of inactivity. I think it should be pretty trivial for you to get it enabled again, following the guidelines at https://www.mozilla.org/en-US/about/governance/policies/commit/.
(Assignee)

Comment 5

7 months ago
I'm doing a try-push here, in the interests of helping this along: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfe48ffef0a2fcbfffb3bae5640c1dfdefb23c3a
(Assignee)

Comment 6

7 months ago
Shawn, Anne, I don't see any unexpected failures in that try-run, so flipping the pref should be fine in that regard.

Were there any concerns we had left to address before flipping this pref that you're aware of, or should we go ahead? (I only recall us not wanting to try this out until after Firefox 57 was released).
Flags: needinfo?(shawnjohnjr)
Flags: needinfo?(annevk)
(In reply to Thomas Wisniewski [:twisniewski] from comment #6)
> Shawn, Anne, I don't see any unexpected failures in that try-run, so
> flipping the pref should be fine in that regard.
> 
> Were there any concerns we had left to address before flipping this pref
> that you're aware of, or should we go ahead? (I only recall us not wanting
> to try this out until after Firefox 57 was released).

If nothing goes wrong, we should remove that flag in the future, or now?

You can also get rid of meta config from web-platform-test.

https://searchfox.org/mozilla-central/source/testing/web-platform/meta/xhr/__dir__.ini

The original discussion is here:
https://github.com/whatwg/xhr/issues/146

Extra bug report from chromium:
https://bugs.chromium.org/p/chromium/issues/detail?id=749086

I don't see any further discussion from that bug. It should be fine.
Flags: needinfo?(shawnjohnjr)

Comment 8

7 months ago
We had some web compatibility fallout the last time around (~1 year ago), which is why we reverted the lowercasing behavior. Since then Chrome and Safari both shipped lowercasing behavior and it's seemingly compatible for them so we should try again, but keep the flag until we've done at least one stable release with lowercasing behavior to be safe.
Flags: needinfo?(annevk)
(Assignee)

Comment 9

7 months ago
Alright, then let's just flip the pref for now and we can remove it once we've confirmed that it can go away in a release or three.

(I did another quick try-run just in case, nothing interesting stands out: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f5848d78343ccbd6f70c67376af4d3d1c2ec5d7)
(Assignee)

Comment 10

7 months ago
change default pref to on for lowercase header names in XHR.getAllResponseHeaders
Comment on attachment 9011331 [details]
Bug 1398718 - change default pref to on for lowercase header names in XHR.getAllResponseHeaders; r?hsivonen

Henri Sivonen (:hsivonen) has approved the revision.
Attachment #9011331 - Flags: review+
(Assignee)

Comment 12

7 months ago
Alright, let's do this. Requesting check-in.
Keywords: checkin-needed

Comment 13

7 months ago
Pushed by ccoroiu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/cc0cb1e1912d
change default pref to on for lowercase header names in XHR.getAllResponseHeaders; r=hsivonen
Keywords: checkin-needed
(In reply to Stefan Hindli [:stefan_hindli] from comment #14)
> Failure log:
> https://treeherder.mozilla.org/logviewer.
> html#?job_id=201213744&repo=autoland&lineNumber=74

This doesn't seem to indicate anything wrong with this particular patch but I'll let Thomas respond in case I'm missing something.
Flags: needinfo?(overholt) → needinfo?(twisniewski)
(Assignee)

Comment 16

7 months ago
Yeah, I don't see how my patch would cause that, if it passed the normal try builds.
Flags: needinfo?(twisniewski)

Comment 17

7 months ago
Backout by rgurzau@mozilla.com:
https://hg.mozilla.org/mozilla-central/rev/79a94ed5a62d
Backed out changeset cc0cb1e1912d for task timeout after 3600 seconds bustages CLOSED TREE
(Assignee)

Comment 18

7 months ago
Do you think it would it be ok to try pushing this again in a day or two, Andrew? Or is there a strong reason to suspect that it's really this pref-flip that's causing those failures?
Flags: needinfo?(overholt)
Yes, I think we should try pushing this again. Thanks, Thomas.
Flags: needinfo?(overholt)
(Assignee)

Comment 20

7 months ago
Alright, I still don't see any troubling signs on a try-run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=76e29885054f0ed9f2c40ebb448b83be4826e5f6

Let's try checking in again.
Keywords: checkin-needed
Unfortunately, we're unable to land this bug due to this error from Phabricator:

"This diff does not have the proper author information uploaded to Phabricator. This can happen if the diff was created using the web UI, or a non standard client. The author should re-upload the diff to Phabricator using the "arc diff" command."

Can you please take a look?
Flags: needinfo?(twisniewski)
Keywords: checkin-needed
(Assignee)

Comment 22

7 months ago
Weird. I just reopened the Phabricator issue and did another arc diff.. hopefully that does the trick?
Flags: needinfo?(twisniewski) → needinfo?(cbrindusan)
Yes :)
Flags: needinfo?(cbrindusan)

Comment 24

7 months ago
Pushed by cbrindusan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f49f269d4fd0
change default pref to on for lowercase header names in XHR.getAllResponseHeaders; r=hsivonen

Comment 25

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f49f269d4fd0
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Assignee: nobody → twisniewski

Comment 27

6 months ago
This landed in Fx 64, right? Please set Target Milestone to avoid confusion.
Is there a bug filed to remove that flag?
(Assignee)

Updated

6 months ago
Blocks: 1504344
(Assignee)

Comment 28

6 months ago
I've just filed bug 1504344 to track the removal of the flag. Thanks for the reminder!
Target Milestone: mozilla58 → mozilla64
Note for docs team:

I've added a note to the Fx 64 rel notes:
https://developer.mozilla.org/en-US/docs/Mozilla/Firefox/Releases/64#APIs

When documenting this, it'll just need compat data updating and a note adding to the actual ref page to explain the change.
Documented:

I have added a note to the ref page:
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/getAllResponseHeaders

And submitted a PR to the compat data repo to add a data point about it:
https://github.com/mdn/browser-compat-data/pull/3114
Depends on: 1519456
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.