Support http/2 ORIGIN frame

RESOLVED FIXED in Firefox 55

Status

()

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

People

(Reporter: nwgh, Assigned: mcmanus)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [necko-active][spdy])

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Updated

a year ago
Whiteboard: [necko-next] → [necko-active]
(Assignee)

Updated

a year ago
Blocks: 1316316, 1056987
Whiteboard: [necko-active] → [necko-active][spdy]
(Assignee)

Comment 3

a year ago
Created attachment 8852696 [details] [diff] [review]
JoinConnection() from psm 1/3
Attachment #8852696 - Flags: review?(dkeeler)
(Assignee)

Updated

a year ago
Assignee: hurley → mcmanus
Status: NEW → ASSIGNED
(Assignee)

Comment 4

a year ago
Created attachment 8852698 [details] [diff] [review]
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.
Attachment #8852698 - Flags: review?(hurley)
(Assignee)

Comment 5

a year ago
Created attachment 8852699 [details] [diff] [review]
http/2 origin frame extension test 3/3
Attachment #8852699 - Flags: review?(hurley)
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/
Attachment #8852696 - Flags: review?(dkeeler) → review+
(Reporter)

Comment 7

a year ago
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
Attachment #8852698 - Flags: review?(hurley) → review-
(Reporter)

Comment 8

a year ago
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 :)
Attachment #8852699 - Flags: review?(hurley) → review+
(Assignee)

Comment 9

a year ago
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 hurley@todesschaf.org) 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
(Reporter)

Comment 10

a year ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8854178 - Flags: review?(dkeeler)
(Assignee)

Updated

a year ago
Attachment #8854179 - Flags: review?(hurley)
(Assignee)

Updated

a year ago
Attachment #8854180 - Flags: review?(hurley)
(Assignee)

Updated

a year ago
Attachment #8852696 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8852698 - Attachment is obsolete: true
(Assignee)

Updated

a year ago
Attachment #8852699 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 18

a year ago
@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.
(Assignee)

Updated

a year ago
Attachment #8854180 - Attachment is obsolete: true
Attachment #8854180 - Flags: review?(hurley)
(Assignee)

Updated

a year ago
Attachment #8854179 - Attachment is obsolete: true
Attachment #8854179 - Flags: review?(hurley)
(Assignee)

Updated

a year ago
Attachment #8854178 - Attachment is obsolete: true
Attachment #8854178 - Flags: review?(dkeeler)
(Assignee)

Comment 19

a year ago
Created attachment 8854238 [details] [diff] [review]
JoinConnection() from psm 1/3
(Assignee)

Comment 20

a year ago
Created attachment 8854240 [details] [diff] [review]
http/2 origin frame extension 2/3
Attachment #8854240 - Flags: review?(hurley)
(Assignee)

Comment 21

a year ago
Created attachment 8854241 [details] [diff] [review]
http/2 origin frame extension test 3/3
(Assignee)

Updated

a year ago
Attachment #8854238 - Flags: review+
(Assignee)

Updated

a year ago
Attachment #8854241 - Flags: review+
(Assignee)

Comment 22

a year ago
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.
(Reporter)

Comment 23

a year ago
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!
Attachment #8854240 - Flags: review?(hurley) → review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 24

a year ago
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
(Assignee)

Updated

a year ago
Depends on: 1353497
(Assignee)

Comment 25

a year ago
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

a year ago
bugherder
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
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Updated

a year ago
Depends on: 1355277
(Assignee)

Updated

a year ago
Depends on: 1355591
(Assignee)

Updated

a year ago
No longer depends on: 1355591
(Assignee)

Updated

a year ago
Depends on: 1355591
(Assignee)

Updated

a year ago
Depends on: 1381340
You need to log in before you can comment on or make changes to this bug.