1337791 - Support http/2 ORIGIN frame

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[u408661]

https://github.com/httpwg/http-extensions/blob/master/draft-ietf-httpbis-origin-frame.md

Comment 1

[Patrick McManus [:mcmanus]]

wip
 https://tbpl.mozilla.org/?tree=Try&rev=d4acb7e091b6

Comment 2

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6f2d8de48640

Comment 3

[Patrick McManus [:mcmanus]]

Attachment: JoinConnection() from psm 1/3

Comment 4

[Patrick McManus [:mcmanus]]

Attachment: http/2 origin frame extension 2/3

https://github.com/httpwg/http-extensions/blob/master/draft-ietf-httpbis-origin-frame.md

ORIGIN allows a h2 connection to advertise other origins that it is
willing to service on the same connection - normal TLS validation
still applies for the new origin. Fundamentally this is a connection
scoped dns reply.

Comment 5

[Patrick McManus [:mcmanus]]

Attachment: http/2 origin frame extension test 3/3

Comment 6

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Comment on attachment 8852696 [details] [diff] [review]
JoinConnection() from psm 1/3

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

LGTM.

::: netwerk/socket/nsISSLSocketControl.idl
@@ +69,5 @@
> +      in ACString npnProtocol, /* e.g. "h2" */
> +      in ACString hostname,
> +      in long port);
> +
> +    /* just like joinConnection() except do not mark a successful test as joined.

nit: s/j/J/

Comment 7

[u408661]

Comment on attachment 8852698 [details] [diff] [review]
http/2 origin frame extension 2/3

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

Mostly looks great, but the ORIGIN parsing algorithm in the code doesn't match that defined in the spec.

::: netwerk/protocol/http/Http2Session.cpp
@@ +2328,5 @@
> +  MOZ_ASSERT(self->mInputFrameType == FRAME_TYPE_ORIGIN);
> +  LOG3(("Http2Session::RecvOrigin %p Flags 0x%X id 0x%X\n", self,
> +        self->mInputFrameFlags, self->mInputFrameID));
> +
> +  if (self->mInputFrameFlags & 0xF0) {

This is the wrong way for flags - 0x{1,2,4,8} need to be zero, but this checks to see if every bit *except* those is zero.

@@ +2369,5 @@
> +    RefPtr<nsStandardURL> originURL;
> +    originString.Assign(self->mInputFrameBuffer.get() + kFrameHeaderBytes + offset + 2, originLen);
> +    if (NS_FAILED(Http2Stream::MakeOriginURL(originString, originURL))){
> +      LOG3(("Http2Session::RecvOrigin %p origin frame string %s failed to parse\n", self, originString.get()));
> +      self->ResetDownstreamState();

So this should be a continue instead of ignoring the rest of the frame. https://tools.ietf.org/html/draft-ietf-httpbis-origin-frame-02#appendix-A (note the end of step 6 sub-step 1). Same for the rest of these reset/returns in this loop.

@@ +2381,5 @@
> +      self->ResetDownstreamState();
> +      return NS_OK;
> +    }
> +
> +    self->InitializeOriginSet();

This should be hoisted outside the loop (same link as above).

@@ +2403,5 @@
> +      LOG3(("Http2Session::RecvOrigin %p origin frame already in set\n", self));
> +    }
> +    offset += originLen + 2;
> +  }
> +  // init in case the loop did not run and we didn't hit early return on first iter

OK, I see what you did here, but a simple read-through makes this non-obvious. Above the loop makes more sense here.

::: netwerk/protocol/http/nsHttpConnectionMgr.cpp
@@ +584,5 @@
>  
> +nsHttpConnection *
> +nsHttpConnectionMgr::FindCoalescableConnectionByHashKey(nsConnectionEntry *ent,
> +                                                        const nsCString &key,
> +                                                        bool justKidding)

:)

@@ +596,5 @@
> +        return nullptr;
> +    }
> +
> +    uint32_t listLen = listOfWeakConns->Length();
> +    for (uint32_t j = 0; j < listLen; ) {

nit: this loop body feels like it could take up a few more lines, vertically - it's a bit hard to read with everything squashed together with no blank lines in sight.

@@ +626,5 @@
> +                 potentialMatch.get(), key.get(), ci->HashKey().get()));
> +        }
> +        ++j; // bypassed by continue when weakptr fails
> +    }
> +    if (!listLen) { // shurnk to 0 while iterating

nit: s/shurnk/shrunk/

@@ +685,5 @@
> +
> +    LOG(("FindCoalescableConnection(%s) no match conn\n", ci->HashKey().get()));
> +    return nullptr;
> +}
> +       

nit: trailing whitespace

@@ +721,5 @@
> +            LOG(("UpdateCoalescingForNewConn() need new list element\n"));
> +            listOfWeakConns = new nsTArray<nsWeakPtr>(1);
> +            mCoalescingHash.Put(ent->mCoalescingKeys[i], listOfWeakConns);
> +        }
> +        listOfWeakConns->AppendElement(do_GetWeakReference(static_cast<nsISupportsWeakReference*>(conn)));

nit: This could just be splinter displaying stuff poorly, but it looks to me like this line has no leading whitespace. (Watch, it'll show up as leading whitespace just fine in my saved review comment.)

@@ +736,5 @@
> +    if (ent->mActiveConns.Length() > 1) {
> +        // this is a new connection that can be coalesced onto
> +        // if there is more than 1 live and established spdy connection (e.g.
> +        // some could still be handshaking, shutting down, etc..) then close
> +        // this one down after any transactions that are on it are complete.

This comment sounds to me like you'll be shutting down the connection passed into this method (conn), but the code reads like you're shutting down anything *not* the one passed into this method (otherConn). Let's make the two line up.

@@ +2081,5 @@
>      if (mTrafficTimer) {
>        mTrafficTimer->Cancel();
>        mTrafficTimer = nullptr;
>      }
> +    mCoalescingHash.Clear();

So associated with this, it might be good to assert in the dtor that mCoalescingHash.Length() == 0

Comment 8

[u408661]

Comment on attachment 8852699 [details] [diff] [review]
http/2 origin frame extension test 3/3

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

LGTM. Please upstream the node-http2 changes. I promise I'll take the patch :)

Comment 9

[Patrick McManus [:mcmanus]]

thanks for taking the time with this one.. I know the conncection manager bits were a lot more invasive than the origin frame parts.. but the result is simpler and it closes 2 blocker bugs for a bonus!

(In reply to Nicholas Hurley [:nwgh][:hurley] (also [e-mail]) from comment #7)
> Comment on attachment 8852698 [details] [diff] [review]
> http/2 origin frame extension 2/3
> 
> Review of attachment 8852698 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Mostly looks great, but the ORIGIN parsing algorithm in the code doesn't
> match that defined in the spec.
> 
> ::: netwerk/protocol/http/Http2Session.cpp
> @@ +2328,5 @@
> > +  MOZ_ASSERT(self->mInputFrameType == FRAME_TYPE_ORIGIN);
> > +  LOG3(("Http2Session::RecvOrigin %p Flags 0x%X id 0x%X\n", self,
> > +        self->mInputFrameFlags, self->mInputFrameID));
> > +
> > +  if (self->mInputFrameFlags & 0xF0) {
> 
> This is the wrong way for flags - 0x{1,2,4,8} need to be zero, but this
> checks to see if every bit *except* those is zero.

ha! you're right thank you. Interestingly I did this because the spec calls them the "first four flags" - which doesn't really have a definition, but I certainly think of as 0xf0 LtoR for first and all.. (it also enumerates them which I should have noticed.

> 
> @@ +2369,5 @@
> > +    RefPtr<nsStandardURL> originURL;
> > +    originString.Assign(self->mInputFrameBuffer.get() + kFrameHeaderBytes + offset + 2, originLen);
> > +    if (NS_FAILED(Http2Stream::MakeOriginURL(originString, originURL))){
> > +      LOG3(("Http2Session::RecvOrigin %p origin frame string %s failed to parse\n", self, originString.get()));
> > +      self->ResetDownstreamState();
> 
> So this should be a continue instead of ignoring the rest of the frame.
> https://tools.ietf.org/html/draft-ietf-httpbis-origin-frame-02#appendix-A
> (note the end of step 6 sub-step 1). Same for the rest of these
> reset/returns in this loop.
> 

I guess I can do that. Appendix A is not normative and is pretty clear that it is one way a client could do this processing and I'm not a big fan of the kind of soft error handling it describes. otoh since I'm neither rolling back state (which is proably the best thing to do) nor throwing protocol error my approach isn't especially consistent.

> 
> nit: s/shurnk/shrunk/
> 

heh. I'm kinda liking the sound of shurnk

Comment 10

[u408661]

(In reply to Patrick McManus [:mcmanus] from comment #9)
> thanks for taking the time with this one.. I know the conncection manager
> bits were a lot more invasive than the origin frame parts.. but the result
> is simpler and it closes 2 blocker bugs for a bonus!

Indeed! The connmgr changes are going to be great to have - that logic was so hard to wrap my head around!

> > @@ +2369,5 @@
> > > +    RefPtr<nsStandardURL> originURL;
> > > +    originString.Assign(self->mInputFrameBuffer.get() + kFrameHeaderBytes + offset + 2, originLen);
> > > +    if (NS_FAILED(Http2Stream::MakeOriginURL(originString, originURL))){
> > > +      LOG3(("Http2Session::RecvOrigin %p origin frame string %s failed to parse\n", self, originString.get()));
> > > +      self->ResetDownstreamState();
> > 
> > So this should be a continue instead of ignoring the rest of the frame.
> > https://tools.ietf.org/html/draft-ietf-httpbis-origin-frame-02#appendix-A
> > (note the end of step 6 sub-step 1). Same for the rest of these
> > reset/returns in this loop.
> > 
> 
> I guess I can do that. Appendix A is not normative and is pretty clear that
> it is one way a client could do this processing and I'm not a big fan of the
> kind of soft error handling it describes. otoh since I'm neither rolling
> back state (which is proably the best thing to do) nor throwing protocol
> error my approach isn't especially consistent.

Yeah, I know A isn't normative, it was just the clearest explanation. Section 2.2 does strongly hint at the behavior outlined in A, though - "Each ASCII-Origin field in the frame's payload MUST be parsed as an ASCII serialisation of an origin ([RFC6454], Section 6.2).  If parsing fails, the field MUST be ignored." - note it calls out the field being ignored, not the entire frame.

> > nit: s/shurnk/shrunk/
> > 
> 
> heh. I'm kinda liking the sound of shurnk

:) It does have a certain ring to it!

Comment 11

[Patrick McManus [:mcmanus]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4b162a111682

Comment 12

[Patrick McManus [:mcmanus]]

Attachment: Bug 1337791 - JoinConnection() from psm 1/3

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

Comment 13

[Patrick McManus [:mcmanus]]

Attachment: Bug 1337791 - http/2 origin frame extension 2/3

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

Comment 14

[Patrick McManus [:mcmanus]]

Attachment: Bug 1337791 - http/2 origin frame extension test 3/3

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

Comment 15

[Patrick McManus [:mcmanus]]

Comment on attachment 8854178 [details]
Bug 1337791 - JoinConnection() from psm 1/3

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126142/diff/1-2/

Comment 16

[Patrick McManus [:mcmanus]]

Comment on attachment 8854179 [details]
Bug 1337791 - http/2 origin frame extension 2/3

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126144/diff/1-2/

Comment 17

[Patrick McManus [:mcmanus]]

Comment on attachment 8854180 [details]
Bug 1337791 - http/2 origin frame extension test 3/3

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/126146/diff/1-2/

Comment 18

[Patrick McManus [:mcmanus]]

@hurley @keeler - I'm sorry for the noise.. I simultaneously tried to update my git to use a different root, rebase the patch, and introduce mozreview (to make the interdiff easier or even correct afaict).

it did not make anything easier. I'll update with the new version later the old way and leave my new workflow for new patch sets.

Comment 19

[Patrick McManus [:mcmanus]]

Attachment: JoinConnection() from psm 1/3

Comment 20

[Patrick McManus [:mcmanus]]

Attachment: http/2 origin frame extension 2/3

Comment 21

[Patrick McManus [:mcmanus]]

Attachment: http/2 origin frame extension test 3/3

Comment 22

[Patrick McManus [:mcmanus]]

Comment on attachment 8854240 [details] [diff] [review]
http/2 origin frame extension 2/3

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

this is the real r? for the updated patch.. sorry again about the noise - our toolchain can be frustrating.

Comment 23

[u408661]

Comment on attachment 8854240 [details] [diff] [review]
http/2 origin frame extension 2/3

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

Indeed, the toolchain can be frustrating.

Only other thing I'd bikeshed on would be I feel like the originextension pref should be under the spdy namespace, but it's not worth changing. If I'd thought of that first round, I may have asked, but at this point... land that thing!

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f8f5cada5b7
Part 1: JoinConnection() from psm. r=keeler
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbb9934d1a90
Part 2: http/2 origin frame extension. r=nwgh
https://hg.mozilla.org/integration/mozilla-inbound/rev/71b228f12fc1
Part 3: http/2 origin frame extension test. r=nwgh

Comment 25

[Patrick McManus [:mcmanus]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b7e27246485369587088ec572f59f397c4bff72
bug 1337791 - disable h2 origin test for android that has no server r=test-only

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7f8f5cada5b7
https://hg.mozilla.org/mozilla-central/rev/fbb9934d1a90
https://hg.mozilla.org/mozilla-central/rev/71b228f12fc1
https://hg.mozilla.org/mozilla-central/rev/8b7e27246485