Jason Duell Reporter
	Description • 13 years ago

Right now the logic in nsHttpHeaderArray::SetHeaderFromNet creates a header array that doesn't precisely mirror what came in off the net.  It would be nice to have the ability to see exactly what came in.

Known issues:  

1) We ignore headers with blank values (they don't show up in flattened log); 

2) We merge headers as they come in, so Firebug, etc sees the merged header instead of the separate header.

	Jason Duell Reporter
	Comment 1 • 13 years ago

3) We don't show the 2nd, 3rd, etc values of non-mergeable headers, just the first.

Note that chromium at least handles this by adding duplicate headers to the header array, but then returning the first one when it's asked for (while presumably allowing iterators to see all of them).

	Sebastian Zartner [:sebo]
	Comment 3 • 12 years ago

Here is a related Firebug issue:
http://code.google.com/p/fbug/issues/detail?id=5869

Note that this is not just limited to extensions but also the Web Console network output needs this.

Because I didn't find this issue when entering 786829, I want to add that I searched for nsIHttpChannel.
As Jason mentioned there it might be better to add a new API like e.g. nsIHttpChannel2 for this to avoid breaking existing extensions.

Sebastian

	Ache
	Comment 4 • 12 years ago

To send multiply headers with the same name (in case their values can be comma-separated) is allowed according to RFC 2616 (see the quote below). But even some server violates RFC 2616 sending some headers with not comma-separated values (what to do with extensions like X-some-header?), it will be very useful to detect such situation either by Firebug or Live HTTP Headers for server debugging purposes. I think FF need some sort of new API (to not breaking thing related to old API) returning just exactly what server sent i.e. not parsed in any way.

http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html
"Multiple message-header fields with the same field-name MAY be present in a message if and only if the entire field-value for that header field is defined as a comma-separated list [i.e., #(values)]. It MUST be possible to combine the multiple header fields into one "field-name: field-value" pair, without changing the semantics of the message, by appending each subsequent field-value to the first, each separated by a comma. The order in which header fields with the same field-name are received is therefore significant to the interpretation of the combined field value, and thus a proxy MUST NOT change the order of these field values when a message is forwarded."

	Boris Zbarsky [:bzbarsky]
	Comment 6 • 12 years ago

This blocks us passing the CORS test suite (because it breaks XHR getRequestHeader in cases when the server sent an empty header).

Note that for purposes of XHR just issue 1 from comment 0 is relevant .... sort of.

	Jason Duell Reporter
	Comment 8 • 10 years ago

A couple things to add to my list from comment 0 and 1:

4) Calls to to SetHeader("foo", "") (by, for instance, addons that implement on-modify-request, on-examine-response, etc, observers) need to cause a header to be cleared in the sense that any further calls to GetHeader("foo") should return as if the header was never set.  But the API that developer tools, etc use (and this may need to be a new API) to "see all headers as they really were on the network" should still see the original headers only. This is also the case if SetHeader is used to modify the value--IIUC dev tools wants the headers as they were on the network.  For request headers this is easy--SetHeader changes what goes out on the wire (logically one could call SetRequestHeader to modify/delete a header after it's already gone out on the wire, but I expect this is a rare use case, so it's less important to get that right).  For response headers we need to make sure we keep a set of the original headers as well as something that captures changes made to them.   It's been a long time, but I seem to remember gravitating towards an architecture that keeps two different data structures (original headers for devtools and another one that Set/GetHeader sees)

5) Given the XHR use case mentioned we want to make GetHeader() return an empty string for a header that had a blank value, instead of throwing.  So this will be a little weird--if a blank header came in the on the network, GetHeader() will return an empty string.  But if SetHeader("") is called at any point after that, after that GetHeader() should throw as if the header was never set.  At least that's my reading of what the IDL promises

  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsIHttpChannel.idl#258
  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsIHttpChannel.idl#119

(more important than the IDL is that I'm pretty sure this is what we actually do right now, i.e. SetHeader("") deletes the value, and we don't want to change that else we'll break addons)

6) We need to handle requests that are partially served by the cache and partly from the network (206 responses)--we wind up merging in the headers from the cache with the ones from the network.  Make sure to think about that case.

  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#2392
  http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpResponseHead.cpp#569

It might actually be quite simple--just add a given headers from cache so long as it's not identical to one already passed in network (or vice versa if we start with the cached ones and are merging in from the network).

	Jason Duell Reporter
	Comment 9 • 10 years ago

So for this fix we're going to be keeping separate data structures of 1) headers exactly as they came in on network (including empty values and duplicates), and 2) the regular/current hash which both lacks empty headers and also contains any changes to the headers that have been made (by addons, etc) via SetResponseHeader. 

I'm feeling ambitious so I think we're going to try keeping empty strings in the #2 hash, so that GetResponseHeader(foo) will return an empty string if the Foo header had no value (rather than throwing, as we currently do).  We'll need to keep a pref that reverts that back to throwing, in case we discover that the change breaks addons too badly.  I'm hoping it won't.  One benefit of this approach is that we won't need to keep "[deleted]" records in the #2 hash (GetResponseHeader can just check the #2 hash, and ignore the original network values hash). 

XHR can for now just use GetResponseHeader, and if we need to revert the empty string behavior, we can change it to also look at the original network value to see if there was an empty header.  (To be really precise about it, we might want to track [deleted] values in the #2 hash, which would tell us not to look in the #1 hash as the header has been deliberately deleted by an addon, etc).

Also let's make sure that when we iterate over the original headers as they appear on the network, they show up in the actual order they came in on the network--I'm guessing there will be cases where developers will want to know the order.

	Axel Kratel
	Comment 10 • 9 years ago

Answers to some questions:

What would you like to see when request is received from the cache? Are developers interested in the headers that have been stored or something else, like information about the cache entry (the expiration time, the time it is created, etc.)?

There are different answers depending on what a developer is trying to achieve. 
1) Cache minimization: the average developer tries to avoid caching, but the browser tries to maximize caching. In that context it would be useful to surface to a developer when a request comes from cache. Perhaps we can surface this info in the context of the network timeline, i.e. what is the browser spending its time doing. 
2) CDN optimization and optimizing cache control on the network, not the browser. Not sure about this one, the servers on the edge manipulate the content based on request origin. Do they leave a cache signature that we can pick up?
3) app network optimization: if the developer forces the browser to not cache, but obvious parts can be cached, it would be nice to be able to to surface this to the developers.

How to show requests that are partially served by the cache and partially from the network. Are the developers interested to see just the headers received from the network or somehow merged headers (from cache and from network)? (Note: partial requests typically happen when we've done Range requests, so we have some of the content in cache, but need to hit the network for part of it)

This would be useful in the context of the network timeline, and the actual header content. Is it possible to determine if some header elements came from cache as opposed to the network? As the timeline goes, it helps to show time spent retrieving from network versus time spent retrieving from cache. 

Sorry if this muddles the waters.

	Dragana Damjanovic [:dragana] Assignee
	Comment 11 • 9 years ago

(In reply to Axel Kratel from comment #10)
> Answers to some questions:
> 
> What would you like to see when request is received from the cache? Are
> developers interested in the headers that have been stored or something
> else, like information about the cache entry (the expiration time, the time
> it is created, etc.)?
> 
> There are different answers depending on what a developer is trying to
> achieve. 
> 1) Cache minimization: the average developer tries to avoid caching, but the
> browser tries to maximize caching. In that context it would be useful to
> surface to a developer when a request comes from cache. Perhaps we can
> surface this info in the context of the network timeline, i.e. what is the
> browser spending its time doing. 

This is possible to have, but we should open a different bug for that. 


> 2) CDN optimization and optimizing cache control on the network, not the
> browser. Not sure about this one, the servers on the edge manipulate the
> content based on request origin. Do they leave a cache signature that we can
> pick up?

I do not know about it. I expect that if there is something, like signature, we probably do not need to change necko for that and everything is already expose in js. Maybe also opening a different bug for this?

> 3) app network optimization: if the developer forces the browser to not
> cache, but obvious parts can be cached, it would be nice to be able to to
> surface this to the developers.
> 

That would mean that we should cache everything but treat it as not cached to be able to inform developer that it would be better to cache. Isn't it easier if developer would try both with and without caching?

> How to show requests that are partially served by the cache and partially
> from the network. Are the developers interested to see just the headers
> received from the network or somehow merged headers (from cache and from
> network)? (Note: partial requests typically happen when we've done Range
> requests, so we have some of the content in cache, but need to hit the
> network for part of it)
> 
> This would be useful in the context of the network timeline, and the actual
> header content. Is it possible to determine if some header elements came
> from cache as opposed to the network? As the timeline goes, it helps to show
> time spent retrieving from network versus time spent retrieving from cache. 
> 

So now (for the requests that are partially served from the network partially from  the cache) the header are merged, header from the cache are merged with headers from the network. We can expose cached headers. I have already added "the original headers" these are headers in the exact form as received from the network. I could expose the cached headers as well - "the original headers that came from network for content that is cached".

The timeline is the same thing as above.

> Sorry if this muddles the waters.

	Dragana Damjanovic [:dragana] Assignee
	Comment 13 • 9 years ago

Cached headers can be obtained by reading from the cached. Original headers received from the network at the time cache entry is created are stored in the cache. They are not updated if request is served from the cache (partially or completely).

Cached headers can be obtained from the cache similar as the content is obtained from the cache:
http://mxr.mozilla.org/mozilla-central/source/toolkit/devtools/webconsole/network-monitor.js#253

	Decky Coss (:deckycoss)
	Comment 15 • 8 years ago

i ran into the blank headers bug while investigating the failing cors/response-headers.htm web platform test (http://mxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/cors/response-headers.htm?force=1%27#70). the test expects `getResponseHeader("x-custom-header-empty")` to return the empty string, but instead it returns null. this is presumably because nsHttpHeaderArray::GetHeader cannot find an entry for "x-custom-header-empty".

the headers for the test are defined here: http://mxr.mozilla.org/mozilla-central/source/testing/web-platform/tests/cors/resources/cors-headers.asis

	Honza Bambas (:mayhemer)
	Comment 16 • 8 years ago

Comment on attachment 8736269 [details] [diff] [review]
bug_669259_v1.patch

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

Patrick, this particular code is more your domain.  Do you have time to do the review?  If not, feel free to bounce back on me.  Thanks.

	Patrick McManus [:mcmanus]
	Comment 17 • 8 years ago

Comment on attachment 8736269 [details] [diff] [review]
bug_669259_v1.patch

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

I have a concern here that this essentially doubles the size of the response header ipc transfer for the sake of devtools which is usually not active.. and indeed usually it will have the same data as the original set of headers.

I offer two solutions but am open to others:
 * if the sets of headers are the same (i.e. no empty headers) have the new interface reference the old headers and don't keep new ones
 * is there some service we can query (or have devtools opt into a service interface) to conditionally enable this? when we don't enable it just return NS_ERROR_NOT_AVAILABLE

::: devtools/shared/webconsole/network-monitor.js
@@ -614,5 @@
>      };
>  
>      let setCookieHeader = null;
>  
> -    channel.visitResponseHeaders({

please get a review from the devtools team for this bit of code

::: modules/libpref/init/all.js
@@ +1502,5 @@
>  pref("network.http.signed-packages.enabled", false);
>  
> +// If it is set to false, headers with empty value will not appear in the header
> +// array - behavior as it used to be. If it is true: empty headers coming from
> +// the network will exits in header array as empty string. Call SetHeader with

typo exits/exist

::: netwerk/protocol/http/nsHttpResponseHead.cpp
@@ +123,5 @@
>      ParseStatusLine(block);
>  
>      do {
>          block = p + 2;
> +        

whitespace

::: netwerk/protocol/http/nsIHttpChannel2.idl
@@ +7,5 @@
> +
> +interface nsIHttpHeaderVisitor;
> +
> +/**
> + * nsIHttpChannel2.idl

no need for a new idl here.. add it to nsIHttpChannel.idl

::: netwerk/test/unit/test_original_sent_received_head.js
@@ +8,5 @@
> +//  and the form as they have been received from the network.
> +//  Here, the "original headers" are tested.
> +//
> +
> +// Note: sets Cc and Ci variables

please add an e10s version of the test

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

This is a different approach.
nsHttpHeaderArray was a strange structure already (used by request and response headers which need a bit different functions). So I just extended that a bit :)

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

Comment on attachment 8748913 [details] [diff] [review]
bug_669259_v1.patch

May I ask you for a review of the devtool part?

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

I have change Flatten() function, so now it behaves as it use to.
I will change the cache behaviour in a separate bug, which Honza Bambas needs to review.

	Patrick McManus [:mcmanus]
	Comment 21 • 8 years ago

Comment on attachment 8749101 [details] [diff] [review]
bug_669259_v1.patch

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

I like this a lot better!

::: netwerk/protocol/http/nsHttpResponseHead.cpp
@@ +108,5 @@
>      do {
>          block = p + 2;
>  
> +        if (*block == 0)
> +			      break;

spacing looks weird in splinter

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +1563,5 @@
>          if (LOG3_ENABLED()) {
>              LOG3(("http response [\n"));
>              nsAutoCString headers;
>              mResponseHead->Flatten(headers, false);
> +            mResponseHead->FlattenOriginalHeader(headers);

would probably be good to comment in the flatten declarations that they append rather than set

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

I have removed the devtools change. That should be done in a separate bug and I do not need a review from devtools.

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1e8864cee063

	Pulsebot
	Comment 25 • 8 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/d10dd5841d7f

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

https://hg.mozilla.org/mozilla-central/rev/d10dd5841d7f

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

It looks like we ended up with the preference 'network.http.keep_empty_response_headers_as_empty_string', which only keeps empty header values when accessing them via the nsIHttpChannel methods, right?

What happened to point 2 and 3 of comment 0 and comment 1? And why was it decided to implement this behind a pref instead of introducing a new function, which returns the raw headers?

Sebastian

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

(In reply to Sebastian Zartner [:sebo] from comment #27)
> It looks like we ended up with the preference
> 'network.http.keep_empty_response_headers_as_empty_string', which only keeps
> empty header values when accessing them via the nsIHttpChannel methods,
> right?
> 
> What happened to point 2 and 3 of comment 0 and comment 1? And why was it
> decided to implement this behind a pref instead of introducing a new
> function, which returns the raw headers?
> 
> Sebastian

Hi,
we have that too.

So we have 2 new functions in nsIHttpChannel:
void visitOriginalResponseHeaders(in nsIHttpHeaderVisitor aVisitor); - is used to visit headers in the same form as when they came from the network, e.g. the headers will not be merged, the empty headers will be shown.

void getOriginalResponseHeader(in ACString aHeader, in nsIHttpHeaderVisitor aVisitor); - is use to visit specific header as it came from the network. Since they will not be merge there could be multiple values for the same header so it needs nsIHttpHeaderVisitor and not just nsACString.

About the empty headers we decided to try to keep them always and hope that we are not going to break something. Till now we where keeping them for some special headers like Location and from now we will keep them for all. I have not turn on pref because the test in bug 918721 needs to be fix at the same time.

We have chosen to use a pref in case we break something we can fix it fast. (pref is only for keeping empty headers, the 2 functions are permanent)

I have not change devTools(network-monitor.js) that needs to be done in a separate bug, we would probably like to show both versions.

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

Thank you for the detailed info, Dragana!

(In reply to Dragana Damjanovic [:dragana] from comment #28)
> (In reply to Sebastian Zartner [:sebo] from comment #27)
> I have not change devTools(network-monitor.js) that needs to be done in a
> separate bug, we would probably like to show both versions.

I've created bug 1273503 for that.

Sebastian

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

I've updated the documentation at https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIHttpChannel.
Please verify if I missed anything, especially in regard of the preference.

(In reply to Patrick McManus [:mcmanus] from comment #17)
> ::: modules/libpref/init/all.js
> @@ +1502,5 @@
> >  pref("network.http.signed-packages.enabled", false);
> >  
> > +// If it is set to false, headers with empty value will not appear in the header
> > +// array - behavior as it used to be. If it is true: empty headers coming from
> > +// the network will exits in header array as empty string. Call SetHeader with
> 
> typo exits/exist
> 
> ::: netwerk/protocol/http/nsHttpResponseHead.cpp
> @@ +123,5 @@
> >      ParseStatusLine(block);
> >  
> >      do {
> >          block = p + 2;
> > +        
> 
> whitespace

As far as I can see, those two points were not addressed in your patch.

Sebastian

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

(In reply to Sebastian Zartner [:sebo] from comment #30)
> I've updated the documentation at
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsIHttpChannel.
> Please verify if I missed anything, especially in regard of the preference.
> 

Looks good.
Small fix needed "empty headers will be have an empty string" remove "be" 

There is a note about calling setResponseHeader() during visitHeaders. In bug 507571 and 1274509 I have change the behavior so now call to this functions during visitHeaders will return an error.
Also call to referrer (only setting it, get will work fine) and I miss note for setReferrerWithPolicy(will fix that) 
I will mark bugs as dev-doc-needed

> (In reply to Patrick McManus [:mcmanus] from comment #17)
> > ::: modules/libpref/init/all.js
> > @@ +1502,5 @@
> > >  pref("network.http.signed-packages.enabled", false);
> > >  
> > > +// If it is set to false, headers with empty value will not appear in the header
> > > +// array - behavior as it used to be. If it is true: empty headers coming from
> > > +// the network will exits in header array as empty string. Call SetHeader with
> > 
> > typo exits/exist
> > 
> > ::: netwerk/protocol/http/nsHttpResponseHead.cpp
> > @@ +123,5 @@
> > >      ParseStatusLine(block);
> > >  
> > >      do {
> > >          block = p + 2;
> > > +        
> > 
> > whitespace
> 
> As far as I can see, those two points were not addressed in your patch.
> 
> Sebastian

I have fix this in another bug.

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

(In reply to Dragana Damjanovic [:dragana] from comment #31)
> (In reply to Sebastian Zartner [:sebo] from comment #30)
> > I've updated the documentation at
> > https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> > Interface/nsIHttpChannel.
> > Please verify if I missed anything, especially in regard of the preference.
> > 
> 
> Looks good.
> Small fix needed "empty headers will be have an empty string" remove "be"

Fixed that. Thanks!

> There is a note about calling setResponseHeader() during visitHeaders. In
> bug 507571 and 1274509 I have change the behavior so now call to this
> functions during visitHeaders will return an error.
> Also call to referrer (only setting it, get will work fine) and I miss note
> for setReferrerWithPolicy(will fix that) 
> I will mark bugs as dev-doc-needed

Good, I'll have a look at them.

Sebastian

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

I've totally missed before that this change also affects the output of the web-facing XMLHttpRequest.getResponseHeader() and XMLHttpRequest.getAllResponseHeaders(). So, I've added a note about this to the following pages:

https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/getResponseHeader
https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/getAllResponseHeaders
https://developer.mozilla.org/en-US/Firefox/Releases/49

Dragana, can you please verify again that I covered everything now? Probably I should also add a note about this to https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIHttpChannel#getResponseHeader(), right?

Sebastian

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

(In reply to Sebastian Zartner [:sebo] from comment #33)
> I've totally missed before that this change also affects the output of the
> web-facing XMLHttpRequest.getResponseHeader() and
> XMLHttpRequest.getAllResponseHeaders(). So, I've added a note about this to
> the following pages:
> 
> https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/
> getResponseHeader
> https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/
> getAllResponseHeaders
> https://developer.mozilla.org/en-US/Firefox/Releases/49
> 

Thank! Looks good, I would write a longer explanation, something like: "if a empty string has been received, the function will return empty string if pref ... is set. Before 49 empty headers had been ignored."

Maybe it is ok as it is, and it is just me :)

(FF kept track of empty values for Content length and Location even before 49)

> Dragana, can you please verify again that I covered everything now? Probably
> I should also add a note about this to
> https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/
> Interface/nsIHttpChannel#getResponseHeader(), right?
> 

Yes, Thanks!

> Sebastian

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

(In reply to Dragana Damjanovic [:dragana] from comment #34)
> (In reply to Sebastian Zartner [:sebo] from comment #33)
> > I've totally missed before that this change also affects the output of the
> > web-facing XMLHttpRequest.getResponseHeader() and
> > XMLHttpRequest.getAllResponseHeaders(). So, I've added a note about this to
> > the following pages:
> > 
> > https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/
> > getResponseHeader
> > https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/
> > getAllResponseHeaders
> > https://developer.mozilla.org/en-US/Firefox/Releases/49
> > 
> 
> Thank! Looks good, I would write a longer explanation, something like: "if a
> empty string has been received, the function will return empty string if
> pref ... is set. Before 49 empty headers had been ignored."

Thank you for the feedback! I've adjusted the notes accordingly.

Sebastian