Closed Bug 1370485 Opened 7 years ago Closed 7 years ago

Disable lowercase header names in XHR.getAllResponseHeaders()

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 + fixed

People

(Reporter: shawnjohnjr, Assigned: shawnjohnjr)

References

Details

(Keywords: site-compat, Whiteboard: [webcompat])

Attachments

(1 file)

Bug 1370485 - Do not return lowercased header names for getAllResponseHeaders()
1.01 KB, patch
smaug
: review+
Details | Diff | Splinter Review 
Disable lowercase header names in XHR.getAllResponseHeaders()
Some sites and add-on are not ready for this change.
Assignee: nobody → shuang
[Tracking Requested - why for this release]:
status-firefox55: --- → affected
tracking-firefox55: --- → ?
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
Tracking for 55 as webcompat issue
status-firefox53: --- → unaffected
status-firefox54: --- → unaffected
status-firefox-esr52: --- → unaffected
tracking-firefox55: ? → +
Priority: -- → P1
As :Baku is on vacation, would if be possible to ask other peers for review?
Flags: needinfo?(shuang)
Flags: needinfo?(overholt)
Why aren't we just backing out the regressing patch?
Clearly the pref is enabling something not web compatible.
If comment 7 doesn't work, I'd ask bkelly for review.
Flags: needinfo?(overholt)
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?
Flags: needinfo?(shuang)
Attachment #8875199 - Flags: review?(amarchesini) → review?(bugs)
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 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.
Attachment #8875199 - Flags: review?(bugs) → review+
(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.
Pushed by shuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9819afb3f523
Do not return lowercased header names for getAllResponseHeaders(), r=smaug
Whiteboard: [webcompat]
https://hg.mozilla.org/mozilla-central/rev/9819afb3f523
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Keywords: site-compat
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...
Flags: needinfo?(shuang)
Flags: needinfo?(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.
Flags: needinfo?(annevk)
> 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.
Hmm, I missed those. I'll update the standards issue.
(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
Flags: needinfo?(shuang) → needinfo?(annevk)
For now just lowercase, since that's what we've had web compat issues with, right?  Should see what happens in the spec issue.
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...
Flags: needinfo?(annevk)
(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 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?
Flags: needinfo?(bzbarsky)
Flags: needinfo?(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.
Flags: needinfo?(annevk)
Should we open a new bug to track lowercasing or reopen this one?
(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.
(In reply to Anne (:annevk) from comment #25)
> Should we open a new bug to track lowercasing or reopen this one?

Bug 1398718.
Comment 24 works for me.
Flags: needinfo?(bzbarsky)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: