h2 origin frame push with implicit sni

RESOLVED FIXED in Firefox 55

Status

()

Core
Networking: HTTP
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mcmanus, Unassigned)

Tracking

50 Branch
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

a year ago
I realized that the https://bugzilla.mozilla.org/show_bug.cgi?id=1337791 code for ORIGIN frame was a little wrong wrt push.

normally the SNI origin is implicit in the origin set, and the h2 layer doesn't worry about that because connectionMgr will always use the connection entry with the matching sni even if "coalescing" fails.

but using the testJoinConnection logic for push validation means that in the presence of an origin frame it will reject pushes for sni because they are not in the origin set.

easy fix - plus a push/originFrame test case for the various permuations.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment on attachment 8854544 [details]
Bug 1353497 - h2 pushes against origin set need implicit SNI

https://reviewboard.mozilla.org/r/126500/#review129030

Oof, I should've caught this on initial review.
Attachment #8854544 - Flags: review?(hurley) → review+

Comment 5

a year ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 3200490c8cb1 -d f3f83b9c428c: rebasing 386719:3200490c8cb1 "Bug 1353497 - h2 pushes against origin set need implicit SNI r=nwgh" (tip)
remote changed netwerk/test/unit/test_origin.js which local deleted
use (c)hanged version, leave (d)eleted, or leave (u)nresolved? u
merging netwerk/protocol/http/Http2Session.cpp
merging testing/xpcshell/moz-http2/moz-http2.js
warning: conflicts while merging netwerk/protocol/http/Http2Session.cpp! (edit, then use 'hg resolve --mark')
warning: conflicts while merging testing/xpcshell/moz-http2/moz-http2.js! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)

Comment 7

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/22c341bd832a
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 8

a year ago
We're seeing some changes to SPDY probes that might be caused by this bug: [1][2][3]
It looks like there's a sudden increase in the number of zeros being reported, as though the mechanism for recording these values (Http2Session::~Http2Session()) is being called 2-3x more often than before.

Are these good changes? Bad? 

Are they intentional? Expected?

Are these probes (still) measuring something useful?

[1]: http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/1559/alerts/?from=2017-04-06&to=2017-04-06
[2]: http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/59/alerts/?from=2017-04-06&to=2017-04-06
[3]: http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/319/alerts/?from=2017-04-06&to=2017-04-06

Updated

a year ago
Flags: needinfo?(mcmanus)
(Reporter)

Comment 9

a year ago
thanks for this 

(In reply to Chris H-C :chutten from comment #8)
> We're seeing some changes to SPDY probes that might be caused by this bug:
> [1][2][3]

are those two nightly builds being compared? It would be really much better if that page would provide branches and build csets - dates are much much fuzzier :)

bug 1337791 might be responsible. (not this one). it could be benign or not - I need to look harder at each one.

[1] so this shifts a huge number of goaway received error codes from PROTOCOL_ERROR into NO_ERROR. On its face, that's a good thing.

[2] this is kind of a meh shift - but perhaps a bad sign

[3] this one is the most striking but it might just be a side effect of more aggressive coalescing leaving some connections serving nothing - which would be expected

> It looks like there's a sudden increase in the number of zeros being
> reported, as though the mechanism for recording these values
> (Http2Session::~Http2Session()) is being called 2-3x more often than before.

the charts don't really show that (they just show the distribution).. source? it could be interesting.

> 
> Are these good changes? Bad? 
> 
> Are they intentional? Expected?
> 
> Are these probes (still) measuring something useful?

yes!

> 
> [1]:
> http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/1559/
> alerts/?from=2017-04-06&to=2017-04-06
> [2]:
> http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/59/
> alerts/?from=2017-04-06&to=2017-04-06
> [3]:
> http://alerts.telemetry.mozilla.org/index.html#/detectors/1/metrics/319/
> alerts/?from=2017-04-06&to=2017-04-06

maybe make a new bug?
(Reporter)

Updated

a year ago
Flags: needinfo?(mcmanus)
(In reply to Patrick McManus [:mcmanus] from comment #9)
> thanks for this 
> 
> (In reply to Chris H-C :chutten from comment #8)
> > We're seeing some changes to SPDY probes that might be caused by this bug:
> > [1][2][3]
> 
> are those two nightly builds being compared? It would be really much better
> if that page would provide branches and build csets - dates are much much
> fuzzier :)

Sorry, yes, Nightly builds. Here's the changelog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b5d8b27a753725c1de41ffae2e338798f3b5cacd&tochange=3c68d659c2b715f811708f043a1e7169d77be2ba
 
> > It looks like there's a sudden increase in the number of zeros being
> > reported, as though the mechanism for recording these values
> > (Http2Session::~Http2Session()) is being called 2-3x more often than before.
> 
> the charts don't really show that (they just show the distribution)..
> source? it could be interesting.

Sure: 
Before (7.7M samples) https://mzl.la/2opw0RG
After (17.4M samples) https://mzl.la/2oprin1

> > Are these probes (still) measuring something useful?
> 
> yes!

Oh good. They don't have alert_emails fields set, though. Shall I file a bug for that?
(Reporter)

Comment 11

a year ago
thanks, I'll make the bug for investigating this. I'm not surprised the submissions shifted a bit - but that's worth making sure its well understood.

sure, you can file a bug for alert_emails.. or just add necko@mozilla.com if you like :)
(Reporter)

Updated

a year ago
Blocks: 1355591
(Reporter)

Updated

a year ago
No longer blocks: 1355591
Depends on: 1355591
(Reporter)

Updated

a year ago
No longer depends on: 1355591
You need to log in before you can comment on or make changes to this bug.