1370485 - Disable lowercase header names in XHR.getAllResponseHeaders()

Status: RESOLVED FIXED

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

Description

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

Disable lowercase header names in XHR.getAllResponseHeaders()
Some sites and add-on are not ready for this change.

Comment 1

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

[Tracking Requested - why for this release]:

Comment 2

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

Currently most of broken sites directly use case-sensitive header name to access array of response headers.
Even site like store.nike.com Japanese version now is broken.
And based on the analysis in [1], we shouldn't under estimate the number of broken sites.

As discussed with Anne, it's better to wait for a couple of releases.

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=716994#c5

Comment 3

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

Attachment: Bug 1370485 - Do not return lowercased header names for getAllResponseHeaders()

Comment 4

[Julien Cristau [:jcristau]]

Tracking for 55 as webcompat issue

Comment 5

[Benson Wong [:mostlygeek]]

We hit this bug working on BuildHub, https://github.com/mozilla-services/buildhub/issues/98.

Comment 6

[Hsin-Yi Tsai (she/her) [:hsinyi]]

As :Baku is on vacation, would if be possible to ask other peers for review?

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Why aren't we just backing out the regressing patch?
Clearly the pref is enabling something not web compatible.

Comment 8

[Andrew Overholt [:overholt]]

If comment 7 doesn't work, I'd ask bkelly for review.

Comment 9

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

Comment on attachment 8875199 [details] [diff] [review]
Bug 1370485 - Do not return lowercased header names for getAllResponseHeaders()

Hi Olli,
The patch is to disable the regression patch bug 1348390 lowercased header name.

The original spec issue https://github.com/whatwg/xhr/issues/109 and https://github.com/whatwg/xhr/commit/ecce3904ace.

getAllResponseHeaders() is supposed to follow steps defined in the Fetch spec, including lowercasing all the header names it returns. Per XHR 4.6.5, make response’s header list sorted and lowercased. That fixes wpt tests XMLHttpRequest/getallresponseheaders-cl, getallresponseheaders.htm, getresponseheader-chunked-trailer.htm

Safari issue, reviewed, patch landed, but I'm not sure when they will release it.
https://bugs.webkit.org/show_bug.cgi?id=169286
http://trac.webkit.org/changeset/214252/webkit

Chrome issue, patch landed, and targets on M60:
https://bugs.chromium.org/p/chromium/issues/detail?id=716994

It seems to me no other browsers disagree the specification update and they are going to change their xhr implementation. HTTP/2 is lowercasing headers, it makes sense to me to go in that direction.

I can backout the regression patch if you prefer.
I understand we should reduce webcompat issues as much as we can. I just wonder when we should make this change or we should make change to xhr spec, make header names case-sensitive again to avoid webcompat issue?
Can you share ideas, how to proceed to resolve this situation?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

But why would we want to keep the code dealing with the pref which is clearly not web compatible?

And I'm now a bit lost here. Did other browsers manage to land the lowercase approach without regressions but we got tons of broken sites?

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8875199 [details] [diff] [review]
Bug 1370485 - Do not return lowercased header names for getAllResponseHeaders()

but ok, at least for now we can just disable.

Comment 12

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

(In reply to Olli Pettay [:smaug] from comment #10)
> But why would we want to keep the code dealing with the pref which is
> clearly not web compatible?
I hope xhr spec issue can address the webcompat risk sooner.
Anne said we should wait for a couple of releases.
> And I'm now a bit lost here. Did other browsers manage to land the lowercase
> approach without regressions but we got tons of broken sites?
Based on discussion and analysis in https://bugs.chromium.org/p/chromium/issues/detail?id=716994#c5, they choose to land that lowercase approach. As for Safari, I don't see much web compat risk discussion.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9819afb3f523
Do not return lowercased header names for getAllResponseHeaders(), r=smaug

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9819afb3f523

Comment 15

[Boris Zbarsky [:bzbarsky]]

Why didn't we report the web-incompatibility here to the spec issue?  Seems like if the spec is not web-compatible, then the spec should change...

Comment 16

[Anne (:annevk)]

How is this web incompatible? Thus far all the breakage seems outside the web. Having said that, we do have https://github.com/whatwg/xhr/issues/146 tracking this now.

Comment 17

[Boris Zbarsky [:bzbarsky]]

> Thus far all the breakage seems outside the web

There are multiple bugs on this bug's "Blocks" list that certainly look like "web" to me.

Comment 18

[Anne (:annevk)]

Hmm, I missed those. I'll update the standards issue.

Comment 19

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

(In reply to Anne (:annevk) from comment #18)
> Hmm, I missed those. I'll update the standards issue.
Shall I backout both lowercased and sorted? Or just remove lowercased?

webcompat has been reported in bug 1370812.
See: https://webcompat.com/issues/7028

Comment 20

[Boris Zbarsky [:bzbarsky]]

For now just lowercase, since that's what we've had web compat issues with, right?  Should see what happens in the spec issue.

Comment 21

[Anne (:annevk)]

I agree with bz. The specification issue is going nowhere fast. It seems that at least Safari 11 will always lowercase. Would be interesting to know if the websites fail in Safari somehow or manage to work due to different code paths...

Comment 22

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

(In reply to Anne (:annevk) from comment #21)
> I agree with bz. The specification issue is going nowhere fast. It seems
> that at least Safari 11 will always lowercase. Would be interesting to know
> if the websites fail in Safari somehow or manage to work due to different
> code paths...

Do we need to remove lowercase pref-off for 57? Since the spec issue is already closed. Or we shall do it after 57?

Comment 23

[Hsin-Yi Tsai (she/her) [:hsinyi]]

(In reply to Shawn Huang [:shawnjohnjr] from comment #22)
> (In reply to Anne (:annevk) from comment #21)
> > I agree with bz. The specification issue is going nowhere fast. It seems
> > that at least Safari 11 will always lowercase. Would be interesting to know
> > if the websites fail in Safari somehow or manage to work due to different
> > code paths...
> 
> Do we need to remove lowercase pref-off for 57? Since the spec issue is
> already closed. Or we shall do it after 57?

In consideration of how late we are at nightly 57 and the quality of 57 should really be the focus for us, I am concerned the webcompat risk. Additionally, I don't really get big benefit we ship this (remove pref0-off) at this moment, doing it post 57 sounds a better plan to me. Anne, :bz, what do you think?

Comment 24

[Anne (:annevk)]

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 25

[Anne (:annevk)]

Should we open a new bug to track lowercasing or reopen this one?

Comment 26

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

(In reply to Anne (:annevk) from 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.

Yeah, I think we should play safe for 57. I asked simply because the spec issue has been stopped for a while.
I will open new issue for this.

Comment 27

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

(In reply to Anne (:annevk) from comment #25)
> Should we open a new bug to track lowercasing or reopen this one?

Bug 1398718.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment 24 works for me.