All HTTP2 server pushes are being ignored due to faulty origin check

RESOLVED FIXED in Firefox 56

Status

()

Core
Networking: HTTP
P1
normal
RESOLVED FIXED
10 months ago
8 months ago

People

(Reporter: Austin Donisan, Assigned: nwgh)

Tracking

(Depends on: 1 bug, {regression})

56 Branch
mozilla57
regression
Points:
---

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 wontfix, firefox56 fixed, firefox57 fixed)

Details

(Whiteboard: [spdy])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

10 months ago
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:56.0) Gecko/20100101 Firefox/56.0
Build ID: 20170903140023

Steps to reproduce:

Visit a page using HTTP2 server push. For example https://http2.golang.org/serverpush is pretty simple.

Check the network tab and see that the resources aren't being successfully pushed (they are in Chrome and Edge).


Actual results:

The pushes are being ignored due to a bug in the origin check logic. The logging from about:networking#logging has snippets like this:

2017-09-07 05:48:37.832000 UTC - [Socket Thread]: I/nsHttp Http2Session::RecvPushPromise 0000013ADA262000 origin check https://http2.golang.org
2017-09-07 05:48:37.832000 UTC - [Socket Thread]: I/nsHttp JoinConnection 0000013ADA262000 no origin frame check used.
2017-09-07 05:48:37.832000 UTC - [Socket Thread]: V/nsHttp joinconnection [0000013ADA262000 .S.....http2.golang.org:443] http2.golang.org:k-1 result=0 cache
2017-09-07 05:48:37.832000 UTC - [Socket Thread]: I/nsHttp Http2Session::RecvPushPromise 0000013ADA262000 pushed stream mismatched origin https://http2.golang.org
2017-09-07 05:48:37.832000 UTC - [Socket Thread]: I/nsHttp Http2Session::CleanupStream 0000013ADA262000 0000013AD62F5260 0x4 80004005

It looks like it thinks that the push is for a resource on port -1.


Expected results:

HTTP2 push should work. It works in Firefox 54 and doesn't in 55.

I think bug 1337791 broke this.

Updated

10 months ago
status-firefox55: --- → unaffected
status-firefox56: --- → ?
status-firefox57: --- → ?
Component: Untriaged → Networking: HTTP
Product: Firefox → Core
(Assignee)

Updated

10 months ago
Assignee: nobody → hurley
Whiteboard: [necko-next][spdy]
(Assignee)

Updated

9 months ago
Priority: P2 → P1
(Assignee)

Updated

9 months ago
Whiteboard: [necko-next][spdy] → [spdy]
Comment hidden (mozreview-request)

Comment 3

9 months ago
hg error in cmd: hg push -r . -f try: pushing to ssh://hg.mozilla.org/try
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 0 changes to 0 files (+1 heads)
remote: 
remote: 
remote: ************************** ERROR ****************************
remote: Error accessing https://treestatus.mozilla-releng.net/trees/try :
remote: HTTP Error 503: Service Unavailable
remote: Unable to check if the tree is open - treating as if CLOSED.
remote: To push regardless, include "CLOSED TREE" in your push comment.
remote: *************************************************************
remote: 
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.a_treeclosure hook failed
abort: push failed on remote
thanks nick.. can you see if this can get into 56?

Comment 6

9 months ago
mozreview-review
Comment on attachment 8909539 [details]
Bug 1397621 - Ensure we use the proper default port for origin matching with h2 pushes.

https://reviewboard.mozilla.org/r/181006/#review186618
Attachment #8909539 - Flags: review?(mcmanus) → review+

Comment 7

9 months ago
Pushed by hurley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8a27ba2f85b4
Ensure we use the proper default port for origin matching with h2 pushes. r=mcmanus

Comment 8

9 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/8a27ba2f85b4
Status: UNCONFIRMED → RESOLVED
Last Resolved: 9 months ago
status-firefox57: ? → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Assignee)

Comment 9

9 months ago
Comment on attachment 8909539 [details]
Bug 1397621 - Ensure we use the proper default port for origin matching with h2 pushes.

Approval Request Comment
[Feature/Bug causing the regression]: ORIGIN frame
[User impact if declined]: No http/2 pushes accepted, potentially slower site loads
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: small fix to ensure an appropriate default value is used
[String changes made/needed]: none
Attachment #8909539 - Flags: approval-mozilla-beta?
status-firefox56: ? → affected
Comment on attachment 8909539 [details]
Bug 1397621 - Ensure we use the proper default port for origin matching with h2 pushes.

56 is on mozilla-release now.
Attachment #8909539 - Flags: approval-mozilla-beta? → approval-mozilla-release?
(Reporter)

Comment 11

9 months ago
I'm not sure how much the "tracking flags" matter but this does affect version 55.
Did you test this with 56 and 55? How do we know when this regressed?
status-firefox55: unaffected → ?
Flags: needinfo?(hurley)
(Reporter)

Comment 13

9 months ago
Yes, with 54, 55, and 56. I linked to what caused this in my initial report.
(Assignee)

Comment 14

9 months ago
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #12)
> Did you test this with 56 and 55? How do we know when this regressed?

What the reporter said in comment 13. The offending code was introduced in 55, so this affects 55, 56, and 57.
Flags: needinfo?(hurley)
(Assignee)

Updated

9 months ago
status-firefox55: ? → affected
Comment on attachment 8909539 [details]
Bug 1397621 - Ensure we use the proper default port for origin matching with h2 pushes.

Should improve perf, verified in nightly, let's uplift for the 56 RC2 build.
Attachment #8909539 - Flags: approval-mozilla-release? → approval-mozilla-release+
status-firefox55: affected → wontfix
status-firefox-esr52: --- → unaffected
Keywords: regression

Comment 16

9 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-release/rev/6afc59b029f2
status-firefox56: affected → fixed

Updated

8 months ago
Depends on: 1409570
You need to log in before you can comment on or make changes to this bug.