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

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P2)

Description

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

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."

Comment 1

[Andrew Overholt [:overholt]]

Shawn! /me waves o/

Comment 2

[Thomas Wisniewski [:twisniewski]]

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).

Comment 3

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

I can't make any try push, it seems my account suspended :(
Reset assignee.

Comment 4

[Anne (:annevk)]

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/.

Comment 5

[Thomas Wisniewski [:twisniewski]]

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

Comment 6

[Thomas Wisniewski [:twisniewski]]

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).

Comment 7

[Shawn Huang [:shawnjohnjr][:shuang] (as a happy gecko contributor)]

(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.

Comment 8

[Anne (:annevk)]

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.

Comment 9

[Thomas Wisniewski [:twisniewski]]

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)

Comment 10

[Thomas Wisniewski [:twisniewski]]

Attachment: Bug 1398718 - change default pref to on for lowercase header names in XHR.getAllResponseHeaders; r?hsivonen

change default pref to on for lowercase header names in XHR.getAllResponseHeaders

Comment 11

[Henri Sivonen (:hsivonen)]

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.

Comment 12

[Thomas Wisniewski [:twisniewski]]

Alright, let's do this. Requesting check-in.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/cc0cb1e1912d
change default pref to on for lowercase header names in XHR.getAllResponseHeaders; r=hsivonen

Comment 14

[Stefan Hindli [:stefan_hindli]]

Backed out changeset cc0cb1e1912d (Bug 1398718) for task timeout after 3600 seconds bustages CLOSED TREE

Failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed,busted,exception&classifiedState=unclassified&fromchange=787f9095543f9b60d22301d6c3ef06c4b47b1173&selectedJob=201213744

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=201213744&repo=autoland&lineNumber=74

Comment 15

[Andrew Overholt [:overholt]]

(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.

Comment 16

[Thomas Wisniewski [:twisniewski]]

Yeah, I don't see how my patch would cause that, if it passed the normal try builds.

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/79a94ed5a62d
Backed out changeset cc0cb1e1912d for task timeout after 3600 seconds bustages CLOSED TREE

Comment 18

[Thomas Wisniewski [:twisniewski]]

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?

Comment 19

[Andrew Overholt [:overholt]]

Yes, I think we should try pushing this again. Thanks, Thomas.

Comment 20

[Thomas Wisniewski [:twisniewski]]

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.

Comment 21

[Cristian Brindusan [:cbrindusan]]

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?

Comment 22

[Thomas Wisniewski [:twisniewski]]

Weird. I just reopened the Phabricator issue and did another arc diff.. hopefully that does the trick?

Comment 23

[Cristian Brindusan [:cbrindusan]]

Yes :)

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f49f269d4fd0
change default pref to on for lowercase header names in XHR.getAllResponseHeaders; r=hsivonen

Comment 25

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/f49f269d4fd0

Comment 26

[Kohei Yoshino]

Posted site compatibility note just in case: https://www.fxsitecompat.com/en-CA/docs/2018/xhr-getallresponseheaders-now-returns-header-names-in-lowercase/

Comment 27

[j.j.]

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

Comment 28

[Thomas Wisniewski [:twisniewski]]

I've just filed bug 1504344 to track the removal of the flag. Thanks for the reminder!

Comment 29

[Chris Mills [:cmills]]

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.

Comment 30

[Chris Mills [:cmills]]

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