Intent to implement and ship: lowercase header names in XHR.getAllResponseHeaders()

[Raphael Kubo da Costa]

(Resending with a different subject following
https://groups.google.com/a/chromium.org/d/msg/blink-dev/cakEmcqoWoA/ckWVE-8ZBwAJ)

Hirano-san started a thread earlier today asking whether this change
deserved an intent-to-something email. Since the consensus seems to be
that sending an intent message doesn't hurt, I'm starting this thread
that's a mix between an intent to implement and an intent to ship: this
is not about a new feature or API, but rather changing the behavior of
an existing API call.

Intent to Implement/Ship
------------------------
Return lower-case header names in XMLHttpRequest.getAllResponseHeaders()

Contact emails
--------------
raphael.ku...@intel.com
tyos...@chromium.org
yhi...@chromium.org

Spec
----
https://xhr.spec.whatwg.org/#the-getallresponseheaders%28%29-method

Summary
-------
In June 2016, the XHR spec was updated and getAllResponseHeaders()'s
description started referencing the Fetch spec to determine how the
headers and values should be returned:
<https://github.com/whatwg/xhr/commit/ecce3904ace0dbe382a652ea1d8e29c7885fe8c4>

Specifically, it refers to Fetch's "sort and combine" steps that include
lower-casing all header names, which was not the case before.

We'd like to align Blink's behavior with the spec and start lower-casing
the header names returned by XHR.getAllResponseHeaders().

Despite its title, http://crrev.com/2787003002 already did the same to
Blink's Fetch API code.

Motivation
----------
Align Blink's implementation with the spec, make more web-platform-tests
pass and reduce interoperability issues.

Interoperability risk
---------------------
Firefox: under development
https://bugzilla.mozilla.org/show_bug.cgi?id=1348390#c8
Edge: ?
Safari: Similar change landed in March 2017
https://bugs.webkit.org/show_bug.cgi?id=169286
Web developers: ?

Compatibility risk
------------------
This is the biggest question. Quoting Mike West in the other thread:

> it's a developer-visible change in behavior that you could imagine
> sufficiently brittle code relying on: `x == "Content-Type"` will fail
> tomorrow when it didn't fail today. I think lowercasing is a
> reasonable change to make, but it's something we'd also want to tell
> developers about in one way or another, and evaluate whether other
> vendors will hop on board.

Is this feature supported on all six Blink platforms?
-----------------------------------------------------
Yes.

Is this feature fully tested by web-platform-tests?
---------------------------------------------------
Yes.

Related bug report
------------------
https://bugs.chromium.org/p/chromium/issues/detail?id=700434

Requesting approval to ship the behavior change
-----------------------------------------------
Yes, in master (M60).

[Chris Harrelson]

Hi,

I think we need more data to say whether this is a safe enough change to make. Phistuck did a little bit of digging (on the prior thread for this intent) and did find some cases that would break. More investigation and stats are needed to quantify how much risk there is

Chris

On Thu, Apr 20, 2017 at 9:21 AM, Raphael Kubo da Costa <raphael.ku...@intel.com> wrote:

(Resending with a different subject following
https://groups.google.com/a/chromium.org/d/msg/blink-dev/cakEmcqoWoA/ckWVE-8ZBwAJ)

Hirano-san started a thread earlier today asking whether this change
deserved an intent-to-something email. Since the consensus seems to be
that sending an intent message doesn't hurt, I'm starting this thread
that's a mix between an intent to implement and an intent to ship: this
is not about a new feature or API, but rather changing the behavior of
an existing API call.

Intent to Implement/Ship
------------------------
Return lower-case header names in XMLHttpRequest.getAllResponseHeaders()

Contact emails
--------------

raphael.kubo.da.costa@intel.com

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.

[Rick Byers]

Ah just saw this fork of the thread (sorry).  Here's what I said:

Thanks for doing the GitHub search PhistucK - that gives us some amount of confidence.

Evaluating the compat risk (beyond just scanning existing websites) certainly seems tough on this one.  Perhaps we should add a UseCounter for getAllResponseHeaders to place an upper bound on the possible breakage?  If that's small (eg. <0.1% of page views) then I'd have no hesitation with just giving this a try.  If it was large, then I'd probably advocate for some additional analysis (eg. we could try to do a more systemic look at the top hits in HTTP Archive).  Other thoughts?

That WebKit has landed this already is helpful, but given that we'd still likely ship to a large audience before Safari, I think the compat risk is still really on us.  I didn't see any real discussion of compat risk in their bug.

On Sat, Apr 22, 2017 at 7:17 AM, Chris Harrelson <chri...@chromium.org> wrote:

Hi,

I think we need more data to say whether this is a safe enough change to make. Phistuck did a little bit of digging (on the prior thread for this intent) and did find some cases that would break. More investigation and stats are needed to quantify how much risk there is

Chris

On Thu, Apr 20, 2017 at 9:21 AM, Raphael Kubo da Costa <raphael.kubo.da.costa@intel.com> wrote:

(Resending with a different subject following
https://groups.google.com/a/chromium.org/d/msg/blink-dev/cakEmcqoWoA/ckWVE-8ZBwAJ)

Hirano-san started a thread earlier today asking whether this change
deserved an intent-to-something email. Since the consensus seems to be
that sending an intent message doesn't hurt, I'm starting this thread
that's a mix between an intent to implement and an intent to ship: this
is not about a new feature or API, but rather changing the behavior of
an existing API call.

Intent to Implement/Ship
------------------------
Return lower-case header names in XMLHttpRequest.getAllResponseHeaders()

Contact emails
--------------

raphael.ku...@intel.com

[Takeshi Yoshino]

On Sat, Apr 22, 2017 at 5:01 PM, Rick Byers <rby...@chromium.org> wrote:

Ah just saw this fork of the thread (sorry).  Here's what I said:

Thanks for doing the GitHub search PhistucK - that gives us some amount of confidence.

Evaluating the compat risk (beyond just scanning existing websites) certainly seems tough on this one.  Perhaps we should add a UseCounter for getAllResponseHeaders to place an upper bound on the possible breakage?  If that's small (eg. <0.1% of page views) then I'd have no hesitation with just giving this a try.  If it was large, then I'd probably advocate for some additional analysis (eg. we could try to do a more systemic look at the top hits in HTTP Archive).  Other thoughts?

Explored the HTTP archive a bit. Many sites (e.g. runs.rank = 2, 4, 8, 9, 10, ...) have it.

I guessed they're not for debugging since compiled for production use, but not fully sure since compiled/obfuscated.

Many of them are jQuery. jQuery's ajaxTransport() passes received response headers to the completion callback. It's created by parsing the result of getAllResponseHeaders(). It's clearly noted in the jQuery API document.

http://api.jquery.com/jQuery.ajaxTransport/

[Raphael Kubo da Costa]

Takeshi Yoshino <tyos...@chromium.org> writes:
> On Sat, Apr 22, 2017 at 5:01 PM, Rick Byers <rby...@chromium.org> wrote:
>> Evaluating the compat risk (beyond just scanning existing websites)
>> certainly seems tough on this one. Perhaps we should add a UseCounter for
>> getAllResponseHeaders to place an upper bound on the possible breakage? If
>> that's small (eg. <0.1% of page views) then I'd have no hesitation with
>> just giving this a try. If it was large, then I'd probably advocate for
>> some additional analysis (eg. we could try to do a more systemic look at
>> the top hits in HTTP Archive). Other thoughts?
>
> Explored the HTTP archive a bit. Many sites (e.g. runs.rank = 2, 4, 8, 9,
> 10, ...) have it.
>

>> On Sat, Apr 22, 2017 at 7:17 AM, Chris Harrelson <chri...@chromium.org>
>> wrote:
>>
>>> Hi,
>>>
>>> I think we need more data to say whether this is a safe enough change to
>>> make. Phistuck did a little bit of digging (on the prior thread for this
>>> intent) and did find some cases that would break. More investigation and
>>> stats are needed to quantify how much risk there is

Hi everyone,

Given the feedback above, I'm confused as to the best course of action
here. Do we want to add a UseCounter to get an upper bound on the calls
to getAllResponseHeaders or is that not necessary given Takeshi's
explorations?

[PhistucK]

Since it is used by jQuery.ajax, I expect the numbers to be very, very high and therefore pretty meaningless.

You should probably go over HTTPArchive results for this and see whether they fail. :(

☆PhistucK

[Philip Jägenstedt]

I think there are two things worth doing that might give better insights than static HTTPArchive analysis, which seems tricky in this case.

First, take the top sites identified by tyoshino@ and try them with a build that has the change. Looking visually for breakage is hard if you're not a regular user of the site, so instead set a breakpoint in DevTools where getAllResponseHeaders is called and try to follow the return value to where it's used.

Second, figure out how far in the Safari release channel https://bugs.webkit.org/show_bug.cgi?id=169286 has gotten and search for regressions in their bug database. I assume it's not in stable Safari yet, but once it is, that's a good signal that the change is web compatible.

Because it's a matter of how the return value is used, a use counter probably won't do any good here. The number you'll get is roughly the risk of removing the method entirely, and that's much different to what's being proposed.

HTH

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+...@chromium.org.

[Takeshi Yoshino]

On Wed, Apr 26, 2017 at 4:07 AM, Philip Jägenstedt <foo...@chromium.org> wrote:

I think there are two things worth doing that might give better insights than static HTTPArchive analysis, which seems tricky in this case.

First, take the top sites identified by tyoshino@ and try them with a build that has the change. Looking visually for breakage is hard if you're not a regular user of the site, so instead set a breakpoint in DevTools where getAllResponseHeaders is called and try to follow the return value to where it's used.

Second, figure out how far in the Safari release channel https://bugs.webkit.org/show_bug.cgi?id=169286 has gotten and search for regressions in their bug database. I assume it's not in stable Safari yet, but once it is, that's a good signal that the change is web compatible.

Thanks Philip. These make sense.

I'll help you doing the first item if needed, Raphael. Let's discuss on the bug or review.

 

Because it's a matter of how the return value is used, a use counter probably won't do any good here. The number you'll get is roughly the risk of removing the method entirely, and that's much different to what's being proposed.

HTH

On Tue, Apr 25, 2017 at 10:26 PM PhistucK <phis...@gmail.com> wrote:

Since it is used by jQuery.ajax, I expect the numbers to be very, very high and therefore pretty meaningless.

You should probably go over HTTPArchive results for this and see whether they fail. :(

☆PhistucK

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.

[Raphael Kubo da Costa]

Takeshi Yoshino <tyos...@chromium.org> writes:

> On Wed, Apr 26, 2017 at 4:07 AM, Philip Jägenstedt <foo...@chromium.org>
> wrote:
>
>> I think there are two things worth doing that might give better insights
>> than static HTTPArchive analysis, which seems tricky in this case.
>>
>> First, take the top sites identified by tyoshino@ and try them with a
>> build that has the change. Looking visually for breakage is hard if you're
>> not a regular user of the site, so instead set a breakpoint in DevTools
>> where getAllResponseHeaders is called and try to follow the return value to
>> where it's used.
>>
>> Second, figure out how far in the Safari release channel
>> https://bugs.webkit.org/show_bug.cgi?id=169286 has gotten and search for
>> regressions in their bug database. I assume it's not in stable Safari yet,
>> but once it is, that's a good signal that the change is web compatible.
>>
>
> Thanks Philip. These make sense.
>
> I'll help you doing the first item if needed, Raphael. Let's discuss on the
> bug or review.

tyoshino@ and I have been discussing this on crbug.com/716994. I checked
the top sites he'd mentioned, and these are the most important findings:

- The vast majority of the websites reference getAllResponseHeaders()
because they ship jQuery. However, no code path calling gARH() is ever
called in any of them (I couldn't test code that required a login,
though).

- There are some other frameworks such as YUI (2 and 3) and Naver's
Jindo.js that also just forward whatever getAllResponseHeaders()
returns, but none of the websites that use them reach those code paths
at all.

- Angular.js code does not immediately normalize getAllResponseHeader()
header names, but it provides several functions such as parseHeaders()
that automatically convert all header names to lowercase.

- Google's closure-library seems to behave similarly, and none of the
websites using it ever called the getAllResponseHeaders() functions.

- Many websites also ship a fetch() polyfill that's obviously not used
by us.

- There's some analytics stuff from IBM Tealeaf and Clicktale that call
getAllResponseHeaders(), but that function was never called here.

- Some websites just call getAllResponseHeaders() for debugging purposes
(ie. if something goes wrong the headers are printed somewhere).

- Most of the websites that really use getAllResponseHeaders(), like
YouTube, already lowercase the header names when doing comparisons and
lookups.

The only website that can genuinely break in some cases with this change
is ok.ru. It contains code that looks like this:
var y = q.getAllResponseHeaders(), x;
while (x = p.exec(y)) {
j[x[1]] = x[2];
}
if (!g && j["Redirect-Location"]) {
f(j["Redirect-Location"]);
t(q, q.statusText, "Redirect");
return;
}
where p is a regular expression object. I didn't receive any payload
with redirect-location in my tests, but it's clear that the check above
will break when all headers are lowercased.

As for looking for regressions in WebKit's bug tracker: I couldn't find
anything.

Given the above, how should we proceed here?

[Simon Pieters]

(Yay for web compat analysis! I'm happy that this is standard procedure
now.)

> As for looking for regressions in WebKit's bug tracker: I couldn't find
> anything.
>
> Given the above, how should we proceed here?

1 broken site from httparchive seems manageable, and on the order of other
breaking changes/removals I believe.

Coordinating shipping of breaking changes between multiple browsers
increases the chance of success that it will stick.

The WebKit bug is fixed; Gecko appears to have a patch now:
https://bugzilla.mozilla.org/show_bug.cgi?id=1348390#c31

It seems to me we should go ahead, try to sync shipping with Firefox or
Safari, whichever is first, and watch out for any regressions.

--
Simon Pieters
Opera Software

[Chris Harrelson]

I agree: this compat analysis, plus the positive signs from WebKit and Gecko make this worth trying. Thanks for doing the work to analyze the situation.

LGTM1

Please report back with any regressions, if for no other reason than to calibrate the decision to ship (assuming you get two other LGTMs.)

On Tue, May 16, 2017 at 1:19 PM, Simon Pieters <sim...@opera.com> wrote:

--
You received this message because you are subscribed to the Google Groups "blink-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/op.y0cle1lfidj3kv%40simons-mbp.lan.

[Rick Byers]

+1.  Thanks for the analysis, and it'll be great to here how this works out in practice!  LGTM2

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAOMQ%2Bw_YGo709qZzkE99yXs5Pjo9oCJ6-R6iG7CBadcKsGykCw%40mail.gmail.com.

[Raphael Kubo da Costa]

Any volunteers for a third l-g-t-m? :-)

>>> email to blink-dev+...@chromium.org.

>>> To view this discussion on the web visit https://groups.google.com/a/ch
>>> romium.org/d/msgid/blink-dev/op.y0cle1lfidj3kv%40simons-mbp.lan.
>>>
>>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "blink-dev" group.
>> To view this discussion on the web visit https://groups.google.com/a/
>> chromium.org/d/msgid/blink-dev/CAOMQ%2Bw_YGo709qZzkE99yXs5Pjo9oCJ6-
>> R6iG7CBadcKsGykCw%40mail.gmail.com

>> <https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAOMQ%2Bw_YGo709qZzkE99yXs5Pjo9oCJ6-R6iG7CBadcKsGykCw%40mail.gmail.com?utm_medium=email&utm_source=footer>
>> .
>>

[Jochen Eisinger]

lgtm3

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/87tw4drzda.fsf%40rkubodac-desk.ger.corp.intel.com.

[Raternals]

Thanks a lot for breaking our applications by lowercasing the keys in the getAllResponseHeaders()-function...

Probably not the only one, see https://twitter.com/thegecko/status/890346862875742210, and I guess a lot will follow... Why is this done in Chrome 60? And why is that nowhere anounced ? Not noted in the changelog? https://developers.google.com/web/updates/2017/07/nic60

This is stupid! Who can I charge for my hours spent debugging and fixing this?

Op donderdag 20 april 2017 18:21:32 UTC+2 schreef Raphael Kubo Da Costa:

[Philip Jägenstedt]

Raternals, sorry that you've had to spend time debugging because of this change. From what we were able to tell by looking at top sites it looked like this change was not riskier than many other changes we have made that have worked out. However, understanding the risk is far from a science, and we want to learn from our mishaps.

In your case, can you share what the code in question was? Was it simply comparing against a literal like "Redirect-Location" without any case folding?

https://github.com/whatwg/xhr/issues/146 is the issue discussing the fallout from this change and what to do about it. I've already linked to your message from the thread, so no need to repeat it though.

--

You received this message because you are subscribed to the Google Groups "blink-dev" group.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/6f62eaae-06f0-45b8-8122-2a7e9eabbd8c%40chromium.org.

[PhistucK]

Damn, I did not catch the missing ChromeStatus entry, so I guess it was missing from the beta blog post as a result as well. :(

It would be great if the API owners also made sure everything has a ChromeStatus entry. :(

Was the used template for the intent post changed manually to completely exclude the ChromeStatus section?

☆PhistucK

To unsubscribe from this group and stop receiving emails from it, send an email to blink-dev+unsubscribe@chromium.org.
To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/CAARdPYcZnXCeh_BwCxBs%2BT-FQNcXyU%2B98h8ojF6dukaMcNAt2w%40mail.gmail.com.

[stefanv...@gmail.com]

Philip Jägenstedt, we've build a web-application using our self-build API for receiving and sending data. We don't use sessions or cookies to store session/login-information, but use a JWT-token for this. Our web-application uses the XHR.getAllResponseHeaders()-function to fetch the header field names and corresponding values. We use the header field name X-Access-Token to receive a JWT-token which we sent in the next request (inside the headers) to keep info about the logged in user.

After each request, the API will return a new token with an update expiry timestamp, so that way, a user can stay logged in as long as there's some interaction. When no or an invalid JWT-token, or a token with an expired timestamp is sent, we detect it and send a 401 Unauthorized. Based on this, the user will be redirected back to the login page of our application in order to login again and get a new token.

Since today, after login in, each next request resulted in a 401 Unauthorized response and a redirect back to the login page. After a long search, rebooting my pc and reinstalling Chrome (after first attempts disabling all extensions one by one, to exclude it was one of them...) we noted it happened only on Chrome... and only on my pc...

After more searching, testing and debugging, we finally found the problem was caused due to an empty token in the requests. After that, we found that the header field name (in Chrome 60) was converted from X-Access-Token to x-access-token. Due to this change, we weren't able to fetch the JWT-token from the response-headers, causing the ongoing redirects to te login page after loggin in succesfully. So yes, it was indeed a literal comparison... 

We changed our code to check on the header field x-access-token for the JWT-token from the API, which made it work in Chrome. Unfortunately, this caused the same problem in all other browsers, because there, the XHR.getAllResponseHeaders()-function does return the header field names untouched.

Now, we're converting all header field name to lowercase in order to make it work in all browsers. So, after a long fight against Internet Explorer, we're back in a situation where various browsers respond differently to the same functions... I do not think we want to go there!

Op maandag 31 juli 2017 14:06:34 UTC+2 schreef Philip Jägenstedt:

[Philip Jägenstedt]

That does not sound like fun debugging, I feel your pain.

Rather than changing the capitalization of the headers on the wire, have you tried name.toLowerCase()=="x-access-token", where name is the header name presumably extracted with a RegExp or similar? That should fix the problem for all browsers, before and after this change was introduced.

In https://github.com/whatwg/xhr/issues/146 the conclusion is tending towards leaving the change in Chrome and Safari. It was already reverted in Firefox, so there's some question of whether those issues will sort themselves out by Chrome and Safari breaking the same pages, or if there's a risk that they're Firefox-specific due to UA sniffing.

To view this discussion on the web visit https://groups.google.com/a/chromium.org/d/msgid/blink-dev/e2445577-c684-474c-9b74-e15850ab0ec3%40chromium.org.