Network tab protocol column shows "HTTP/2.0+h2" when individual requests are selected
Categories
(DevTools :: Netmonitor, defect, P2)
Tracking
(firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 fixed)
People
(Reporter: lucaspardue.24.7, Assigned: asorholm, Mentored)
Details
(Keywords: good-first-bug, Whiteboard: good-first-bug)
Attachments
(2 files, 2 obsolete files)
75.17 KB,
image/png
|
Details | |
7.32 KB,
patch
|
Honza
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:62.0) Gecko/20100101 Firefox/62.0 Steps to reproduce: * Open dev tools Network tab and enable the Protocol column * Visit https://mozilla.org * See there are some requests listed with protocol "HTTP/2.0" * Click a request * See that the protocol value changes to "HTTP/2.0+h2" Actual results: HTTP/2 requests are rendered as "HTTP/2.0+h2" when clicked. Expected results: HTTP/2 request protocol value remains unchanged when it is clicked.
Comment 1•6 years ago
|
||
Hi Lucas, Reproduce the issue on the latest Nightly 65.0a1 (2018-10-25), Firefox Beta 64.0b3 and Firefox Release 63.0 on Windows 10 x64. Thanks for the report!
Comment 2•6 years ago
|
||
Looks like a problem with the devtools UI.
Comment 3•6 years ago
|
||
Thanks for the report! I can also reproduce the issue on my machine. Honza
Comment 4•6 years ago
|
||
Some info for anyone interested in fixing this issue: 1) Here is the place where the Protocol columns renders its values https://searchfox.org/mozilla-central/rev/5b3b6b8fd9f90087f618c20382e631451136ed2b/devtools/client/netmonitor/src/components/RequestListColumnProtocol.js#27 2) Here is the function that looks for the value in HTTP headers https://searchfox.org/mozilla-central/rev/5b3b6b8fd9f90087f618c20382e631451136ed2b/devtools/client/netmonitor/src/utils/request-utils.js#429 It might be that headers are not available until the Side panel is opened? Honza
(In reply to Jan Honza Odvarko [:Honza] (need-info? me) from comment #4) > Some info for anyone interested in fixing this issue: > > 1) Here is the place where the Protocol columns renders its values > https://searchfox.org/mozilla-central/rev/ > 5b3b6b8fd9f90087f618c20382e631451136ed2b/devtools/client/netmonitor/src/ > components/RequestListColumnProtocol.js#27 > > 2) Here is the function that looks for the value in HTTP headers > https://searchfox.org/mozilla-central/rev/ > 5b3b6b8fd9f90087f618c20382e631451136ed2b/devtools/client/netmonitor/src/ > utils/request-utils.js#429 > > It might be that headers are not available until the Side panel is opened? > > Honza I'd be interested in working on this.
So it looks like this behaviour was added in bug 736882 - that was specifically to show the underlying protocol information for spdy (which kept "HTTP/1.1" as the version in the response). That's a reasonable thing to do, I would say. However, I don't think it makes a whole lot of sense to ever show "HTTP/2.0+h2" - that's totally redundant (and also, as noted here, buggy). Perhaps it would make the most sense to only show the "+<alpn>" bit if the alpn (x-firefox-spdy header) doesn't match the expected default for the version in the response? So, if we still supported spdy (we don't), showing "HTTP/1.1+spdy/3.1" would still happen, but we wouldn't show "HTTP/1.1+http/1.1" or "HTTP/2.0+h2", ever. Now, if we were to add a new lower-level protocol (let's take h3 as an example) but not change the version in the response, that would make sense to show - so potentially you could have "HTTP/2.0+h3". Just something to think about as you design your solution.
Comment 7•6 years ago
|
||
(In reply to Nicholas Hurley [:nwgh][:hurley] (he/him) from comment #6) > So it looks like this behaviour was added in bug 736882 - that was > specifically to show the underlying protocol information for spdy (which > kept "HTTP/1.1" as the version in the response). That's a reasonable thing > to do, I would say. However, I don't think it makes a whole lot of sense to > ever show "HTTP/2.0+h2" - that's totally redundant (and also, as noted here, > buggy). > Perhaps it would make the most sense to only show the "+<alpn>" bit if the > alpn (x-firefox-spdy header) doesn't match the expected default for the > version in the response? Thanks for the comment! > So, if we still supported spdy (we don't), showing > "HTTP/1.1+spdy/3.1" would still happen, but we wouldn't show > "HTTP/1.1+http/1.1" or "HTTP/2.0+h2", ever. Now, if we were to add a new > lower-level protocol (let's take h3 as an example) but not change the > version in the response, that would make sense to show - so potentially you > could have "HTTP/2.0+h3". So, can we somehow define a table of expected results (rendered in the Protocol column), so we can actually implement the suggested logic? Something like the following: HTTP version | x-firefox-spdy | Protocol Column ------------------------------------------------------ HTTP/1.1 http/1.1 HTTP/1.1 HTTP/2.0 h2 HTTP/2.0 HTTP/1.1 * HTTP/1.1+* HTTP/2.0 * HTTP/2.0+* ------------------------------------------------------ Where `*` means any value different from `http/1.1` and `h2` Does that make sense? Honza
Comment 8•6 years ago
|
||
(In reply to Adam Holm from comment #5) > I'd be interested in working on this. Sure, assigned to you, thanks! Honza
(In reply to Jan Honza Odvarko [:Honza] (need-info? me) from comment #7) > So, can we somehow define a table of expected results (rendered in the > Protocol column), so we can actually implement the suggested logic? > > Something like the following: > > HTTP version | x-firefox-spdy | Protocol Column > ------------------------------------------------------ > HTTP/1.1 http/1.1 HTTP/1.1 > HTTP/2.0 h2 HTTP/2.0 > HTTP/1.1 * HTTP/1.1+* > HTTP/2.0 * HTTP/2.0+* > ------------------------------------------------------ > > Where `*` means any value different from `http/1.1` and `h2` > > Does that make sense? Yep, that makes perfect sense. Still doesn't solve the bug of the +<foo> not showing up until you click the request (which is the other half of what this bug was originally about), but I think it's reasonable to solve the two simultaneously.
Comment 10•6 years ago
|
||
@Adam, so two things should be solved in this bug report: 1) The value rendered in Protocol column should be calculated according to the table from comment #7 2) The value should show immediately (not after clicking the request) Honza
Assignee | ||
Comment 11•6 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] (need-info? me) from comment #8) > (In reply to Adam Holm from comment #5) > > I'd be interested in working on this. > Sure, assigned to you, thanks! > > Honza (In reply to Jan Honza Odvarko [:Honza] (need-info? me) from comment #10) > @Adam, so two things should be solved in this bug report: > > 1) The value rendered in Protocol column should be calculated according to > the table from comment #7 > 2) The value should show immediately (not after clicking the request) > > Honza Thanks! The explanation has been very helpful.
Assignee | ||
Comment 12•6 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] (need-info? me) from comment #10) > @Adam, so two things should be solved in this bug report: > > 1) The value rendered in Protocol column should be calculated according to > the table from comment #7 > 2) The value should show immediately (not after clicking the request) > > Honza I think I have a solution, but I'd like to write some tests for it before submitting a patch for feedback (I'd like to pass |getFormattedProtocol| protocol values and see if it pushes the correct values to |protocol|). Is xpcshell the correct testing suite to use for this purpose?
Comment 13•6 years ago
|
||
(In reply to Adam Holm from comment #12) > I think I have a solution, but I'd like to write some tests for it before > submitting a patch for feedback (I'd like to pass |getFormattedProtocol| > protocol values and see if it pushes the correct values to |protocol|). Is > xpcshell the correct testing suite to use for this purpose? All network monitor tests are mochitests, I think we should use it even for this bug. Unless it isn't possible? Honza
Assignee | ||
Comment 14•5 years ago
|
||
Here's what I have so far for a fix for this bug. I included the test file I created, but if you'd like a patch file without the test file, let me know and I'll attach one.
Comment 15•5 years ago
|
||
Comment on attachment 9029381 [details] [diff] [review] bug1501357.patch Review of attachment 9029381 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch and sorry for the delay! Please see my inline comments. Also, thanks for the test. It's totally ok to have it as part of the same patch. I'd like to hear from Lucas before landing this. Thanks! Honza ::: devtools/client/netmonitor/src/utils/request-utils.js @@ +436,5 @@ > + protocol[0].toLowerCase() !== "http/2.0") { > + protocol.push(h.value); > + return true; > + } > + } Could we have a comment explaining the logic here? Also, shouldn't the condition be rather something like as follows? if (h.value.toLowerCase() !== "h2" && h.value.toLowerCase() !== "http/2.0") {
Reporter | ||
Comment 16•5 years ago
|
||
Hi :Honza
Apologies for the delay.
Going back to comment 7 (https://bugzilla.mozilla.org/show_bug.cgi?id=1501357#c7)
I think this is a generally correct approach. My one concern is how future proof this is to HTTP/3 i.e. if you have the HTTP version set to HTTP/3.0 and x-firefox-spdy is set to h3, would the code (as written in current patch) cause dev tools to show "HTTP/3.0 + h3)?
Assignee | ||
Comment 18•5 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] (need-info? me) from comment #15)
Thanks for the patch and sorry for the delay!
No worries about the delay. Thanks for the feedback!
Could we have a comment explaining the logic here?
I added a comment explaining my logic. Let me know if that's not what you had in mind.
Also, shouldn't the condition be rather something like as follows?
if (h.value.toLowerCase() !== "h2" &&
h.value.toLowerCase() !== "http/2.0") {
I'm not sure what you mean. If h.value.toLowerCase() and protocol[0].toLowerCase() are "http/1.1", we don't add anything to the protocol. My if statement's condition checks for values that fall outside of that case.
(In reply to Lucas Pardue from comment #16)
Hi :Honza
Apologies for the delay.
Going back to comment 7 (https://bugzilla.mozilla.org/show_bug.cgi?id=1501357#c7)
I think this is a generally correct approach. My one concern is how future proof this is to HTTP/3 i.e. if you have the HTTP version set to HTTP/3.0 and x-firefox-spdy is set to h3, would the code (as written in current patch) cause dev tools to show "HTTP/3.0 + h3)?
(In reply to Jan Honza Odvarko [:Honza] (need-info? me) from comment #17)
@Adam, please look at comment #16
Honza
I made some changes to account for future proofing it. How exhaustive should the x-firefox-spdy value checking be? For example, right now, an HTTP version of HTTP/3.0 and x-firefox-spdy of h3.0 will show "HTTP/3.0", as opposed to "HTTP/3.0+h3.0".
Comment 19•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c3670f327917d262ad608cc86f15201b938d6df
Comment 20•5 years ago
|
||
Thanks for the update, looks good now.
Try sever reports some Eslint errors, it's all related to the test file:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcb04d74ebb01615076ba39f25b7e6059dde4b2b&selectedJob=222664405
As soon as this is fixed, I think we can land it.
Thanks!
Honza
Assignee | ||
Comment 21•5 years ago
|
||
(In reply to Jan Honza Odvarko [:Honza] (need-info? me) from comment #20)
Thanks for the update, looks good now.
Try sever reports some Eslint errors, it's all related to the test file:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcb04d74ebb01615076ba39f25b7e6059dde4b2b&selectedJob=222664405As soon as this is fixed, I think we can land it.
Thanks!
Honza
Here's a patch with the Eslint errors fixed.
Comment 22•5 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=e49bebd77d95d0096bd92f583e8a73c3560185b9
Comment 23•5 years ago
|
||
Comment on attachment 9037599 [details] [diff] [review] bug1501357.patch Review of attachment 9037599 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the update! Try is green now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8cf333a9f17b72933f23db34b0d3075d09661398 Honza
Updated•5 years ago
|
Comment 24•5 years ago
|
||
Pushed by ncsoregi@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a65e39af3ad6
Fixing display of DevTools network tab protocol column values. r=Honza
Comment 25•5 years ago
|
||
bugherder |
Updated•5 years ago
|
Description
•