Closed Bug 1348390 Opened 7 years ago Closed 7 years ago

Permfail: XMLHttpRequest/getallresponseheaders.htm assert_equals: expected "also-here: Mr. PB\r\newok: lego\r\nfoo-test: 1, 2\r\n" but got "foo-TEST: 1, 2\r\nALSO-here: Mr. PB\r\newok: lego\r\n"

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: aryx, Assigned: shawnjohnjr)

References

Details

(Keywords: dev-doc-needed, Whiteboard: [stockwell disabled])

Attachments

(1 file, 4 obsolete files)

Bug 1348390 - Sort and lowercase response's header lists for getAllResponseHeaders(), r=baku
5.47 KB, patch
Details | Diff | Splinter Review 
https://treeherder.mozilla.org/logviewer.html#?job_id=84702273&repo=mozilla-inbound

At least three times on the inbound tree at the moment, all Linux opt (both 32 and 64 bit). Test landed two days ago as part of bug 1345490.

[task 2017-03-17T18:51:43.112234Z] 18:51:43     INFO - TEST-START | /XMLHttpRequest/getallresponseheaders.htm
[task 2017-03-17T18:51:43.327068Z] 18:51:43     INFO - 
[task 2017-03-17T18:51:43.328185Z] 18:51:43     INFO - TEST-UNEXPECTED-FAIL | /XMLHttpRequest/getallresponseheaders.htm | XMLHttpRequest: getAllResponseHeaders() - assert_equals: expected "foo-test: 1, 2, 3\r\n" but got "foo-TEST: 1, 2, 3\r\n"
[task 2017-03-17T18:51:43.328578Z] 18:51:43     INFO - client.onload<@http://web-platform.test:8000/XMLHttpRequest/getallresponseheaders.htm:10:5
[task 2017-03-17T18:51:43.329074Z] 18:51:43     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1409:20
[task 2017-03-17T18:51:43.331013Z] 18:51:43     INFO - Test.prototype.step_func_done/<@http://web-platform.test:8000/resources/testharness.js:1449:17
[task 2017-03-17T18:51:43.331084Z] 18:51:43     INFO - EventHandlerNonNull*@http://web-platform.test:8000/XMLHttpRequest/getallresponseheaders.htm:9:19
[task 2017-03-17T18:51:43.331145Z] 18:51:43     INFO - Test.prototype.step@http://web-platform.test:8000/resources/testharness.js:1409:20
[task 2017-03-17T18:51:43.331202Z] 18:51:43     INFO - async_test@http://web-platform.test:8000/resources/testharness.js:518:13
[task 2017-03-17T18:51:43.331260Z] 18:51:43     INFO - @http://web-platform.test:8000/XMLHttpRequest/getallresponseheaders.htm:7:1
[task 2017-03-17T18:51:43.331294Z] 18:51:43     INFO - .
[task 2017-03-17T18:51:43.331343Z] 18:51:43     INFO - TEST-OK | /XMLHttpRequest/getallresponseheaders.htm | took 215ms
this is a new failure as of the 17th where bug 1345490 landed which updated wpt and tests.  From what I can tell this test was added then and shortly afterwards this started failing (81 failures to date since then):
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1348390&startday=2017-03-14&endday=2017-03-22&tree=all

:jgraham, can you look into fixing this bug or disabling it?  This appears to be a linux32/64 thing, opt/debug as well.
Flags: needinfo?(james)
Whiteboard: [stockwell needswork]
Anne: your test seems to be unstable. Can we fix it or should it be disabled?
Flags: needinfo?(annevk)
Flags: needinfo?(james)
According to: https://github.com/whatwg/xhr/issues/109
We should do header casing/header sorting.
This doesn't seem to be an intermittent bug now, but a perma failure.
Assignee: nobody → shuang
I think it's better to disable it first then fix it.
It's not an intermittent bug at my side. If anyone has solution, feel free to take it.
I am happy to review the patch for disabling these until we have more info to fix the bug.
Clear ni, since I'm going to fix this bug.
Flags: needinfo?(annevk)
Comment on attachment 8851481 [details] [diff] [review]
Bug 1348390 - Disable new XHR wpt for instability

Review of attachment 8851481 [details] [diff] [review]:
-----------------------------------------------------------------

thanks!
Attachment #8851481 - Flags: review?(jmaher) → review+
Pushed by shuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/854b5f25ff67
Disable new XHR wpt for instability, r=jmaher
Whiteboard: [stockwell needswork] → [stockwell disabled]
Hi Anne,
I'm new here.
I tried to fix getallresponseheaders case. But I found bug 1293881 showing concerns to match the current XHR specification, it lowercased all incoming header names. In that bug, it says that "There's no guarantee that this (behavioral) change will be web-compatible, so it was decided to revert it."

Since XHR specification also suggests we should run 'sort and combine' with response’s header list, do we really have potential lowercased web-compatible issue here? Now I'm not sure if we have any potential risk. Although I think it makes sense to consider header name case insensitive. It seems to me to follow the new XHR spec is the right thing to do.
Flags: needinfo?(annevk)
There's some risk here, yes: https://groups.google.com/a/chromium.org/d/msg/blink-dev/_oxlCPNsrck/2FePT4QaBwAJ. Safari has already shipped the lowercasing behavior, but that could still mean that we run into issues. The safest solution might be to ship it with a flag that we can quickly toggle if there's a problem.

(Note that the standard no longer requires lowercasing request headers either.)
Flags: needinfo?(annevk)
Change summary since the original bug has been fixed in bug 1334776.
Summary: Intermittent /XMLHttpRequest/getallresponseheaders.htm | XMLHttpRequest: getAllResponseHeaders() - assert_equals: expected "foo-test: 1, 2, 3\r\n" but got "foo-TEST: 1, 2, 3\r\n" → Permfail: XMLHttpRequest/getallresponseheaders.htm assert_equals: expected "also-here: Mr. PB\r\newok: lego\r\nfoo-test: 1, 2\r\n" but got "foo-TEST: 1, 2\r\nALSO-here: Mr. PB\r\newok: lego\r\n"
(In reply to Shawn Huang [:shawnjohnjr] from comment #26)
> try:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3a74a12de228c0b6e48754e196a332fe7474baf1

So this also fixes two tests :-O

TEST-UNEXPECTED-PASS | /XMLHttpRequest/getresponseheader-chunked-trailer.htm | XMLHttpRequest: getResponseHeader() and HTTP trailer - expected FAIL

TEST-UNEXPECTED-PASS | /XMLHttpRequest/getallresponseheaders-cl.htm | Casing of known headers - expected FAIL
Comment on attachment 8868094 [details] [diff] [review]
Bug 1348390 - Sort and lowercase response's header lists for getAllResponseHeaders()

Based on Anne's suggestion, I added pref to disable lowercase.
Attachment #8868094 - Flags: review?(amarchesini)
Comment on attachment 8868094 [details] [diff] [review]
Bug 1348390 - Sort and lowercase response's header lists for getAllResponseHeaders()

Review of attachment 8868094 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/xhr/XMLHttpRequestMainThread.cpp
@@ +3770,5 @@
>  NS_IMETHODIMP XMLHttpRequestMainThread::
>  nsHeaderVisitor::VisitHeader(const nsACString &header, const nsACString &value)
>  {
>    if (mXHR.IsSafeHeader(header, mHttpChannel)) {
> +    if (!Preferences::GetBool("dom.xhr.lowercase_header.enabled", false)) {

we should use AddBoolCache() here. We should cache other boolean prefs in this file, actually. Follow up?

@@ +3771,5 @@
>  nsHeaderVisitor::VisitHeader(const nsACString &header, const nsACString &value)
>  {
>    if (mXHR.IsSafeHeader(header, mHttpChannel)) {
> +    if (!Preferences::GetBool("dom.xhr.lowercase_header.enabled", false)) {
> +      Unused << mHeaderList.InsertElementSorted(HeaderEntry(header, value),

if (!mHeaderList.InsertElementSorted(HeaderEntry(header, value), fallible)) {
  return NS_ERROR_OUT_OF_MEMORY;
}

@@ +3779,5 @@
> +
> +    nsAutoCString lowerHeader(header);
> +    ToLowerCase(lowerHeader);
> +    Unused << mHeaderList.InsertElementSorted(HeaderEntry(lowerHeader, value),
> +                                              fallible);

same here.

::: dom/xhr/XMLHttpRequestMainThread.h
@@ +626,5 @@
>        : mXHR(aXMLHttpRequest), mHttpChannel(aHttpChannel) {}
> +    const nsACString &Headers()
> +    {
> +      for (uint32_t i = 0; i < mHeaderList.Length(); i++) {
> +        HeaderEntry header = mHeaderList.ElementAt(i);

HeaderEntry& header
Attachment #8868094 - Flags: review?(amarchesini) → review+
Like Bug 1341376, I will open an follow-up bug to take care of |AddBoolCache()|.
Component: web-platform-tests → DOM
Product: Testing → Core
Pushed by shuang@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cb64c677091
Sort and lowercase response's header lists for getAllResponseHeaders(), r=baku
https://hg.mozilla.org/mozilla-central/rev/8cb64c677091
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1367688
Depends on: 1370071
Unfortunately this broke Nike's Japanese storefront (reported at https://webcompat.com/issues/7028).

For instance, see http://store.nike.com/jp/ja_jp/product/lunarepic-flyknit-low-2-id/?piid=43767&pbid=547557084

The page never renders, and the console logs this:

>Unhandled promise rejection TypeError: e.headers['Content-Type'] is undefined

Sure enough, e.headers contains "content-type" rather than "Content-Type", and setting dom.xhr.lowercase_header.enabled=false makes the problem go away.
See Also: → https://github.com/webcompat/web-bugs/issues/7028
It looks like Chrome also introduced this change.
See:
https://codereview.chromium.org/2828753002

Yeah, I will disable lowercase for now and wait for other vendors shipping this feature.
(In reply to Shawn Huang [:shawnjohnjr] from comment #38)
> It looks like Chrome also introduced this change.
> See:
> https://codereview.chromium.org/2828753002
> 
> Yeah, I will disable lowercase for now and wait for other vendors shipping
> this feature.

https://bugs.chromium.org/p/chromium/issues/detail?id=716994
For posterity:

Please add keyword "dev-doc-needed" for bugs like this. Lowercasing should be documented here
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/getAllResponseHeaders
and elsewhere.

Don't forget the keyword where lowercasing header names gets enabled later.
Component: DOM → DOM: Core & HTML
No longer depends on: 1367688
Regressions: 1367688
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: