Closed Bug 1398718 Opened 3 years ago Closed 2 years ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: shawnjohnjr, Assigned: twisniewski)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: addon-compat, dev-doc-complete, site-compat)

Attachments

(1 file)

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."
Blocks: xhr
Priority: -- → P2
Target Milestone: --- → Future
Assignee: nobody → shuang
Target Milestone: Future → mozilla58
Assignee: shuang → nobody
Assignee: nobody → shawnjohnjr
Shawn! /me waves o/
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)
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/.
I'm doing a try-push here, in the interests of helping this along: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfe48ffef0a2fcbfffb3bae5640c1dfdefb23c3a
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)
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)
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)
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+
Alright, let's do this. Requesting check-in.
Keywords: checkin-needed
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)
Yeah, I don't see how my patch would cause that, if it passed the normal try builds.
Flags: needinfo?(twisniewski)
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
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)
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
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)
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
https://hg.mozilla.org/mozilla-central/rev/f49f269d4fd0
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Assignee: nobody → twisniewski
This landed in Fx 64, right? Please set Target Milestone to avoid confusion.
Is there a bug filed to remove that flag?
Blocks: 1504344
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
Regressions: 1540688
You need to log in before you can comment on or make changes to this bug.