This patch fixes, multiple socket status events. The second problem that is partially fixing is: a nsHttpTransaction finds a halfOpen speculative connection and transform it into non-speculative, but if TryDispatchTransaction gets call again it will open a new halfOpen socket (that was happening in Patrick's log). We will still have multiple halfOpen sockets for a single transaction if a transaction finds a NullHttpTransaction on an active connection: https://hg.mozilla.org/mozilla-central/annotate/tip/netwerk/protocol/http/nsHttpConnectionMgr.cpp#l1215

I'm a little nervous at what it means for a half-open socket to drop mTransaction and replace it with something else mid stream. I can't find a reason it wouldn't work though. do you see any way to extract the event logic here for uplift? (In reply to Dragana Damjanovic [:dragana] from comment #1) > Created attachment 8839580 [details] [diff] [review] > bug_1340164_v1.patch > > This patch fixes, multiple socket status events. > > The second problem that is partially fixing is: > a nsHttpTransaction finds a halfOpen speculative connection and transform it > into non-speculative, but if TryDispatchTransaction gets call again it will double check if I'm interpreting this right: so on iteration one setspeculative gets set to false, but the connection is incomplete so the trans just gets stuck in the pending queue.. and pretty soon something makes it come out of the pending queue and it just makes a new connection because it can't find an idle or speculative half-open one.. when it should really be waiting around for the one from iteration one to complete. do I have that right? I think there is a similar problem around 1210.. where 1 trans spinning around on the pending queue could Claim() all the active null transactions (across multiple iterations) before one actually got put back inthe idle pool. that would result in extra conns too. wdyt?

Comment on attachment 8839580 [details] [diff] [review] bug_1340164_v1.patch Review of attachment 8839580 [details] [diff] [review]: ----------------------------------------------------------------- are you missing the SetNullTransaction logic in AddToShortestPipeline()? ::: netwerk/protocol/http/nsHttpConnectionMgr.cpp @@ +1690,5 @@ > } > + // Remove this transaction from its half open socket, if existing. > + for (int32_t j = 0; j < ((int32_t) ent->mHalfOpens.Length()); ++j) { > + if (ent->mHalfOpens[j]->Transaction() == trans) { > + ent->mHalfOpens[j]->SetNullTransaction(); trailing whitespace @@ +1720,5 @@ > + > + // Remove this transaction from its half open socket, if existing. > + if (NS_SUCCEEDED(rv)) { > + for (int32_t j = 0; j < ((int32_t) ent->mHalfOpens.Length()); ++j) { > + if (ent->mHalfOpens[j]->Transaction() == trans) { this loop deserves its own subroutine ::: netwerk/protocol/http/nsHttpConnectionMgr.h @@ +487,5 @@ > > bool mPrimaryConnectedOK; > bool mBackupConnectedOK; > + > + nsresult mPrimaryStreamStatus; I would feel better if this was initialized

(In reply to Patrick McManus [:mcmanus] from comment #3) > I'm a little nervous at what it means for a half-open socket to drop > mTransaction and replace it with something else mid stream. I can't find a > reason it wouldn't work though. > I did tested it quit a bit, you can easily trigger it with larger rtt(larger than 250ms). > do you see any way to extract the event logic here for uplift? > > (In reply to Dragana Damjanovic [:dragana] from comment #1) > > Created attachment 8839580 [details] [diff] [review] > > bug_1340164_v1.patch > > > > This patch fixes, multiple socket status events. > > > > The second problem that is partially fixing is: > > a nsHttpTransaction finds a halfOpen speculative connection and transform it > > into non-speculative, but if TryDispatchTransaction gets call again it will > > double check if I'm interpreting this right: > so on iteration one setspeculative gets set to false, but the connection is > incomplete so the trans just gets stuck in the pending queue.. and pretty > soon something makes it come out of the pending queue and it just makes a > new connection because it can't find an idle or speculative half-open one.. > when it should really be waiting around for the one from iteration one to > complete. do I have that right? > Yes. > I think there is a similar problem around 1210.. where 1 trans spinning > around on the pending queue could Claim() all the active null transactions > (across multiple iterations) before one actually got put back inthe idle > pool. that would result in extra conns too. wdyt? Yes, that was the second part of my comment #1 and reference to line 1215 :). But that is a bit harder to resolve. I cannot just switch transactions here (I have not looked into details, but it is lot of work and sounds really risky). I had a patch that had a flag in nsHttpTransaction that was true if transaction is waiting for a speculatively started halfOpen or a the case from line 1215, but I am afraid that i will forgot one of the cases where I should change the flag to false and the transaction will wait forever. I just have an idea: I could give each transaction a reference to the HalfOpen sock that it is waiting for. That could work in all cases and it is less risky, I just need to transform nsHalfOpenSocket into RefPtrs (currently it is: nsTArray<nsHalfOpenSocket*> mHalfOpens;). Or better, keep nsHalfOpenSocket internal for nsHttpConnectionMgr and change transaction pending queue..

For uplift, I can make a patch that checks transaction state in nsHttpTransaction. This par alone cannot be resolved in nsHttpConnectionMgr.

This needs to be fix in nsHttpTransaction, because a halfOpen socket with 2 streams and a connection can have a reference to nsHttpTransaction.

Comment on attachment 8839848 [details] [diff] [review] bug_1340164_socket_state_v1.patch Review of attachment 8839848 [details] [diff] [review]: ----------------------------------------------------------------- heh. funny patch :) Thanks. We can uplift this and fix half-opens and idle nulls in a different bug.

I have already open the second bug earlier today - bug 1341572. The patch is almost ready. (working on that other patch I found a different way to resolved this one, with a change in nsHttpConnectionMgr, but all together it is not much different so this one is good too. You will see it in the patch for bug 1341572).

Pushed by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/0c4922134c7f Fix socketTrasport states in nsHttpTransaction coming from multiple sockets. r=mcmanus

Comment on attachment 8839848 [details] [diff] [review] bug_1340164_socket_state_v1.patch Approval Request Comment [Feature/Bug causing the regression]: This is along exiting bug [User impact if declined]: Socket status events are wrong, e.g. there are sent multiple times. This influences network panel, thet timings in network panel are wrong. [Is this code covered by automated tests?]: manually tested, it is not easy to reproduce in a test. [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: not at all [Why is the change risky/not risky?]:this only influence socket status events and they do not influence browser behavior at all. The patch only prevents sending multiple duplicate events [String changes made/needed]:none

Comment on attachment 8839848 [details] [diff] [review] bug_1340164_socket_state_v1.patch More accurate timing sounds good to me. This is a bit last minute for aurora 53, but let's try it.