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

RESOLVED FIXED in Firefox 50

Status

()

P3
normal
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: hsteen, Assigned: wisniewskit)

Tracking

({dev-doc-complete})

unspecified
mozilla50
x86
Linux
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: [necko-would-take], URL)

Attachments

(1 attachment, 3 obsolete attachments)

Comment 1

4 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
Component: DOM: Core & HTML → Networking
Note bug 474845, which I think got dupped incorrectly....
Flags: needinfo?(jduell.mcbugs)
(Reporter)

Comment 3

4 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)
> 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.
Flags: needinfo?(jduell.mcbugs)
This will be resolved in bug 669259.
Whiteboard: [necko-would-take]
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.
(Assignee)

Comment 7

2 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.
Flags: needinfo?(dd.mozilla)
(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 :)
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 9

2 years ago
Created attachment 8767231 [details] [diff] [review]
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?)
Assignee: nobody → wisniewskit
Status: NEW → ASSIGNED
Attachment #8767231 - Flags: review?(dd.mozilla)
Attachment #8767231 - Flags: review?(dd.mozilla) → review+
(Assignee)

Comment 10

2 years ago
Created attachment 8768787 [details] [diff] [review]
918721-enable-pref-to-get-getResponseHeader-sending-empty-string-instead-of-null.diff

Minor rebase. Carrying over r=dragana.
Attachment #8767231 - Attachment is obsolete: true
(Assignee)

Updated

2 years ago
Keywords: checkin-needed
The commit message mentions web-platform-test annotation changes but this patch doesn't contain any. Was that on purpose?
Flags: needinfo?(wisniewskit)
Keywords: checkin-needed
(Assignee)

Comment 12

2 years ago
Created attachment 8769229 [details] [diff] [review]
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.
Attachment #8768787 - Attachment is obsolete: true
Flags: needinfo?(wisniewskit)
Keywords: checkin-needed

Comment 13

2 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
Keywords: checkin-needed
sorry had to back this out for orange results like bug 1285919
Flags: needinfo?(wisniewskit)

Comment 15

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

Comment 16

2 years ago
Created attachment 8769799 [details] [diff] [review]
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
Attachment #8769229 - Attachment is obsolete: true
Flags: needinfo?(wisniewskit)
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 17

2 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
Keywords: checkin-needed

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/98f2ac456f11
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
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
Flags: needinfo?(wisniewskit)
Keywords: dev-doc-needed
(Assignee)

Comment 21

2 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.
Flags: needinfo?(wisniewskit) → needinfo?(dd.mozilla)
(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. :)
Flags: needinfo?(dd.mozilla)
(Assignee)

Comment 23

2 years ago
Alright, I've filed bug 1286554 so we don't forget to remove it eventually.
Thomas, did you also have a look at the docs already?

Sebastian
(Assignee)

Comment 25

2 years ago
I was just reading them right now, actually :)

The changes seem fine to me.
Great! Thanks for the feedback!

Sebastian
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.