Seen while trying to debug bug 1170928. (One of many things blocking me from actually debugging that issue.) I have an rr trace, and am tracking down what's going wrong to cause this. Specifically, this is on one of the OCSP channels that's being opened - it's HTTP, which should not be using CONNECT, but somewhere we're thinking we should try to CONNECT (which my backend proxy is, currently, apparently not set up to support). The CONNECT fails, and that's when things go crazy. The real question - why are we trying to CONNECT in the first place?

OK, I've made a bit of progress here, but need to validate some assumptions I'm making (which, if correct, will invalidate some assumptions the code is making). The problem (at first glance) seems to come in nsHttpConnection::Activate (specifically https://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpConnection.cpp#410) - at that point, we've determined that the transaction should be placed onto the proxy connection (since we're proxying everything), and since this is the first transaction on the connection, we immediately go to Activate. There, we look at the conninfo for the proxy to see if we should use CONNECT, which, in the case of a connection to an h2 proxy, will always say "yes" (since it's end-to-end encrypted). So it seems to me as if we should really be asking the transaction's conninfo if CONNECT is necessary. I tried that, but that ended up hanging the browser (instead of crashing on an assert). So, I'm unsure if the assumptions I'm making (that we should ask the transaction's conninfo about CONNECT) are incorrect, or if there's some assumption somewhere else in the proxy path that is incorrect that my change is tickling in a bad way. Patrick, any thoughts on my assumption? (I can always dig in and find the incorrect assumption in the code if my decision seems right to you.)

this involved resurrecting some retired brain cells. sorry for the delay - and I'm still not 100% confident of what I'm saying but I hope it helps. (In reply to Nicholas Hurley [:nwgh][:hurley] from comment #1) > >There, we look at the conninfo for the proxy to > see if we should use CONNECT, which, in the case of a connection to an h2 > proxy, will always say "yes" (since it's end-to-end encrypted). > well, you look at the conninfo of the connection right? and its different that the transaction conninfo? There is probably a reason for that I forget but it doesn't sound right. A http:// transaction should have mUsingConnect being false because musingConnect is based on mEndToEndSSL (or RESOLVE_ALWAYS_TUNNEL which shouldn't be in play here) which isn't implied by the h2 proxy.. the h2 proxy should set usingHTTPSProxy and using HTTPProxy though. What's the hash key for the connection info? > So it seems to me as if we should really be asking the transaction's > conninfo if CONNECT is necessary. I tried that, but that ended up hanging > the browser (instead of crashing on an assert). So, I'm unsure if the > assumptions I'm making (that we should ask the transaction's conninfo about > CONNECT) are incorrect, or if there's some assumption somewhere else in the > proxy path that is incorrect that my change is tickling in a bad way. > > Patrick, any thoughts on my assumption? (I can always dig in and find the > incorrect assumption in the code if my decision seems right to you.)

So for fun, I did one more run using the transaction conninfo to determine end-to-end-ssl instead of the conninfo of the connection to the proxy. This time it worked just fine (didn't hang or anything). I am seeing one strange thing in the http log of a working session, though, which could be related to this: 2016-04-14 16:42:47.251126 UTC - [Socket Thread]: V/nsHttp nsHttpConnectionMgr::MakeConnEntryWildCard conn 7fce1e4f2100 has requested to change CI from T.A...localhost:3000 to TSA...*:0 (https:localhost:3000)[:] Which is changing the CI on a connection (the connection to the proxy) from non-end-to-end-ssl to end-to-end-ssl (as well as making it wildcard). That doesn't seem totally wrong, or it at least seems intentional (the comments in nsHttpConnectionInfo::CreateWildCard straight up say the S bit on the hash will end up set). I'm going to keep playing with this a bit more to see if I can get it to fail in the same ways I've gotten it to fail previously before putting up a patch, as I'm not totally confident in what's going on yet :)

Tatsuhiro - can you try a build from https://archive.mozilla.org/pub/firefox/try-builds/hurley@todesschaf.org-c3bbf93c69e239d555b2284d7241f64fcedb1374/ (it will probably take another few hours before they're ready) to see if they fix your issue reported in bug 1286568? They should, but I'm especially interested if any new issues arise. For my own reference, the try run associated with the builds linked above is https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3bbf93c69e2

Thank you. I tried the build, and it looks like it fixed the issue I observed so far.

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

So the review request above is for the same build I gave to Tatsuhiro. Everything in my testing (and the try run) seems to indicate it's OK, and so far it seems that Tatsuhiro has had no issues, either.

Comment on attachment 8775251 [details] Bug 1253699 - Use transaction to determine end-to-end tls when proxying. https://reviewboard.mozilla.org/r/67442/#review64526 I can see that. thanks!

Thanks for the review. I'm going to wait until 51 to land this, just to ensure it gets a full cycle to bake (it's only a 1-liner, but I want to have as little chance of unexpectedly breaking proxying as possible)

Pushed by hurley@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/13a2730a7ef4 Use transaction to determine end-to-end tls when proxying. r=mcmanus