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

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P3)

Description

[Fernando García Gómez, stripTM]

Attachment: NetPanel in Firefox vs chrome

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

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 2

[Fernando García Gómez, stripTM]

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.

Comment 3

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Comment 4

[Michal Novotny [:michal]]

(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/, *

Comment 5

[Fernando García Gómez, stripTM]

So, in this case it should be shown this?

Access-Control-Allow-Origin: , *

or

Access-Control-Allow-Origin: (null), *

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Just NI Michal for comment #5

Honza

Comment 7

[Michal Novotny [:michal]]

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.

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Note that it might be useful for web-dev folks to see that some headers are actually duplicated.

Honza

Comment 9

[Honza Bambas (:mayhemer)]

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

Comment 10

[Dragana Damjanovic [:dragana]]

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

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 12

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 13

[Honza Bambas (:mayhemer)]

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

Comment 14

[Dragana Damjanovic [:dragana]]

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

Comment 15

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Bug 1427718 - Display multiple headers with the same name;

Review commit: https://reviewboard.mozilla.org/r/213206/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/213206/

Comment 16

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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

Comment 17

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=cd321f5574abecd2f6d0063d90d1b405899696a4
Honza

Comment 18

[Ricky Chien [:rickychien] (inactive)]

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

Comment 19

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213206/diff/1-2/

Comment 20

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(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 21

[Ricky Chien [:rickychien] (inactive)]

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.

Comment 22

[Jan Honza Odvarko [:Honza] (always need-info? me)]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/213206/diff/2-3/

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1a34671e1b5f
Display multiple headers with the same name; r=rickychien

Comment 24

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/1a34671e1b5f