http/2 alt-svc support

RESOLVED FIXED in mozilla35

Status

()

Core
Networking: HTTP
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: nwgh, Assigned: mcmanus)

Tracking

({dev-doc-needed})

unspecified
mozilla35
dev-doc-needed
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [spdy])

Attachments

(2 attachments, 8 obsolete attachments)

12.12 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
128.53 KB, patch
mcmanus
: review+
Details | Diff | Splinter Review
Since we're kind of responsible for it, we should really implement alt-svc for http/2 :) Patrick - assigning to you, since you've got a branch somewhere that at least has a POC for this.
(Assignee)

Updated

3 years ago
Duplicate of this bug: 981660
(Assignee)

Comment 2

3 years ago
https://httpwg.github.io/http-extensions/alt-svc.html
https://tools.ietf.org/html/draft-ietf-httpbis-http2-encryption-00
(Assignee)

Comment 3

3 years ago
Created attachment 8488228 [details] [diff] [review]
HTTP/2 Alternate Service and Opportunistic Security [1/2 PSM]
(Assignee)

Comment 4

3 years ago
Created attachment 8488229 [details] [diff] [review]
HTTP/2 Alternate Service and Opportunistic Security [2/2 necko]
(Assignee)

Comment 5

3 years ago
Comment on attachment 8488228 [details] [diff] [review]
HTTP/2 Alternate Service and Opportunistic Security [1/2 PSM]

Review of attachment 8488228 [details] [diff] [review]:
-----------------------------------------------------------------

David, these bits are pretty minimal given the scope of the overall necko patch. It basically boils down to
1] an option to ignore authentication
2] a way to report whether or not authentication failed outside of psm without doing i/o
3] the option to provide a different connection host/port than the authentication host/port without using proxy semantics. (proxies already do this)
Attachment #8488228 - Flags: review?(dkeeler)
(Assignee)

Comment 6

3 years ago
Comment on attachment 8488229 [details] [diff] [review]
HTTP/2 Alternate Service and Opportunistic Security [2/2 necko]

Review of attachment 8488229 [details] [diff] [review]:
-----------------------------------------------------------------

this is kind of a mushy long lived branch, so I'm sure it will require an iteration of cleanup - but let me know what you think. At least you've seen this before.
Attachment #8488229 - Flags: review?(hurley)
(Assignee)

Comment 7

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=56b7fc160306
Comment on attachment 8488228 [details] [diff] [review]
HTTP/2 Alternate Service and Opportunistic Security [1/2 PSM]

Review of attachment 8488228 [details] [diff] [review]:
-----------------------------------------------------------------

I wasn't able to get to this this week - sorry. I'll review it early next week.
Comment on attachment 8488228 [details] [diff] [review]
HTTP/2 Alternate Service and Opportunistic Security [1/2 PSM]

Review of attachment 8488228 [details] [diff] [review]:
-----------------------------------------------------------------

I don't have any problems with this patch as implemented, but I'd like to know what effect this has on the UI (see first comment). So, I guess I'll just cancel the review for now as an indication that I'd like a bit more information. Thanks!

::: netwerk/socket/nsISSLSocketControl.idl
@@ +120,5 @@
> +    /**
> +     * set bypassAuthentication to true if the server certificate checks should
> +     * not be enforced. This is to enable non-secure transport over TLS.
> +     */
> +    [infallible] attribute boolean bypassAuthentication;

What effect does this have on the UI/mixed content/etc.? In other words, will a page transmitted over non-secure TLS have the lock icon? If there's non-secure TLS content in a page that's otherwise secure, will the mixed content blocker do the right thing?

::: security/manager/ssl/src/nsNSSIOLayer.cpp
@@ +229,5 @@
>    return NS_OK;
>  }
>  
>  NS_IMETHODIMP
> +nsNSSSocketInfo::GetBypassAuthentication(bool *arg)

nit: I realize this file is probably all over the place, but in PSM we've been trying to have the */& in type names next to the type as much as possible, as opposed to the name (so in this case this would be 'bool* arg')

@@ +243,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsNSSSocketInfo::GetFailedVerification(bool *arg)

nit: same

@@ +250,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsNSSSocketInfo::GetAuthenticationName(nsACString & aAuthenticationName)

nit: same idea: 'nsACString& aAuthenticationName'

@@ +252,5 @@
> +
> +NS_IMETHODIMP
> +nsNSSSocketInfo::GetAuthenticationName(nsACString & aAuthenticationName)
> +{
> +  //aAuthenticationName return GetHostName(aAuthenticationName);

not sure what this comment is for

@@ +258,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +nsNSSSocketInfo::SetAuthenticationName(const nsACString & aAuthenticationName)

nit: same

@@ +264,5 @@
> +  return SetHostName(PromiseFlatCString(aAuthenticationName).get());
> +}
> +
> +NS_IMETHODIMP
> +nsNSSSocketInfo::GetAuthenticationPort(int32_t *aAuthenticationPort)

nit: same

@@ -386,4 @@
>  {
> -  *_retval = false;
> -
> -  // Different ports may not be joined together

Is this not the case any longer? (I mean, I see this test was moved to the new JoinConnection, but should it still be enforced here as well?)

@@ -390,5 @@
> -  if (port != GetPort())
> -    return NS_OK;
> -
> -  // Make sure NPN has been completed and matches requested npnProtocol
> -  if (!mNPNCompleted || !mNegotiatedNPN.Equals(npnProtocol))

Same idea - do we still need this to be the case for the connection to be joinable?

::: security/manager/ssl/src/nsNSSIOLayer.h
@@ +114,5 @@
>    void SetMACAlgorithmUsed(int16_t mac) { mMACAlgorithmUsed = mac; }
>  
> +  inline bool GetBypassAuthentication()
> +  {
> +    bool result;

nit: let's initialize this to false so that if GetBypassAuthentication fails, we'll return a deterministic value when assertions are disabled.

@@ +122,5 @@
> +  }
> +
> +  inline int32_t GetAuthenticationPort()
> +  {
> +    int32_t result;

same idea (I don't know what you'd use, though - -1? 0? 443?)
Attachment #8488228 - Flags: review?(dkeeler)
(In reply to David Keeler (:keeler) [use needinfo?] from comment #9)
> What effect does this have on the UI/mixed content/etc.? In other words,
> will a page transmitted over non-secure TLS have the lock icon? If there's
> non-secure TLS content in a page that's otherwise secure, will the mixed
> content blocker do the right thing?

If this feature alters anything on the UI at all, then I'd be surprised.  As long as we don't have code anywhere that looks at whether TLS is in use, as opposed to checking for https:// vs http:// we should be OK.
(Assignee)

Comment 11

3 years ago
Comment on attachment 8488229 [details] [diff] [review]
HTTP/2 Alternate Service and Opportunistic Security [2/2 necko]

Review of attachment 8488229 [details] [diff] [review]:
-----------------------------------------------------------------

this is kind of a mushy long lived branch, so I'm sure it will require an iteration of cleanup - but let me know what you think. At least you've seen this before.

I'm going to update this
Attachment #8488229 - Flags: review?(hurley)
(Assignee)

Comment 12

3 years ago
(In reply to David Keeler (:keeler) [use needinfo?] from comment #9)
> Comment on attachment 8488228 [details] [diff] [review]
> HTTP/2 Alternate Service and Opportunistic Security [1/2 PSM]
> 
> Review of attachment 8488228 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> 
> ::: netwerk/socket/nsISSLSocketControl.idl
> @@ +120,5 @@
> > +    /**
> > +     * set bypassAuthentication to true if the server certificate checks should
> > +     * not be enforced. This is to enable non-secure transport over TLS.
> > +     */
> > +    [infallible] attribute boolean bypassAuthentication;
> 
> What effect does this have on the UI/mixed content/etc.? 


> In other words,
> will a page transmitted over non-secure TLS have the lock icon? 

An http:// page will not have a lock icon - no matter how secure any TLS it may or may not use is. An https:// page will always be authenticated.

those things are determined by scheme, and this doesn't impact scheme - so it doesn't have an impact. http:// is still http:// and treated weakly even if its transport is made a smidge better.

we did shake out a related bug in the ui a few weeks ago related to proxying http:// uris over https. So that's broadly a pre-existing concept.

If there's
> non-secure TLS content in a page that's otherwise secure, will the mixed
> content blocker do the right thing?

its all about the scheme. You can explicitly mix tls-http:// with plaintext-http:// without a problem (that's actually a big part of the point of the feature); but you can't mix tls-http:// onto a https:// page to any extent more than we already allow plaintext-http:// on a https:// page.

> 
> @@ -386,4 @@
> >  {
> > -  *_retval = false;
> > -
> > -  // Different ports may not be joined together
> 
> Is this not the case any longer? (I mean, I see this test was moved to the
> new JoinConnection, but should it still be enforced here as well?)

as you note, JoinConnection()'s semantics are unchanged.. the code is just rearranged so it can share with the new JoinableConnection() function. I guess the latter needs a different name - suggestions welcome, I'll take a pass at it in the update.

the differences here are in the context of http://tools.ietf.org/html/draft-ietf-httpbis-alt-svc-02
1] Joinable() is a test to see if the existing connection is authorized in a TLS sense to speak for another, specific host name. it doesn't actually coalesce connections like JoinConnection().
2] The port check does not apply for alt-svc as this is an explict routing (unlike spdy coalescing) that uses the port number as part of the routing information. Moving between ports is an expected part of operations.
3] the npn check does not apply for alt-svc as changing between protocols is actually a primary purpose of alt-svc when used in a lb context

e.g. you connect to a https uri at a big CDN that uses http/1 on 443 and the CDN would like to move requests for images.facebook.com from h2 speaking and SNI capable https traffic onto another cluster using h2 at IP=foo, port=900. (This is an unrelated use case to OS - full auth will be used here, because its https). Joinable() is meant to answer the question "is this existing connection authorized to provide routing for images.facebook.com?". If the answer is yes, a completely different check is needed after we connect to foo/900 to make sure that new connection is authoritative for images.facebook.com as well.

hth

I'll update the patches for the other bits. Thanks!
(Assignee)

Comment 13

3 years ago
Created attachment 8490187 [details] [diff] [review]
[PATCH 1/2]  - HTTP/2 Alternate Service and Opportunistic Security [1/2 PSM]
Attachment #8490187 - Flags: review?(dkeeler)
(Assignee)

Comment 14

3 years ago
Created attachment 8490188 [details] [diff] [review]
[PATCH 2/2]  - HTTP/2 Alternate Service and Opportunistic Security [2/2 necko]
Attachment #8490188 - Flags: review?(hurley)
(Assignee)

Comment 15

3 years ago
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b2eb92c48d37
(Assignee)

Updated

3 years ago
Attachment #8488228 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8488229 - Attachment is obsolete: true

Comment 16

3 years ago
Can i take this patch and apply to nightly code branch or is this patch generated for specific branch? if so, could you provide me branch/code base info/link. Once applied the patch, are there about:config options to enable/disable the feature or it's compile time changes and always enabled?
(Assignee)

Comment 17

3 years ago
(In reply to ritesh.patani@citrix.com from comment #16)
> Can i take this patch and apply to nightly code branch or is this patch
> generated for specific branch? if so, could you provide me branch/code base
> info/link. Once applied the patch, are there about:config options to
> enable/disable the feature or it's compile time changes and always enabled?

its basically nightly - the try build is on top of 3b7921328fc1

you can see the prefs in all.js https://hg.mozilla.org/try/rev/086e83f5de7d
Comment on attachment 8490187 [details] [diff] [review]
[PATCH 1/2]  - HTTP/2 Alternate Service and Opportunistic Security [1/2 PSM]

Review of attachment 8490187 [details] [diff] [review]:
-----------------------------------------------------------------

Ok - LGTM.
Attachment #8490187 - Flags: review?(dkeeler) → review+

Comment 19

3 years ago
Thanks Patrick for the info. 
Downloaded the "try" build windows32 version and with that build I see the about:config option for "network.http.altsvc.enabled" and "network.http.altsvc.oe" both set to True. so that's all good. 

I tried to test this firefox browser/build with http2 server which has h2-14 implemented and advertised "alt-svc" header with h2-14=443 in response to http URI (http 1.1). I was expecting firefox to send subsequent/future requests for the same http URI to the alt-svc location i.e 443 over TLS connection but i don't see firefox initiating that. It continues to send the http 1.1 request as before. So essentially it seems even though altsvc is enabled, firefox is not triggering the subsequent request to advertised alt-svc location as proposed in https://tools.ietf.org/html/draft-ietf-httpbis-http2-encryption-00

Anybody has tested this? Or I am missing something? 



(In reply to Patrick McManus [:mcmanus] from comment #17)
> (In reply to ritesh.patani@citrix.com from comment #16)
> > Can i take this patch and apply to nightly code branch or is this patch
> > generated for specific branch? if so, could you provide me branch/code base
> > info/link. Once applied the patch, are there about:config options to
> > enable/disable the feature or it's compile time changes and always enabled?
> 
> its basically nightly - the try build is on top of 3b7921328fc1
> 
> you can see the prefs in all.js https://hg.mozilla.org/try/rev/086e83f5de7d
(Assignee)

Comment 20

3 years ago
The governing draft is 
https://tools.ietf.org/html/draft-ietf-httpbis-alt-svc-02

(In reply to Ritesh P from comment #19)
> implemented and advertised "alt-svc" header with h2-14=443 in response to

That's not a legitimate production according to that draft spec. I think you're looking for:

Alt-Svc: h2-14=":443"

The colon and quotes are both required. We'll probably let the quotes slide, but not the colon.
(Assignee)

Comment 21

3 years ago
Comment on attachment 8490188 [details] [diff] [review]
[PATCH 2/2]  - HTTP/2 Alternate Service and Opportunistic Security [2/2 necko]

Review of attachment 8490188 [details] [diff] [review]:
-----------------------------------------------------------------

I'm gonna fuss with this some more :(
Attachment #8490188 - Flags: review?(hurley)
(Assignee)

Comment 22

3 years ago
Created attachment 8491059 [details] [diff] [review]
HTTP/2 Alternate Service and Opportunistic Security [1/2 PSM]
(Assignee)

Updated

3 years ago
Attachment #8490187 - Attachment is obsolete: true
(Assignee)

Comment 23

3 years ago
Created attachment 8491060 [details] [diff] [review]
HTTP/2 Alternate Service and Opportunistic Security [2/2 necko]
Attachment #8491060 - Flags: review?(hurley)
(Assignee)

Updated

3 years ago
Attachment #8490188 - Attachment is obsolete: true
(Assignee)

Comment 24

3 years ago
Comment on attachment 8491059 [details] [diff] [review]
HTTP/2 Alternate Service and Opportunistic Security [1/2 PSM]

includes david's request for initialized values in getters
Attachment #8491059 - Flags: review+
(Assignee)

Comment 25

3 years ago
a rebased try run that solved an e10s test fail
https://tbpl.mozilla.org/?tree=Try&rev=45d131e6336d
Comment on attachment 8491060 [details] [diff] [review]
HTTP/2 Alternate Service and Opportunistic Security [2/2 necko]

Review of attachment 8491060 [details] [diff] [review]:
-----------------------------------------------------------------

Looking mostly good. Some nits, a couple things I want you to verify before r+ing, and a few things that Just Need Fixed. One more round should do it (feel free to post an interdiff, too, to make the re-review easier)

::: modules/libpref/init/all.js
@@ +1224,5 @@
>  pref("network.http.spdy.push-allowance", 131072);
>  
> +// alt-svc allows separation of transport routing from
> +// the origin host without using a proxy.
> +#ifdef RELEASE_BUILD

Just want to be clear that we're on the same page that this limits this to only nightly & aurora. I'm fine with that, just wanting to make sure you didn't really want EARLY_BETA_OR_EARLIER here.

::: netwerk/protocol/http/AlternateServices.cpp
@@ +260,5 @@
> +  }
> +
> +  if (existing && existing->mValidated) {
> +    // update expires
> +    // todo verify dest info not just key

Either fix this TODO, drop it, or file a follow-up?

::: netwerk/protocol/http/AlternateServices.h
@@ +1,1 @@
> +/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

nit: add the vim modeline here, too (like you did in AlternateServices.cpp). My editor thanks you in advance.

::: netwerk/protocol/http/Http2Session.cpp
@@ +1935,5 @@
> +  uint8_t protoLen = self->mInputFrameBuffer.get()[kFrameHeaderBytes + 6];
> +  LOG3(("Http2Session::RecvAltSvc %p maxAge=%d port=%d protoLen=%d", self,
> +        maxAge, portRoute, protoLen));
> +
> +  if (self->mInputFrameDataSize < (7U + protoLen)) {

8U + protoLen

@@ +2020,5 @@
> +      self->ResetDownstreamState();
> +      return NS_OK;
> +    }
> +  } else {
> +    // no origin, and odd stream id

Either "even" stream id, or "unexpected" stream id (not sure which usage of "odd" you were going for there, but it's confusing all the same)

@@ +2190,2 @@
>      return rv;
> +

nit: remove the new blank line

::: netwerk/protocol/http/SpdySession3.cpp
@@ +1771,2 @@
>      return rv;
> +

nit: get rid of this new blank line

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +1267,5 @@
> +            if (!pairIndex) {
> +                // h2=:443
> +                npnToken = nsCString(parsedAltSvc.mValues[index].mValues[pairIndex].mName);
> +
> +                int32_t colonIndex = parsedAltSvc.mValues[index].mValues[pairIndex].mValue.FindChar(':');

nit: Perhaps suck parsedAltSvc...mName and parsedAltSvc...mValue out into a new "currentName" and "currentValue" (or some other appropriate variable names) before "if (!pairIndex)" above, and then assign them to other values as appropriate. That way you don't have a whole bunch of extra long or strangely-broken lines like you do right now.

@@ +1269,5 @@
> +                npnToken = nsCString(parsedAltSvc.mValues[index].mValues[pairIndex].mName);
> +
> +                int32_t colonIndex = parsedAltSvc.mValues[index].mValues[pairIndex].mValue.FindChar(':');
> +                if (colonIndex >= 0) {
> +                    portno = 

nit: trailing whitespace

@@ +1273,5 @@
> +                    portno = 
> +                        atoi(PromiseFlatCString(parsedAltSvc.mValues[index].mValues[pairIndex].mValue).get() +
> +                             colonIndex + 1);
> +                }
> +                else {

nit: put this on the same line as } above

@@ +4686,5 @@
> +         scheme.Equals(NS_LITERAL_CSTRING("https"))) &&
> +        (mapping = gHttpHandler->GetAltServiceMapping(scheme,
> +                                                      host, port,
> +                                                      mPrivateBrowsing))) {
> +        LOG(("nsHttpChannel %p Alt Service Mapping Found %s://%s::%d\n", this,

nit: ":%d" not "::%d"

::: netwerk/protocol/http/nsHttpConnectionInfo.cpp
@@ +114,2 @@
>  
> +    mHashKey.AssignLiteral(".....");

Add a comment for what byte 4 is, since they're all commented here otherwise. (Alternately kill the comments entirely, though I'm not a big fan of that idea.)

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +2841,1 @@
>          !AtActiveConnectionLimit(ent, args->mTrans->Caps())) {

aside: I feel like we should really comment this if at some point in the near-ish future. It keeps growing and getting more unwieldy and less understandable.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +1848,5 @@
>          if (mConnMgr && gSocketTransportService) {
>              nsCOMPtr<nsIRunnable> event = NS_NewRunnableMethod(mConnMgr,
>                  &nsHttpConnectionMgr::ClearConnectionHistory);
>              gSocketTransportService->Dispatch(event, NS_DISPATCH_NORMAL);
> +            mConnMgr->ClearAltServiceMappings();

This should probably be gated only on mConnMgr, not mConnMgr && gSocketTransportService

::: netwerk/test/unit/test_http2.js
@@ +348,5 @@
> +  onDataAvailable: function test_ODA(request, cx, stream, offset, cnt) {
> +   read_stream(stream, cnt);
> +  },
> +
> +  onStopRequest: function test_onStopR(request, ctx, status) {

So for this, I think I'd rather have a one-time (or n-time, for some low number of n if 1 isn't always reliable) allowance of !isHttp2Connection, instead of the current "allow it until we get h2 or the test gets timed out, whichever comes first". That way we can fail faster, and have a more reasonable explanation in the logs about why, exactly, the test failed, instead of just "Test timed out."

@@ +351,5 @@
> +
> +  onStopRequest: function test_onStopR(request, ctx, status) {
> +    var isHttp2Connection = checkIsHttp2(request);
> +    if (!isHttp2Connection) {
> +	dump("not over tls yet - retry\n");

Turn this into a comment, perhaps - no need to junk up the xpcshell logs

@@ +357,5 @@
> +				  null, null).QueryInterface(Components.interfaces.nsIHttpChannel);
> +	chan.asyncOpen(altsvcClientListener, null);
> +    } else {
> +        do_check_true(isHttp2Connection);
> +	dump(request.originalURI.spec + " X-Firefox-Spdy: " + request.getResponseHeader("X-Firefox-Spdy") + "\n");

drop this
Attachment #8491060 - Flags: review?(hurley)
Comment on attachment 8491060 [details] [diff] [review]
HTTP/2 Alternate Service and Opportunistic Security [2/2 necko]

Review of attachment 8491060 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttp.h
@@ +29,5 @@
>          SPDY_VERSION_2_REMOVED = 2,
>          SPDY_VERSION_3 = 3,
>          SPDY_VERSION_31 = 4,
>  
> +        HTTP_VERSION_2_0 = 20,

Oh, we should coordinate this and bug 1046915 on this, too, since they both have different names (and values) for this one. Since I'm ready to land 1046915, I vote we settle on that patch's name & value, but I could be biased ;)
(Assignee)

Comment 28

3 years ago
Thanks for looking at it in detail Nick!

(In reply to Nicholas Hurley [:hurley] from comment #26)

> Just want to be clear that we're on the same page that this limits this to
> only nightly & aurora.

Right - I'm not looking to maximize testing here. I actually want to leave this off for at least 1 release of h2 so it trails. Probably 1 more than that to give it time to progress on the standards track. But we should get it out there in nightly/aurora.

> ::: netwerk/test/unit/test_http2.js
>quest, ctx, status) {
> 
> So for this, I think I'd rather have a one-time (or n-time, for some low
> number of n if 1 isn't always reliable) allowance of !isHttp2Connection,

so it's async and not time bounded in terms of correctness. Which sucks.

The fail case is always going to be a timeout - why is the normal xpcshell timeout any worse than a lower one? Sounds just like a recipe for random orange to me.

In practice it seems to take 2 or 3 iterations on try to pass.. I don't think the upside of a fast fail on a positive test case is worth while given how annoying random orange is.
(Assignee)

Comment 29

3 years ago
(In reply to Nicholas Hurley [:hurley] from comment #27)
>
> Oh, we should coordinate this and bug 1046915 on this, too, since they both
>

first to land wins (probly means you :( ) - the other guy can just fix it as bitrot.
(Assignee)

Comment 30

3 years ago
Created attachment 8493467 [details] [diff] [review]
[PATCH 3/3]  - HTTP/2 Alternate Service and Opportunistic Security [2/2 necko] - appendix 1 -

I'll merge the two necko patches together before landing
Attachment #8493467 - Flags: review?(hurley)
(Assignee)

Comment 31

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=0999af8b37bc
(In reply to Patrick McManus [:mcmanus] from comment #28)
> so it's async and not time bounded in terms of correctness. Which sucks.
> 
> The fail case is always going to be a timeout - why is the normal xpcshell
> timeout any worse than a lower one? Sounds just like a recipe for random
> orange to me.
> 
> In practice it seems to take 2 or 3 iterations on try to pass.. I don't
> think the upside of a fast fail on a positive test case is worth while given
> how annoying random orange is.

That works for me. I was thinking that, if we explicitly time it out ourselves (after 10 or 20 iterations), the error message would be clearer as to what part broke, at least. For example, say we hit our 20 iteration limit, we could then have a do_check_true(false, "altsvc is failing!"), and that would show up in the logs, instead of just "Test timed out" like we get when the harness times the test out (which is always my least favorite error to get). If you feel like it, maybe add a "soft" timeout after n iterations where we pop something in the log so we could at least have some indicator of the issue, even if we don't time the test out ourselves. Probably not worth the effort, though.

I'll go through the interdiff now.
Comment on attachment 8493467 [details] [diff] [review]
[PATCH 3/3]  - HTTP/2 Alternate Service and Opportunistic Security [2/2 necko] - appendix 1 -

Review of attachment 8493467 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the one note below fixed.

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +1844,5 @@
>          if (mConnMgr) {
>              mConnMgr->ClearAltServiceMappings();
>          }
>      } else if (strcmp(topic, "browser:purge-session-history") == 0) {
> +        if (mConnMgr) {

Argh, sorry. I meant to put just the "mConnMgr->ClearAltServiceMappings();" in a branch that's only dependent on mConnMgr being true-ish. The other bits still need to check gSocketTransportService, since they make use of it.
Attachment #8493467 - Flags: review?(hurley) → review+
Comment on attachment 8491060 [details] [diff] [review]
HTTP/2 Alternate Service and Opportunistic Security [2/2 necko]

Review of attachment 8491060 [details] [diff] [review]:
-----------------------------------------------------------------

r=me when merged with the interdiff part 3

::: netwerk/protocol/http/nsHttp.h
@@ +29,5 @@
>          SPDY_VERSION_2_REMOVED = 2,
>          SPDY_VERSION_3 = 3,
>          SPDY_VERSION_31 = 4,
>  
> +        HTTP_VERSION_2_0 = 20,

FWIW, I just bitrotted you on this.
Attachment #8491060 - Flags: review+
(Assignee)

Updated

3 years ago
Depends on: 1076129
(Assignee)

Comment 35

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=fab7d944e634
(Assignee)

Comment 36

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/086fe4b0ba14
https://hg.mozilla.org/integration/mozilla-inbound/rev/3034162ee435
(Assignee)

Comment 37

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e947d1636f1
https://hg.mozilla.org/integration/mozilla-inbound/rev/61f98b1d29f9
(Assignee)

Comment 38

3 years ago
I believe I managed to make a new head with the comment 36 push. my bad. repushed onto the normal head.
(Assignee)

Comment 39

3 years ago
backed out - see 
https://bugzilla.mozilla.org/show_bug.cgi?id=1076129#c8
(In reply to Patrick McManus [:mcmanus] from comment #37)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/8e947d1636f1
> https://hg.mozilla.org/integration/mozilla-inbound/rev/61f98b1d29f9

Both backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/916f2837201c when I eventually realized that the summary of bug 1076129 was entirely accurate, and once it was backed out these needed to come out too.
(Assignee)

Comment 41

3 years ago
Created attachment 8501349 [details] [diff] [review]
appendix2-connect-tunnel-counting

a followon - I'll merge this into patch 2 (necko) before landing.

The base patch modifies the mConcurrent counting to not count null transactions, which don't follow the normal lifecycle of nsHttpTransactions.. that's all well and good, but spdyconnecttransactions (tunnels) are a null transaction of a sort and get caught up in that. This special cases them for spdy.

h2 is a bit smarter here and tracks things by a reference bit in the stream, so it can stay as is.
Attachment #8501349 - Flags: review?(hurley)
(Assignee)

Comment 42

3 years ago
https://tbpl.mozilla.org/?tree=Try&rev=bf7aaf695287
Comment on attachment 8501349 [details] [diff] [review]
appendix2-connect-tunnel-counting

This is not a patch :)
Attachment #8501349 - Flags: review?(hurley)
FWIW, the patch I see on the top of the try push, however, looks good.
Comment on attachment 8501349 [details] [diff] [review]
appendix2-connect-tunnel-counting

I've been itching to do this...
Attachment #8501349 - Flags: review-
(Assignee)

Comment 46

3 years ago
(In reply to Martin Thomson [:mt] from comment #45)
> Comment on attachment 8501349 [details] [diff] [review]
> appendix2-connect-tunnel-counting
> 
> I've been itching to do this...

:)
(Assignee)

Comment 47

3 years ago
Created attachment 8501982 [details] [diff] [review]
0001-appendix-to-patch-2-in-spdy-count-the-special-null-c.patch

gonna mark it r+ based on comment 44.. adjust if required.
Attachment #8501349 - Attachment is obsolete: true
Attachment #8501982 - Flags: review+
r+ indeed. Though next time, make sure the patches you post in bugzilla compile ;)
(Assignee)

Comment 49

3 years ago
Created attachment 8503444 [details] [diff] [review]
HTTP/2 Alternate Service and Opportunistic Security [2/2 necko]
(Assignee)

Updated

3 years ago
Attachment #8493467 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8491060 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8501982 - Attachment is obsolete: true
(Assignee)

Comment 50

3 years ago
Comment on attachment 8503444 [details] [diff] [review]
HTTP/2 Alternate Service and Opportunistic Security [2/2 necko]

rolled together the previous patches
Attachment #8503444 - Flags: review+
(Assignee)

Comment 51

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/eb92720d6cd2
https://hg.mozilla.org/integration/mozilla-inbound/rev/fcf6d37ae66c
https://hg.mozilla.org/mozilla-central/rev/eb92720d6cd2
https://hg.mozilla.org/mozilla-central/rev/fcf6d37ae66c
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla35

Comment 53

3 years ago
So if I read the patches correctly, the pref is disabled for Release/Beta. Is there a bug to enable it?

This should probably be mentioned in Fx 35 for Developers, and also needs general documentiation --> dev-doc-needed.
Keywords: dev-doc-needed
(Assignee)

Comment 54

3 years ago
Florian a blog post will follow.. and then I'll add dev-doc needed so the doc team has something to work with. Its not high pressure, because as you note its on the full release train yet.

that's because we want actual h2 support to land first and the IETF working group still has it stuck in draft status.
(Assignee)

Comment 55

3 years ago
but in the meantime, its in the early channels so we can see how it works.
(Assignee)

Updated

3 years ago
Depends on: 1092368

Comment 56

3 years ago
Thanks! Is there a bug for pref on, yet? Or a meta/tracking bug where I can follow along?

Updated

3 years ago
Flags: needinfo?(mcmanus)
(Assignee)

Comment 57

3 years ago
Florian - this is pending ietf disposition of h2 as I indicated in comment 54.
Flags: needinfo?(mcmanus)

Comment 58

3 years ago
Thanks, I was just wondering if there was a bug to track to get notified as soon as things are going forward. Doesn't look like that, though!
(Assignee)

Updated

3 years ago
Blocks: 1113790
(Assignee)

Updated

2 years ago
Depends on: 1130874
Depends on: 1170066
Depends on: 1172615
You need to log in before you can comment on or make changes to this bug.