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.

👁 Image

Patrick McManus [:mcmanus]

Reporter

mozreview-request

Comment hidden (mozreview-request)

👁 Image

Patrick McManus [:mcmanus]

Reporter

mozreview-request

Comment hidden (mozreview-request)

👁 Image	Patrick McManus [:mcmanus] Reporter
	Comment 3 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=58cfe727df6775fabceac48eac1957c89ab46f86

👁 Image	u408661
	Comment 4 • 9 years ago
mozreview-review

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+

👁 Image	Mozilla Autoland
	Comment 5 • 9 years 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)

👁 Image	Patrick McManus [:mcmanus] Reporter
	Comment 6 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/22c341bd832a8686fffa0a7bb6dfc3aba086ffe1 Bug 1353497 - h2 pushes against origin set need implicit SNI r=nwgh

👁 Image	Carsten Book [:Tomcat]
	Comment 7 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/22c341bd832a

Status: NEW → RESOLVED

Closed: 9 years ago

status-firefox55: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla55

👁 Image	Chris H-C :chutten
	Comment 8 • 9 years 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

👁 Image

Chris H-C :chutten

Updated

•

9 years ago

Flags: needinfo?(mcmanus)

👁 Image	Patrick McManus [:mcmanus] Reporter
	Comment 9 • 9 years 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?

👁 Image

Patrick McManus [:mcmanus]

Reporter

Updated

•

9 years ago

Flags: needinfo?(mcmanus)

👁 Image	Chris H-C :chutten
	Comment 10 • 9 years ago

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

👁 Image	Patrick McManus [:mcmanus] Reporter
	Comment 11 • 9 years 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 :)

👁 Image

Patrick McManus [:mcmanus]

Reporter