Closed Bug 1397621 Opened 2 years ago Closed 2 years ago

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

Categories

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

56 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- wontfix
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: austin.donisan, Assigned: u408661)

References

Details

(Keywords: regression, Whiteboard: [spdy])

Attachments

(1 file)

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.
Component: Untriaged → Networking: HTTP
Product: Firefox → Core
Assignee: nobody → hurley
Whiteboard: [necko-next][spdy]
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P2
Priority: P2 → P1
Whiteboard: [necko-next][spdy] → [spdy]
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 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+
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
https://hg.mozilla.org/mozilla-central/rev/8a27ba2f85b4
Status: UNCONFIRMED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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?
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?
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?
Flags: needinfo?(hurley)
Yes, with 54, 55, and 56. I linked to what caused this in my initial report.
(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)
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+
Depends on: 1409570
You need to log in before you can comment on or make changes to this bug.