Closed Bug 1398718 Opened 3 years ago Closed 2 years ago
Remove default pref-off for lowercase header names in XHR
.get All Response Headers()
Bug 1398718 - change default pref to on for lowercase header names in XHR.getAllResponseHeaders; r?hsivonen
46 bytes, text/x-phabricator-request
|Details | Review|
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."
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).
I can't make any try push, it seems my account suspended :( Reset assignee.
Assignee: shawnjohnjr → nobody
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).
(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.
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.
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.
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/cc0cb1e1912d change default pref to on for lowercase header names in XHR.getAllResponseHeaders; r=hsivonen
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
(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.
Backout by email@example.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?
Yes, I think we should try pushing this again. Thanks, Thomas.
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.
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?
Weird. I just reopened the Phabricator issue and did another arc diff.. hopefully that does the trick?
Flags: needinfo?(twisniewski) → needinfo?(cbrindusan)
Pushed by firstname.lastname@example.org: https://hg.mozilla.org/integration/autoland/rev/f49f269d4fd0 change default pref to on for lowercase header names in XHR.getAllResponseHeaders; r=hsivonen
Posted site compatibility note just in case: https://www.fxsitecompat.com/en-CA/docs/2018/xhr-getallresponseheaders-now-returns-header-names-in-lowercase/
This landed in Fx 64, right? Please set Target Milestone to avoid confusion. Is there a bug filed to remove that flag?
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
You need to log in before you can comment on or make changes to this bug.