918721 - [XHR2] getResponseHeader() returns null instead of empty string for header without value (X-Custom-Header-Empty: )

Status: RESOLVED FIXED

(Core :: Networking, defect, P3)

Description

[Hallvord R. M. Steen [:hallvors]]

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

Comment 1

[Anne (:annevk)]

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

Comment 2

[Boris Zbarsky [:bzbarsky]]

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

Comment 3

[Hallvord R. M. Steen [:hallvors]]

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)

Comment 4

[Jason Duell]

> 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.

Comment 5

[Dragana Damjanovic [:dragana]]

This will be resolved in bug 669259.

Comment 6

[Dragana Damjanovic [:dragana]]

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.

Comment 7

[Thomas Wisniewski]

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.

Comment 8

[Dragana Damjanovic [:dragana]]

(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 :)

Comment 9

[Thomas Wisniewski]

Attachment: 918721-enable-pref-to-get-getResponseHeader-sending-empty-string-instead-of-null.diff

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?)

Comment 10

[Thomas Wisniewski]

Attachment: 918721-enable-pref-to-get-getResponseHeader-sending-empty-string-instead-of-null.diff

Minor rebase. Carrying over r=dragana.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Thomas Wisniewski]

Attachment: 918721-enable-pref-to-get-getResponseHeader-sending-empty-string-instead-of-null.diff

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.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e78cec726586
Backed out changeset 28cd5a9882d8 for causing 1285919

Comment 16

[Thomas Wisniewski]

Attachment: 918721-enable-pref-to-get-getResponseHeader-sending-empty-string-instead-of-null.diff

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
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

Comment 18

[Carsten Book [:Tomcat]]

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

Comment 19

[Sebastian Zartner [:sebo]]

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

Comment 20

[Sebastian Zartner [:sebo]]

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

Sebastian

Comment 21

[Thomas Wisniewski]

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.

Comment 22

[Dragana Damjanovic [:dragana]]

(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. :)

Comment 23

[Thomas Wisniewski]

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

Comment 24

[Sebastian Zartner [:sebo]]

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

Sebastian

Comment 25

[Thomas Wisniewski]

I was just reading them right now, actually :)

The changes seem fine to me.

Comment 26

[Sebastian Zartner [:sebo]]

Great! Thanks for the feedback!

Sebastian