1447679 - Raw headers are displayed in alphabetical order

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[Joseph Aditya P. G.]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:59.0) Gecko/20100101 Firefox/59.0
Build ID: 20180315233128

Steps to reproduce:

1. Open dev tool
2. Go to network monitor
3. Go to any sites
4. Select any row and details panel will be displayed
5. Click the "Raw headers" button
6. Compare the ordering of the raw and processed response headers


Actual results:

The raw headers field sorts the response headers in alphabetical order just like the processed headers. This doesn't happen in Firefox 58.

Example:
    HTTP/2.0 200 OK
    content-encoding: gzip
    content-security-policy: frame-ancestors 'none'
    content-type: text/html; charset=utf-8
    date: Wed, 21 Mar 2018 14:38:16 GMT
    referrer-policy: strict-origin-when-cross-origin
    server: nginx
    strict-transport-security: max-age=63072000; includeSubDomains
    X-Firefox-Spdy: h2


Expected results:

The raw headers field preserve the original ordering of the response headers

Example:
    HTTP/2.0 200 OK
    server: nginx
    date: Wed, 21 Mar 2018 14:37:36 GMT
    content-encoding: gzip
    content-type: text/html; charset=utf-8
    content-security-policy: frame-ancestors 'none'
    referrer-policy: strict-origin-when-cross-origin
    strict-transport-security: max-age=63072000; includeSubDomains
    X-Firefox-Spdy: h2

Comment 1

[Gingerbread Man]

https://hg.mozilla.org/integration/autoland/json-pushes?changeset=1a34671e1b5f66ff4d8215ca13423cb17c92346d&full=1

Comment 2

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

Attachment: Bug 1447679 - Raw headers are displayed in alphabetical order;

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

Comment 3

[Code Review Bot [:reviewbot]]

Comment on attachment 8961349 [details]
Bug 1447679 - Raw headers are displayed in alphabetical order;

https://reviewboard.mozilla.org/r/230138/#review235764


Code analysis found 1 defect in this patch:
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint path/to/file` (JS/Python)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: devtools/client/netmonitor/test/browser_net_headers_sorted.js:130
(Diff revision 1)
> +  is(actualResponseHeaders.toString(), expectedResponseHeaders.toString(),
> +    "Raw Response Headers are not sorted");
> +
> +  is(actualRequestHeaders.toString(), expectedRequestHeaders.toString(),
> +    "Raw Request Headers are not sorted");
> +}

Error: Newline required at end of file but not found. [eslint: eol-last]

Comment 4

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

Comment on attachment 8961349 [details]
Bug 1447679 - Raw headers are displayed in alphabetical order;

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

Comment 5

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

@Ryan, feel free to forward the review (or tell me). It just feels
you are still member of the DevTools team :-)
Honza

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8961349 [details]
Bug 1447679 - Raw headers are displayed in alphabetical order;

https://reviewboard.mozilla.org/r/230138/#review235906

Thanks for working on this! :) Looks good to me.

Seems like a good candidate for uplift to 60 as well?

::: devtools/client/netmonitor/src/components/HeadersPanel.js:223
(Diff revision 2)
>          code = status;
>        }
>  
>        let statusCodeDocURL = getHTTPStatusCodeURL(status.toString());
>        let inputWidth = status.toString().length + statusText.length + 1;
> -      let toggleRawHeadersClassList = ["devtools-button"];
> +      let toggleRawHeadersClassList = ["devtools-button raw-headers-button"];

Seems like this is an array that is joined manually down below...  Wouldn't a separate array entry be more appropriate here?

Comment 7

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

Comment on attachment 8961349 [details]
Bug 1447679 - Raw headers are displayed in alphabetical order;

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

Comment 8

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

(In reply to J. Ryan Stinnett [:jryans] (use ni?) from comment #6)
> Seems like a good candidate for uplift to 60 as well?
Yes

> ::: devtools/client/netmonitor/src/components/HeadersPanel.js:223
> (Diff revision 2)
> >          code = status;
> >        }
> >  
> >        let statusCodeDocURL = getHTTPStatusCodeURL(status.toString());
> >        let inputWidth = status.toString().length + statusText.length + 1;
> > -      let toggleRawHeadersClassList = ["devtools-button"];
> > +      let toggleRawHeadersClassList = ["devtools-button raw-headers-button"];
> 
> Seems like this is an array that is joined manually down below...  Wouldn't
> a separate array entry be more appropriate here?
Ah, yes, you are right, fixed.

Thanks Ryan for the review!

Honza

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/25b280ef82c9
Raw headers are displayed in alphabetical order; r=jryans

Comment 10

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/25b280ef82c9

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Seems worth an uplift request for 60.

Comment 12

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

Comment on attachment 8961349 [details]
Bug 1447679 - Raw headers are displayed in alphabetical order;

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1427718
[User impact if declined]: Wrong headers order in DevTools Network panel
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: n/a
[Is the change risky?]: Low risk
[Why is the change risky/not risky?]: Relatively small patch, only for web-developers
[String changes made/needed]: n/a

Honza

Comment 13

[Joseph Aditya P. G.]

Just did a test on Firefox Nightly 61.0a1 (2018-03-26) and this bug has been resolved.

Thanks for all the works, @Honza and @jryans!

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 8961349 [details]
Bug 1447679 - Raw headers are displayed in alphabetical order;

devtools net panel fix, beta60+

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/79c7a5ef721f