1627533 - Firefox ignores http/2 push-ed resources

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Frédéric BISSON]

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:76.0) Gecko/20100101 Firefox/76.0

Steps to reproduce:

1. Open a private window
2. Open the devtools and go to the network tab
3. Visit https://http2.akamai.com/demo/index_sp.html?num=30
4. 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

Comment 1

[Frédéric BISSON]

It also works as expected on Firefox 68.5.0esr.

Comment 2

[Andrei Purice]

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 3

[Andy Grover [:grover]]

Dragana, what do you think?

Comment 4

[Dragana Damjanovic [:dragana]]

Andrei, can you run mozregression?

Comment 5

[Andrei Purice]

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

[Dragana Damjanovic [:dragana]]

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

Comment 7

[Matt Woodrow (:mattwoodrow)]

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.

Comment 8

[Matt Woodrow (:mattwoodrow)]

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

[Dragana Damjanovic [:dragana]]

(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

[Rachel Tublitz [:rachel]]

Hi Dragana; should this one still be marked as a P1?

Comment 11

[Dragana Damjanovic [:dragana]]

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

Comment 12

[Matt Woodrow (:mattwoodrow)]

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!).

Comment 13

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1627533 - Create a request context id for each BrowsingContext. r?nika!,dragana!

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.

Comment 14

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1627533 - Create LoadGroups for docshells using the BrowsingContext's request context id. r?dragana

Depends on D75884

Comment 15

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1627533 - Have DocumentLoadListener retrieve the request context id from the BrowsingContext and use it for http loads. r?dragana

Depends on D75885

Comment 16

[Matt Woodrow (:mattwoodrow)]

Clearing ni? for now, since I just went ahead and had a go at the approach I suggested.

Comment 17

[Pascal Chevrel:pascalc]

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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0d1046670534
Create a request context id for each BrowsingContext. r=nika,dragana,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/70fa5e075269
Create LoadGroups for docshells using the BrowsingContext's request context id. r=dragana,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/3b35a1852a60
Have DocumentLoadListener retrieve the request context id from the BrowsingContext and use it for http loads. r=dragana,necko-reviewers

Comment 19

[Noemi Erli[:noemi_erli]]

Backed out 3 changesets (Bug 1627533) for causing assertions in RequestContextService.cpp CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&selectedTaskRun=Ch6FeZX9QiCD8eiXfpxXpA-0&resultStatus=testfailed%2Cbusted%2Cexception&revision=3b35a1852a6011c8094e2d3d538db7093ab6f86c

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

Comment 20

[Matt Woodrow (:mattwoodrow)]

I'll see what I can do!

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a2531f6f6505
Create a request context id for each BrowsingContext. r=nika,dragana,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/80377f949b46
Create LoadGroups for docshells using the BrowsingContext's request context id. r=dragana,necko-reviewers
https://hg.mozilla.org/integration/autoland/rev/fb696e38fc5c
Have DocumentLoadListener retrieve the request context id from the BrowsingContext and use it for http loads. r=dragana,necko-reviewers

Comment 22

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/a2531f6f6505
https://hg.mozilla.org/mozilla-central/rev/80377f949b46
https://hg.mozilla.org/mozilla-central/rev/fb696e38fc5c