The same header with different values is not reflected in the net panel

RESOLVED FIXED in Firefox 59

Status

defect
P3
normal
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: striptm, Assigned: Honza)

Tracking

({nightly-community})

Trunk
Firefox 59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

Attachments

(2 attachments)

When the same header is received with two different values, it would be desirable to have its reflection on the panel.

Currently it only shows one of them.

Example:
HTTP/1.1 200 OK
Accept-Ranges: bytes
Access-Control-Allow-Origin:
Access-Control-Allow-Origin: *
Cache-Control: max-age=60,public, max-age=60, public
Connection: keep-alive
Content-Length: 6452
Content-Type: application/json
Date: Wed, 03 Jan 2018 11:52:50 GMT
X-Cache-Hits: 0

And attached as shown in Firefox and Chrome
I agree, this should be fixed.

@Fernando, do you have a test case (eg an online page or a file you could attach to this bug) that we could use to easily reproduce the problem?

Thanks for the report!
Honza
Has STR: --- → yes
Flags: needinfo?(striptm)
Priority: -- → P3
Yes, you can try it on this url:
http://striptm.com/ejemplos/double_header.php

HTTP/1.1 200 OK
Access-Control-Allow-Origin:
Access-Control-Allow-Origin: *
Connection: keep-alive
Content-Length: 2
Content-Type: text/html; charset=UTF-8
Date: Wed, 03 Jan 2018 12:59:01 GMT
Server: nginx/1.7.11
X-Powered-By: PHP/5.6.24

Thanks for the quick response.
Flags: needinfo?(striptm)
Thanks Fernando!

---

Michal, this seems to be Necko issue since raw headers we get from nsIHttpActivityObserver are already having just one header (in case of a dup).

Here is related piece of code in devtool (extraStringData == raw headers):
https://searchfox.org/mozilla-central/rev/51cd1093c5c2113e7b041e8cc5c9bf2186abda13/devtools/shared/webconsole/network-monitor.js#1035

Can we fix that in Necko?

Honza
Flags: needinfo?(michal.novotny)
(In reply to Jan Honza Odvarko [:Honza] from comment #3)
> Michal, this seems to be Necko issue since raw headers we get from
> nsIHttpActivityObserver are already having just one header (in case of a
> dup).

Duplicate headers are merged into a single one, so no information is lost. The problem in the example is that the first one has no effect so it's not used in the merged header. But if you send:

Access-Control-Allow-Origin: http://www.example.com/
Access-Control-Allow-Origin: *

you'll get:

Access-Control-Allow-Origin: http://www.example.com/, *
Flags: needinfo?(michal.novotny)
So, in this case it should be shown this?

Access-Control-Allow-Origin: , *

or

Access-Control-Allow-Origin: (null), *
Just NI Michal for comment #5

Honza
Flags: needinfo?(michal.novotny)
If you want to see headers that was sent by the server, we would need to use FlattenNetworkOriginalHeaders here:

https://searchfox.org/mozilla-central/rev/b24e6342d744c5a83fab5c15972e11eeb69d68e6/netwerk/protocol/http/nsHttpTransaction.cpp#1849

I don't know whether there is some reason not to do it. Maybe Honza knows.
Flags: needinfo?(michal.novotny) → needinfo?(honzab.moz)
Note that it might be useful for web-dev folks to see that some headers are actually duplicated.

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #3)
> Here is related piece of code in devtool (extraStringData == raw headers):
> https://searchfox.org/mozilla-central/rev/
> 51cd1093c5c2113e7b041e8cc5c9bf2186abda13/devtools/shared/webconsole/network-
> monitor.js#1035

This place deals with request headers, not response headers.

(In reply to Michal Novotny (:michal) from comment #7)
> If you want to see headers that was sent by the server, we would need to use
> FlattenNetworkOriginalHeaders here:
> 
> https://searchfox.org/mozilla-central/rev/
> b24e6342d744c5a83fab5c15972e11eeb69d68e6/netwerk/protocol/http/
> nsHttpTransaction.cpp#1849
> 
> I don't know whether there is some reason not to do it. Maybe Honza knows.

Thanks for pointing to the right place, Michal.  I assume the only consumer these days are dev-tools.

I'm just afraid that we may loose artificially added response headers such as X-Firefox-Spdy and maybe some others that may not be shows when using FlattenNetworkOriginalHeaders.

Worth a try, also Dragana made a lot of changes around that code some time ago, it may be better to ask her.
Flags: needinfo?(honzab.moz)
(In reply to Jan Honza Odvarko [:Honza] from comment #8)
> Note that it might be useful for web-dev folks to see that some headers are
> actually duplicated.
> 
> Honza

I added getOriginalResponseHeader and visitOriginalResponseHeaders for devtools.  I think there is a bug for this issue already.

The response headers are collected using visitors at:
https://dxr.mozilla.org/mozilla-central/source/devtools/shared/webconsole/network-monitor.js#879

I would suggest to show both: the original received from the server and visitResponseHeaders which shows how firefox sees the original headers received from the network.
(In reply to Dragana Damjanovic [:dragana] from comment #10)
> I added getOriginalResponseHeader and visitOriginalResponseHeaders for
> devtools.  I think there is a bug for this issue already.
Ok, great, `visitOriginalResponseHeaders` works well.

However, when I am setting multiple headers of the same name in tests (in *.sjs file),
I am again seeing all values merged into one. Does the HTTPServer (for tests) need
to be fixed too?

An example:

function handleRequest(request, response) {
  response.setHeader("Foo-Bar", "baz", true);
  response.setHeader("Foo-Bar", "baz", true);
  response.setHeader("Foo-Bar", "", true);

  ...
}

Honza
Flags: needinfo?(dd.mozilla)
(In reply to Jan Honza Odvarko [:Honza] from comment #11)
> (In reply to Dragana Damjanovic [:dragana] from comment #10)
> > I added getOriginalResponseHeader and visitOriginalResponseHeaders for
> > devtools.  I think there is a bug for this issue already.
> Ok, great, `visitOriginalResponseHeaders` works well.
> 
> However, when I am setting multiple headers of the same name in tests (in
> *.sjs file),
I am talking about mochitests here.
Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #12)
> I am talking about mochitests here.
> Honza

you can seizePower the response and write response line, headers and body directly.
(In reply to Jan Honza Odvarko [:Honza] from comment #11)
> (In reply to Dragana Damjanovic [:dragana] from comment #10)
> > I added getOriginalResponseHeader and visitOriginalResponseHeaders for
> > devtools.  I think there is a bug for this issue already.
> Ok, great, `visitOriginalResponseHeaders` works well.
> 
> However, when I am setting multiple headers of the same name in tests (in
> *.sjs file),
> I am again seeing all values merged into one. Does the HTTPServer (for
> tests) need
> to be fixed too?
> 
> An example:
> 
> function handleRequest(request, response) {
>   response.setHeader("Foo-Bar", "baz", true);
>   response.setHeader("Foo-Bar", "baz", true);
>   response.setHeader("Foo-Bar", "", true);
> 
>   ...
> }
> 
> Honza

I have added function setHeaderNoCheck which will not do any merge or checks, like singleton headers will appear only once..
This is used in test netwerk/test/unit/test_original_sent_received_head.js which tests visitOriginalResponseHeaders.
Flags: needinfo?(dd.mozilla)
(In reply to Dragana Damjanovic [:dragana] from comment #14)
> I have added function setHeaderNoCheck which will not do any merge or
> checks, like singleton headers will appear only once..
Yep, that's what I need, thanks.
Honza
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Comment on attachment 8942940 [details]
Bug 1427718 - Display multiple headers with the same name;

https://reviewboard.mozilla.org/r/213206/#review219130

Thanks Honza, this patch functions correctly on both toolbox & launchpad.

Overall looks great to me, I've only one architecture suggestion and it needs a one more round review. I think it's minor issue, so don't worry.

Please see below.

::: devtools/client/netmonitor/src/components/HeadersPanel.js:24
(Diff revision 1)
>    fetchNetworkUpdatePacket,
>    writeHeaderText,
>  } = require("../utils/request-utils");
> -const { sortObjectKeys } = require("../utils/sort-utils");
>  
> +const { ObjectProvider } = require("devtools/client/shared/components/tree/ObjectProvider");

nit: moving this kind of "devtools/client..." path require statement below `const dom = ...`

::: devtools/client/netmonitor/src/components/HeadersPanel.js:319
(Diff revision 1)
> + * set of headers. The default ObjectProvider can't be
> + * used since it doesn't allow duplicities by design and
> + * so it can't support duplicity headers (more headers
> + * with the same name).
> + */
> +var HeadersProvider = Object.assign({}, ObjectProvider, {

nit: I'd suggest to use 

```
var HeadersProvider = { ...ObjectProvider, {
  ...
}}
```

::: devtools/client/netmonitor/src/components/HeadersPanel.js:319
(Diff revision 1)
> + * set of headers. The default ObjectProvider can't be
> + * used since it doesn't allow duplicities by design and
> + * so it can't support duplicity headers (more headers
> + * with the same name).
> + */
> +var HeadersProvider = Object.assign({}, ObjectProvider, {

Please move this provider code into a separate module under src/utils/ like as filter-autocomplete-provider.js

I'd suggest the name using duplicity-tree-provider.js or something similarly.

::: devtools/client/netmonitor/src/components/PropertiesView.js:210
(Diff revision 1)
>              }),
>            ),
>          div({ className: "tree-container" },
>            TreeView({
>              object,
> +            provider: this.props.provider,

nit: extract provider props in above destructing assignment.

::: devtools/client/netmonitor/src/connector/firefox-data-provider.js:556
(Diff revision 1)
>     * Handles additional information received for a "responseCookies" packet.
>     *
>     * @param {object} response the message received from the server.
>     */
>    async onResponseCookies(response) {
> +    console.log("=====> response cookies", response);

nit: please remove this log
Attachment #8942940 - Flags: review?(rchien) → review-
(In reply to Ricky Chien [:rickychien] from comment #18)
> Comment on attachment 8942940 [details]
> Bug 1427718 - Display multiple headers with the same name;
> 
> https://reviewboard.mozilla.org/r/213206/#review219130
> 
> Thanks Honza, this patch functions correctly on both toolbox & launchpad.
Thank for the review Ricky!

All fixed, except of the one comment below.

> ::: devtools/client/netmonitor/src/components/HeadersPanel.js:24
> (Diff revision 1)
> >    fetchNetworkUpdatePacket,
> >    writeHeaderText,
> >  } = require("../utils/request-utils");
> > -const { sortObjectKeys } = require("../utils/sort-utils");
> >  
> > +const { ObjectProvider } = require("devtools/client/shared/components/tree/ObjectProvider");
> 
> nit: moving this kind of "devtools/client..." path require statement below
> `const dom = ...`
Not sure what you mean by this...?

Honza
Comment on attachment 8942940 [details]
Bug 1427718 - Display multiple headers with the same name;

https://reviewboard.mozilla.org/r/213206/#review219198

r+ LGTM for this patch. Thank you Honza for this work!

> Not sure what you mean by this...?

Nevermind :) This issue is fixed naturally once you extract headersProvider into a separate file.
Attachment #8942940 - Flags: review?(rchien) → review+
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1a34671e1b5f
Display multiple headers with the same name; r=rickychien
https://hg.mozilla.org/mozilla-central/rev/1a34671e1b5f
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 59
Duplicate of this bug: 1430674
Depends on: 1447679
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.