Hallvord R. M. Steen [:hallvors] Reporter
	Description • 11 years ago

Test case:
http://w3c-test.org/web-platform-tests/master/XMLHttpRequest/getresponseheader-cookies-and-more.htm

	Anne (:annevk)
	Comment 1 • 10 years ago

As far as I can tell this is due to a bug in Necko that removes headers whose value is the empty string. See http://mxr.mozilla.org/mozilla-central/find?string=nsHttpHeaderArray

	Boris Zbarsky [:bzbarsky]
	Comment 2 • 10 years ago

Note bug 474845, which I think got dupped incorrectly....

	Hallvord R. M. Steen [:hallvors] Reporter
	Comment 3 • 10 years ago

Yes, this is wrong: http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpHeaderArray.cpp#30
(Naturally, code that depends on it needs fixing, if any)

	Jason Duell
	Comment 4 • 10 years ago

> Note bug 474845, which I think got dupped incorrectly....

Well, it got duped to a larger bug that would fix this, which is still open.

The code that needs fixing is actually SetHeaderFromNet, not SetHeader.  The SetHeader("") being equal to clearing a header is something addons may be relying on. Headers that come in from the network now use SetHeaderFromNet.

	Dragana Damjanovic [:dragana]
	Comment 5 • 9 years ago

This will be resolved in bug 669259.

	Dragana Damjanovic [:dragana]
	Comment 6 • 8 years ago

Bug 669259 will lend soon.
In bug 669259, we have decided to return empty string for a header with empty value instead of null.

There is a pref that needs to be turned on:
network.http.keep_empty_response_headers_as_empty_string

Currently it is off because this test needs to be fixed too.

	Thomas Wisniewski Assignee
	Comment 7 • 8 years ago

Was there still anything left to be done before the about:config flag can be toggled for good? This web platform test passes just fine when I toggle the flag now, and I can't quite tell if anything still needs to be done in bug 669259.

	Dragana Damjanovic [:dragana]
	Comment 8 • 8 years ago

(In reply to Thomas Wisniewski from comment #7)
> Was there still anything left to be done before the about:config flag can be
> toggled for good? This web platform test passes just fine when I toggle the
> flag now, and I can't quite tell if anything still needs to be done in bug
> 669259.

You can just flip the pref, there is nothing more to be done in bug 669259. 
I introduced that pref in case this change breaks something and we can fix it fast. It should not break anything but you never know with the Internet :)

	Thomas Wisniewski Assignee
	Comment 9 • 8 years ago

Alright, here's a patch. I guess I'll ask you to review it :)

Should I file a follow-up bug about getting rid of that about:config setting entirely? (Or run this patch through try?)

	Thomas Wisniewski Assignee
	Comment 10 • 8 years ago

Minor rebase. Carrying over r=dragana.

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 8 years ago

The commit message mentions web-platform-test annotation changes but this patch doesn't contain any. Was that on purpose?

	Thomas Wisniewski Assignee
	Comment 12 • 8 years ago

Indeed, thanks for catching that. Here's a properly rebased version of the original r+'d patch for checkin, which includes the relevant WPT change.

	Pulsebot
	Comment 13 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/28cd5a9882d8
flip the relevant pref from bug 669259 and mark the relevant web platform test as passing. r=dragana

	Carsten Book [:Tomcat]
	Comment 14 • 8 years ago

sorry had to back this out for orange results like bug 1285919

	Pulsebot
	Comment 15 • 8 years ago

Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e78cec726586
Backed out changeset 28cd5a9882d8 for causing 1285919

	Thomas Wisniewski Assignee
	Comment 16 • 8 years ago

Well, that settles the question of whether I should have sent this through try. I'll do that from now on.

Here's the same patch, but also marking the WPT as passing, which seems to do the trick (so carrying over r+). Here's the try run, with only an unrelated intermittent failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9b0acdcb888

	Pulsebot
	Comment 17 • 8 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/98f2ac456f11
flip the relevant pref from bug 669259 and mark the relevant web platform test as passing. r=dragana

	Carsten Book [:Tomcat]
	Comment 18 • 8 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/98f2ac456f11

	Sebastian Zartner [:sebo]
	Comment 19 • 8 years ago

I've added a note about the changed default value of the preference to the following pages:

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIHttpChannel
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/getResponseHeader
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/getAllResponseHeaders

Thomas, please let me know if something is missing or incorrect.
Also, is there a reason to keep the preference or can we remove it now?

Sebastian

	Sebastian Zartner [:sebo]
	Comment 20 • 8 years ago

Also added a note to https://developer.mozilla.org/en-US/Firefox/Releases/50.

Sebastian

	Thomas Wisniewski Assignee
	Comment 21 • 8 years ago

I suspect it might be wise to keep it around for a release just in case users report issues. Unless :dragana thinks that's overkill, I'll file a bug so we remember to remove it.

	Dragana Damjanovic [:dragana]
	Comment 22 • 8 years ago

(In reply to Sebastian Zartner [:sebo] from comment #19)
> I've added a note about the changed default value of the preference to the
> following pages:
> 
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsIHttpChannel
> https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/
> getResponseHeader
> https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/
> getAllResponseHeaders
> 
> Thomas, please let me know if something is missing or incorrect.
> Also, is there a reason to keep the preference or can we remove it now?
> 
> Sebastian

Let's leave the pref until release. I hope this change will not break anything, but just in case - we are prepared. :)

	Thomas Wisniewski Assignee
	Comment 23 • 8 years ago

Alright, I've filed bug 1286554 so we don't forget to remove it eventually.

	Sebastian Zartner [:sebo]
	Comment 24 • 8 years ago

Thomas, did you also have a look at the docs already?

Sebastian

	Thomas Wisniewski Assignee
	Comment 25 • 8 years ago

I was just reading them right now, actually :)

The changes seem fine to me.

	Sebastian Zartner [:sebo]
	Comment 26 • 8 years ago

Great! Thanks for the feedback!

Sebastian