1825916 - Netmonitor shows the wrong protocol for HTTP/2 and HTTP/3 responses

Status: RESOLVED FIXED

(DevTools :: Netmonitor, defect, P1)

Description

[mix5003]

Attachment: ff.png

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:109.0) Gecko/20100101 Firefox/113.0

Steps to reproduce:

1. open firefox nightly
2. press f12 to open dev tools and go to networks tab
3. go to https://www.youtube.com/ or may be https://bugzilla.mozilla.org
4. right click at network table header to show protocol
5. you should see at least 1 http/2 in networks request

Actual results:

all request protocol is http/1.1

Expected results:

some or all network request should be http/2

Comment 1

[mix5003]

2023-04-01T13:49:31.676000: DEBUG : Found commit message:
Bug 1815460 - [devtools] Merge addRequestPostData, addResponseHeaders, addResponseCookies into addResponseStart r=webdriver-reviewers,ochameau,bomsy,devtools-reviewers,whimboo

Differential Revision: https://phabricator.services.mozilla.com/D168538

2023-04-01T13:49:31.676000: DEBUG : Did not find a branch, checking all integration branches
2023-04-01T13:49:31.677000: INFO : The bisection is done.
2023-04-01T13:49:31.678000: INFO : Stopped

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'DevTools::Netmonitor' component, and is moving the bug to that component. Please correct in case you think the bot is wrong.

Comment 3

[mix5003]

addition information when i click any request to see detail it change from http/1.1 to http/1.1+h2

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

:jdescottes, since you are the author of the regressor, bug 1815460, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Comment 5

[Julian Descottes [:jdescottes]]

Thanks, I can reproduce

Comment 6

[mix5003]

at first i think this is not actually used http2 but if this bug in dev tool only, i think it should change the title.

sorry for inconvenient

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1815460

Comment 8

[Julian Descottes [:jdescottes]]

We used to read the httpVersion mostly from the first line of the raw response headers. With the regressing patch we started using channel APIs instead, but we are reading the version from the request instead of the response. That being said I am not 100% sure this can fully replace what reading the header's first line. I will need to do more tests.

It appears that this area is fully untested, so it might be safer to just keep using the raw response headers for now, and handle cleanups in a follow up.

(In reply to mix5003 from comment #6)

at first i think this is not actually used http2 but if this bug in dev tool only, i think it should change the title.

sorry for inconvenient

No worries at all and thanks again for filing, the bug was appropriately moved to the DevTools component already, so it's all good. I will update the summary for clarity.

Comment 9

[Julian Descottes [:jdescottes]]

Hi kershaw!

I was looking at testing HTTP/3 with browser mochitests, and I see you just added support for that with Bug 1809843.
I would be interested in adding at least one devtools netmonitor mochitest running with a http3 server to check that we retrieve the expected protocol.

Did you plan to add a variant for the devtools mochitest which would run with http3? I imagine a lot of the netmonitor mochitests will fail with such a change, and some will require dedicated asserts that will only be valid for http3. My idea is to add a new netmonitor test suite that would only run if http3, and which would probably contain a single test for the bug I am fixing here. Does that sound fine or would it conflict with some other work in this area?

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1825916 - [devtools] Update getHttpVersion to read the response version

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 1825916 - [devtools] Add a http3 only test for netmonitor response headers

Depends on D174526

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: Bug 1825916 - [devtools] Always read response protocol to fetch http version

Comment 13

[Julian Descottes [:jdescottes]]

(In reply to Julian Descottes [:jdescottes] from comment #9)

Hi kershaw!

I was looking at testing HTTP/3 with browser mochitests, and I see you just added support for that with Bug 1809843.
I would be interested in adding at least one devtools netmonitor mochitest running with a http3 server to check that we retrieve the expected protocol.

Did you plan to add a variant for the devtools mochitest which would run with http3? I imagine a lot of the netmonitor mochitests will fail with such a change, and some will require dedicated asserts that will only be valid for http3. My idea is to add a new netmonitor test suite that would only run if http3, and which would probably contain a single test for the bug I am fixing here. Does that sound fine or would it conflict with some other work in this area?

For info, I tried enabling http3 for devtools mochitests and it seems to trigger a lot of failures right now: https://treeherder.mozilla.org/jobs?repo=try&revision=47f7c309d7ff8b411a5559e2213b6cf00a5972ea
Will probably require some dedicated effort to either update the tests or disable the ones which really can't work seamlessly between the normal variant and the http3 variant.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7d2495ea5489
[devtools] Update getHttpVersion to read the response version r=bomsy,devtools-reviewers
https://hg.mozilla.org/integration/autoland/rev/00fa9fd2d3ad
[devtools] Add a http3 only test for netmonitor response headers r=bomsy,devtools-reviewers

Comment 15

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/7d2495ea5489
https://hg.mozilla.org/mozilla-central/rev/00fa9fd2d3ad

Comment 16

[Phabricator Automation]

Comment on attachment 9326726 [details]
Bug 1825916 - [devtools] Always read response protocol to fetch http version

Revision D174531 was moved to bug 1826954. Setting attachment 9326726 [details] to obsolete.

Comment 17

[Kershaw Chang [:kershaw]]

(In reply to Julian Descottes [:jdescottes] from comment #9)

Hi kershaw!

I was looking at testing HTTP/3 with browser mochitests, and I see you just added support for that with Bug 1809843.
I would be interested in adding at least one devtools netmonitor mochitest running with a http3 server to check that we retrieve the expected protocol.

Did you plan to add a variant for the devtools mochitest which would run with http3? I imagine a lot of the netmonitor mochitests will fail with such a change, and some will require dedicated asserts that will only be valid for http3. My idea is to add a new netmonitor test suite that would only run if http3, and which would probably contain a single test for the bug I am fixing here. Does that sound fine or would it conflict with some other work in this area?

Yes, I plan to enable Http/3 server for more tests, but investigating and fixing failed tests take too much time for now.
Currently, we have more than 600 mochitest-plain tests skipped and we are still trying to come up a better way to tackle these tests.
Feel free to add another test suite that uses Http/3, since I don't have time to do this in the near feature. I'd be happy to provide any assistance if you encounter any problem. Thanks.

Comment 18

[Julian Descottes [:jdescottes]]

(In reply to Kershaw Chang [:kershaw] from comment #17)

(In reply to Julian Descottes [:jdescottes] from comment #9)

Hi kershaw!

I was looking at testing HTTP/3 with browser mochitests, and I see you just added support for that with Bug 1809843.
I would be interested in adding at least one devtools netmonitor mochitest running with a http3 server to check that we retrieve the expected protocol.

Did you plan to add a variant for the devtools mochitest which would run with http3? I imagine a lot of the netmonitor mochitests will fail with such a change, and some will require dedicated asserts that will only be valid for http3. My idea is to add a new netmonitor test suite that would only run if http3, and which would probably contain a single test for the bug I am fixing here. Does that sound fine or would it conflict with some other work in this area?

Yes, I plan to enable Http/3 server for more tests, but investigating and fixing failed tests take too much time for now.
Currently, we have more than 600 mochitest-plain tests skipped and we are still trying to come up a better way to tackle these tests.
Feel free to add another test suite that uses Http/3, since I don't have time to do this in the near feature. I'd be happy to provide any assistance if you encounter any problem. Thanks.

Great, I'll try to enable devtools in Bug 1826954 (with a lot of disabling at first :) )

Comment 19

[Timea Zsoldos [:tzsoldos], Desktop QA]

I have reproduced this issue using Firefox 113.0a1 (2023.04.01) on Win 10.
I can confirm this issue is fixed, I verified using Firefox 113.0 on Win 10, macOS 12 and Ubuntu 22, now most of the responses are HTTP/3.