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

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P2)

Description

[Lucas Pardue]

Attachment: 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.

Comment 1

[Timea Cernea [:tbabos][inactive]]

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

[Valentin Gosu [:valentin] (he/him)]

Looks like a problem with the devtools UI.

Comment 3

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

Thanks for the report! I can also reproduce the issue on my machine.

Honza

Comment 4

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

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

Comment 5

[Adam Holm]

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

Comment 6

[u408661]

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

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

(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

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

(In reply to Adam Holm from comment #5)
> I'd be interested in working on this.
Sure, assigned to you, thanks!

Honza

Comment 9

[u408661]

(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

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

@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

Comment 11

[Adam Holm]

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

Comment 12

[Adam Holm]

(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

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

(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

Comment 14

[Adam Holm]

Attachment: bug1501357.patch

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

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

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") {

Comment 16

[Lucas Pardue]

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

Comment 17

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

@Adam, please look at comment #16

Honza

Comment 18

[Adam Holm]

Attachment: bug1501357.patch

(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

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c3670f327917d262ad608cc86f15201b938d6df

Comment 20

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

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

Comment 21

[Adam Holm]

Attachment: bug1501357.patch

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

Comment 22

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=e49bebd77d95d0096bd92f583e8a73c3560185b9

Comment 23

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

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

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a65e39af3ad6
Fixing display of DevTools network tab protocol column values. r=Honza

Comment 25

[Cristina Coroiu [:ccoroiu]]

https://hg.mozilla.org/mozilla-central/rev/a65e39af3ad6