Closed Bug 1627533 Opened 4 months ago Closed 3 months ago

Firefox ignores http/2 push-ed resources

Categories

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

76 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla78
Tracking Status
firefox-esr68 --- unaffected
firefox75 --- wontfix
firefox76 --- wontfix
firefox77 --- wontfix
firefox78 --- fixed

People

(Reporter: zigazou, Assigned: mattwoodrow)

References

(Depends on 1 open bug, 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:

  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

It also works as expected on Firefox 68.5.0esr.

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.

Status: UNCONFIRMED → NEW
Component: Untriaged → Networking: HTTP
Ever confirmed: true
Product: Firefox → Core

Dragana, what do you think?

Flags: needinfo?(dd.mozilla)

Andrei, can you run mozregression?

Flags: needinfo?(dd.mozilla) → needinfo?(andrei.purice)
Flags: needinfo?(andrei.purice)

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

Flags: needinfo?(matt.woodrow)
Keywords: regression
Priority: -- → P1
Whiteboard: [necko-triaged]

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.

Flags: needinfo?(matt.woodrow) → needinfo?(dd.mozilla)
Assignee: nobody → matt.woodrow

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.

Assignee: matt.woodrow → nobody

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

Flags: needinfo?(dd.mozilla)

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

Flags: needinfo?(dd.mozilla)

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

Flags: needinfo?(dd.mozilla) → needinfo?(matt.woodrow)

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

Flags: needinfo?(matt.woodrow) → needinfo?(dd.mozilla)

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.

Assignee: nobody → matt.woodrow
Status: NEW → ASSIGNED

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

Flags: needinfo?(dd.mozilla)

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?

Flags: needinfo?(matt.woodrow)
Pushed by mwoodrow@mozilla.com:
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

I'll see what I can do!

Flags: needinfo?(matt.woodrow)
Pushed by mwoodrow@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla78
Depends on: 1641167
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.