Firefox ignores http/2 push-ed resources
Categories
(Core :: Networking: HTTP, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr68 | --- | unaffected |
firefox75 | --- | wontfix |
firefox76 | --- | wontfix |
firefox77 | --- | wontfix |
firefox78 | --- | fixed |
People
(Reporter: zigazou, Assigned: mattwoodrow)
References
(Regression)
Details
(Keywords: regression, Whiteboard: [necko-triaged])
Attachments
(3 files)
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:76.0) Gecko/20100101 Firefox/76.0
Steps to reproduce:
- Open a private window
- Open the devtools and go to the network tab
- Visit https://http2.akamai.com/demo/index_sp.html?num=30
- look for requests without any return code (this indicates the response comes from a push request)
Actual results:
There is no such request.
Expected results:
There should have been 30 requests with no return code
Notes :
− this works on Chrome 83.0.4100.3
− this works on Firefox 64.0.2
− this doesn't work on Firefox 74.0.1
For info :
network.http.spdy.allow-push true
network.http.spdy.bug1556491 true
network.http.spdy.bug1563538 true
network.http.spdy.bug1563695 true
network.http.spdy.chunk-size 16000
network.http.spdy.coalesce-hostnames true
network.http.spdy.default-concurrent 100
network.http.spdy.default-hpack-buffer 65536
network.http.spdy.enable-hpack-dump false
network.http.spdy.enabled true
network.http.spdy.enabled.deps true
network.http.spdy.enabled.http2 true
network.http.spdy.enforce-tls-profile true
network.http.spdy.persistent-settings false
network.http.spdy.ping-threshold 58
network.http.spdy.ping-timeout 8
network.http.spdy.pull-allowance 12582912
network.http.spdy.push-allowance 131072
network.http.spdy.send-buffer-size 131072
network.http.spdy.timeout 170
network.http.spdy.websockets true
Reporter | ||
Comment 1•5 years ago
|
||
It also works as expected on Firefox 68.5.0esr.
Comment 2•5 years ago
|
||
Hello,
I was able to reproduce this bug using the latest Nightly 77.0a1 (2020-04-09), beta 76.0b2 and release 75.0.
I will add a component in order to get the dev team involved.
If you think it is not the right one please feel free to change to a more appropriate one.
Comment 4•5 years ago
|
||
Andrei, can you run mozregression?
Comment 5•5 years ago
|
||
I have performed a mozregression on this bug and the pushlog can be found here : https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f748a3d2cdf108e9443fd15332efe477c7c398a9&tochange=8b0d130f24d6f495afcd79532f3c789c5f3c41c3 .
Comment 6•5 years ago
|
||
(In reply to Andrei Purice from comment #5)
I have performed a mozregression on this bug and the pushlog can be found here : https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=f748a3d2cdf108e9443fd15332efe477c7c398a9&tochange=8b0d130f24d6f495afcd79532f3c789c5f3c41c3 .
from the pushlog, this is porbably caused by some of your patches?
Updated•5 years ago
|
Updated•5 years ago
|
Updated•5 years ago
|
Assignee | ||
Comment 7•5 years ago
|
||
Dragana, do you know why this isn't caught by tests?
Do we have tests for http/2 push, and do they run through docshell (to use document channel)?
How hard would it be to add new tests to ensure this doesn't break again?
I think I see why it's broken, should be an easy fix.
Assignee | ||
Updated•5 years ago
|
Assignee | ||
Comment 8•5 years ago
|
||
I think the issue here is that HttpChannelChild pulls the RequestContextID from the load group, and sends it to the parent when we AsyncOpen, but DocumentChannelChild doesn't.
Adding that should be fairly simple, but I have a bunch of other related concerns.
One is when we initiate content process loads from the parent (DocumentLoadListener::OpenFromParent). In that case we're in the parent process, so we don't have the load group when we try to create the http channel.
Another is for process switching. If we start a load in one process, and then switch it into another process (especially common with fission, but already happens when navigating from any document loaded in the parent to a normal web URI). In that case we'd have the request context id from the load group in the old process attached to the channel, but the document load happens in a different process/load-group.
Unassigning for now, and waiting for the ni? response, since this seems like a larger network issue than just this bug.
Comment 9•5 years ago
|
||
(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
Dragana, do you know why this isn't caught by tests?
Do we have tests for http/2 push, and do they run through docshell (to use document channel)?
We have, but they are all xpcshell test (you can find them here). It is possible to make test that use docshell.
How hard would it be to add new tests to ensure this doesn't break again?
It will be doable and hard. There are just some small thinks to take care:
we need to make sure that moz-http2 server is running (currently it is running only for xpcshell test here)
also moz-http2 server cannot handle multiple requests in parallel they need to be serialized or you will need multiple instances of the server.
I think I see why it's broken, should be an easy fix.
Comment 10•5 years ago
|
||
Hi Dragana; should this one still be marked as a P1?
Comment 11•5 years ago
|
||
(In reply to Rachel Tublitz [:rachel] from comment #10)
Hi Dragana; should this one still be marked as a P1?
This is a regression and I want it to be fixed.
Matt, when will you be able to fix this?
Assignee | ||
Comment 12•4 years ago
|
||
I think we need to discuss this further to figure out how to solve this.
As I said, we can easily transfer the request context id in DocumentChannelChild and set it on the nsHttpChannel in the parent, which would fix this in some cases.
It won't fix all cases though (including loads started from the parent process), and will still be totally broken with fission.
I think having request context ids be strongly owned by a load group isn't going to work with fission, since document loads can start in one process and then be switched to another. The new process will have a new docshell/loadgroup (and thus new request context id), and so any subresource within that document won't work.
I suspect we're going to want to move ownership of the request context id to the browsing context (and possibly just re-use the browsing context id) so that we have something that's constant across process switches.
Can we find a time to discuss this more, and figure out how to get test coverage (especially with fission!).
Assignee | ||
Comment 13•4 years ago
|
||
Previously we created a new request context id for each LoadGroup, which meant we didn't have a constant value across process switches.
The next patch uses this request context id when creating a new LoadGroup for an nsDocShell, so that all docshell/loadgroups for a BrowsingContext share the same id.
Updated•4 years ago
|
Assignee | ||
Comment 14•4 years ago
|
||
Depends on D75884
Assignee | ||
Comment 15•4 years ago
|
||
Depends on D75885
Assignee | ||
Comment 16•4 years ago
|
||
Clearing ni? for now, since I just went ahead and had a go at the approach I suggested.
Comment 17•4 years ago
|
||
Our last beta before RC is Friday but this is marked as P1, Matt, is it safe to assume that there won't be a fix and a subsequent uplift request into our last beta or RC builds?
Comment 18•4 years ago
|
||
Comment 19•4 years ago
•
|
||
Backed out 3 changesets (Bug 1627533) for causing assertions in RequestContextService.cpp CLOSED TREE
Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=303185113&repo=autoland&lineNumber=1322
Another assertion: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=303184393&repo=autoland&lineNumber=20396
Backout: https://hg.mozilla.org/integration/autoland/rev/6d55c17a5a856ab243aa4fcaea11368b9890f60b
Assignee | ||
Comment 20•4 years ago
|
||
I'll see what I can do!
Assignee | ||
Updated•4 years ago
|
Comment 21•4 years ago
|
||
Comment 22•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/a2531f6f6505
https://hg.mozilla.org/mozilla-central/rev/80377f949b46
https://hg.mozilla.org/mozilla-central/rev/fb696e38fc5c
Updated•4 years ago
|
Updated•4 years ago
|
Description
•