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

Status: RESOLVED FIXED

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

Description

[Austin Donisan]

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.

Comment 1

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258

Comment 2

[u408661]

Attachment: Bug 1397621 - Ensure we use the proper default port for origin matching with h2 pushes.

Review commit: https://reviewboard.mozilla.org/r/181006/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181006/

Comment 3

[Mozilla Autoland]

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

Comment 4

[u408661]

Stupid try... pushing manually worked: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e2385a0e5d895bc1ee166c795ee374768bd25b1b

Comment 5

[Patrick McManus [:mcmanus]]

thanks nick.. can you see if this can get into 56?

Comment 6

[Patrick McManus [:mcmanus]]

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

Comment 7

[Pulsebot]

Pushed by [e-mail]:
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

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8a27ba2f85b4

Comment 9

[u408661]

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

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 11

[Austin Donisan]

I'm not sure how much the "tracking flags" matter but this does affect version 55.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Did you test this with 56 and 55? How do we know when this regressed?

Comment 13

[Austin Donisan]

Yes, with 54, 55, and 56. I linked to what caused this in my initial report.

Comment 14

[u408661]

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

Comment 15

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/6afc59b029f2