https proxying over http/2 with nghttpx null ptr

RESOLVED FIXED in mozilla32

Status

()

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

People

(Reporter: mcmanus, Assigned: mcmanus)

Tracking

29 Branch
mozilla32
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [spdy])

Attachments

(1 attachment)

origin http2 streams using connect tunnels aren't correctly associated with the stream in the proxy session when connected to the proxy over http2

 https://tbpl.mozilla.org/?tree=Try&rev=7aaea957e171
Created attachment 8425022 [details] [diff] [review]
0001-https-proxying-over-http-2-connect-stream.patch
Attachment #8425022 - Flags: review?(hurley)
reporter (over email) verifies the try build fixes the issue
Comment on attachment 8425022 [details] [diff] [review]
0001-https-proxying-over-http-2-connect-stream.patch

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

::: netwerk/protocol/http/Http2Stream.cpp
@@ +938,5 @@
>  {
> +  if (mAllHeadersReceived) {
> +    return;
> +  }
> +    

nit: whitespace

::: netwerk/protocol/http/TunnelUtils.cpp
@@ +932,4 @@
>  {
>    MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
>    LOG(("SpdyConnectTransaction::WriteSegments %p max=%d cb=%p\n",
> +       this, count, mTunneledConn ? mTunnelStreamIn->mCallback : nullptr));

Even though this technically works since they're both initialized on the same stack, it would be much better as mTunnelStreamIn (or mTunneledConn && mTunnelStreamIn if you *really* want to check mTunneledConn) before the ?
Attachment #8425022 - Flags: review?(hurley) → review+
(In reply to Nicholas Hurley [:hurley] from comment #3)

> ::: netwerk/protocol/http/TunnelUtils.cpp
> @@ +932,4 @@
> >  {
> >    MOZ_ASSERT(PR_GetCurrentThread() == gSocketThread);
> >    LOG(("SpdyConnectTransaction::WriteSegments %p max=%d cb=%p\n",
> > +       this, count, mTunneledConn ? mTunnelStreamIn->mCallback : nullptr));
> 
> Even though this technically works since they're both initialized on the
> same stack, it would be much better as mTunnelStreamIn (or mTunneledConn &&
> mTunnelStreamIn if you *really* want to check mTunneledConn) before the ?

I actually did that on purpose - the tunneledconn represents state (that the tunneled connection has been created) as much as anything else.. and various attributes go with that (e.g. tunnelStream*)
That's what I figured, but it just looks so weird. Maybe a comment for future poor souls? (Commenting about logging... so meta!)
https://hg.mozilla.org/integration/mozilla-inbound/rev/1950bbf9626c
https://hg.mozilla.org/mozilla-central/rev/1950bbf9626c
Assignee: nobody → mcmanus
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.