Closed Bug 1447679 Opened 2 years ago Closed 2 years ago

Raw headers are displayed in alphabetical order

Categories

(DevTools :: Netmonitor, defect, P1)

59 Branch
x86_64
Windows 10
defect

Tracking

(firefox-esr52 unaffected, firefox59 wontfix, firefox60 fixed, firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- wontfix
firefox60 --- fixed
firefox61 --- fixed

People

(Reporter: mail.josephpg, Assigned: Honza)

References

Details

(Keywords: regression)

Attachments

(1 file)

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
OS: Unspecified → Windows 10
Hardware: Unspecified → x86_64
Summary: Raw headers doesn't preserve original headers order → Raw headers are displayed in alphabetical order
https://hg.mozilla.org/integration/autoland/json-pushes?changeset=1a34671e1b5f66ff4d8215ca13423cb17c92346d&full=1
Blocks: 1427718
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
Component: Untriaged → Developer Tools: Netmonitor
Ever confirmed: true
Flags: needinfo?(odvarko)
Keywords: regression
Assignee: nobody → odvarko
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)
Priority: -- → P1
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]
@Ryan, feel free to forward the review (or tell me). It just feels
you are still member of the DevTools team :-)
Honza
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?
Attachment #8961349 - Flags: review?(jryans) → review+
(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
Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/25b280ef82c9
Raw headers are displayed in alphabetical order; r=jryans
https://hg.mozilla.org/mozilla-central/rev/25b280ef82c9
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Seems worth an uplift request for 60.
Flags: needinfo?(odvarko)
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
Flags: needinfo?(odvarko)
Attachment #8961349 - Flags: approval-mozilla-beta?
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 on attachment 8961349 [details]
Bug 1447679 - Raw headers are displayed in alphabetical order;

devtools net panel fix, beta60+
Attachment #8961349 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.