Closed Bug 1501357 Opened 11 months ago Closed 8 months ago

Network tab protocol column shows "HTTP/2.0+h2" when individual requests are selected

Categories

(DevTools :: Netmonitor, defect, P2)

62 Branch
defect

Tracking

(firefox63 wontfix, firefox64 wontfix, firefox65 wontfix, firefox66 fixed)

RESOLVED FIXED
Firefox 66
Tracking Status
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)

Attached image mozh2.png
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.
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!
Status: UNCONFIRMED → NEW
Component: Untriaged → Networking: HTTP
Ever confirmed: true
Product: Firefox → Core
Looks like a problem with the devtools UI.
Component: Networking: HTTP → Netmonitor
Product: Core → DevTools
Thanks for the report! I can also reproduce the issue on my machine.

Honza
Mentor: odvarko
Keywords: good-first-bug
Priority: -- → P2
Whiteboard: good-first-bug
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.
(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
Flags: needinfo?(hurley)
(In reply to Adam Holm from comment #5)
> I'd be interested in working on this.
Sure, assigned to you, thanks!

Honza
Assignee: nobody → asorholm
Status: NEW → ASSIGNED
(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.
Flags: needinfo?(hurley)
@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
(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.
(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?
Flags: needinfo?(odvarko)
(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
Flags: needinfo?(odvarko)
Attached patch bug1501357.patch (obsolete) — Splinter Review
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.
Attachment #9029381 - Flags: feedback?(odvarko)
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") {
Attachment #9029381 - Flags: feedback?(odvarko) → feedback?(lucaspardue.24.7)

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)?

@Adam, please look at comment #16

Honza

Flags: needinfo?(asorholm)
Attached patch bug1501357.patch (obsolete) — Splinter Review

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

Attachment #9029381 - Attachment is obsolete: true
Attachment #9029381 - Flags: feedback?(lucaspardue.24.7)
Flags: needinfo?(asorholm) → needinfo?(odvarko)

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

Flags: needinfo?(odvarko) → needinfo?(asorholm)
Attached patch bug1501357.patchSplinter Review

(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=222664405

As soon as this is fixed, I think we can land it.

Thanks!
Honza

Here's a patch with the Eslint errors fixed.

Attachment #9037116 - Attachment is obsolete: true
Flags: needinfo?(asorholm) → needinfo?(odvarko)
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
Attachment #9037599 - Flags: review+
Flags: needinfo?(odvarko)
Keywords: checkin-needed

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

Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 8 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66
You need to log in before you can comment on or make changes to this bug.