take transport needs a couple changes in nshttppipeline to get access to the real nshttpconnection

Comment on attachment 542096 [details] [diff] [review] v1 Review of attachment 542096 [details] [diff] [review]: ----------------------------------------------------------------- I need explanation of the motivation for this change. As I understand (don't say I do correctly) we should not throw mConnection from nsHttpPipeline sooner then nsHttpTransaction throws its mConnection (here the pipeline) away. So, forwarding call to TakeTransport from nsHttpPipeline to nsHttpConnection should always succeed (there will always be nsHttpPipeline->mConnection). nsHttpPipeline::Close, where we throw the real connection away, is never called sooner then Close method of all nested transactions. At least with the T+S patch. As I looked into the code, nsHttpPipeline::Close is called: - when there is a socket error, through nsHttpConnection::CloseTransaction - indirectly from nsHttpChannel::Cancel or nsHttpChannel::OnStopRequest(NS_ERROR*) (through mgr::CancelTransaction) when the channel's nsHttpTransaction is the first request or not a request of the pipe (see nsHttpPipeline::CloseTransaction) I see no potential problem.

(In reply to comment #2) > I see no potential problem. I have somewhat of a hard time articulating it too (but I'll give it my best shot) - but this is not theoretical. The Websocket tests periodically and often fail on larch without this. * first, data reading is driven from nsHttpPipeline::WriteSegments(), which in turn calls nsHttpTransaction::WriteSegments().. this calls nsHttpTransaction::WriteSegment, which in turn calls ProcessData() and that consumes the entire HTTP 101 response. The 101 is a no-content response (content length is implicitly 0). It marks mTransactionDone and mResponseIsComplete true when the headers are all read. * Then the stack pops back up to nsHttpPipeline which checks if (rv == NS_BASE_STREAM_CLOSED || trans->IsDone()) .. rv is NS_OK, but IsDone() is true. * As a result pipeline calls (nsHttpTransaction)->Close() (which keeps ahold of the nshttptransction::mconnection because it is sticky - that is the mechanism used for keeping it around for the taketransport() call later, but it does trigger the OnStopRequest event). After close() returns, it removes the completed transaction from the response queue (mResponseQ.RemoveElementAt(0)) * so far, nshttppipeline::mConnection is still in tact. This is where it gets a little bit fuzzy for me. There might be another call to writesegments here. Remember that the read that consumed the 101 reutuned NS_OK, and also IsDone().. so there has been no indication to stop reading. If that does happen writesegments returns BASE_STREAM CLOSED because the requestq and responseq are empty (see around 513 of m-c). If that does happen it bubbles out to nsHttpConnection::onSocketReadable which in turn causes OnInputStreamReady to call nsHttpConnection::CloseTransaction(), which calls mTransction(i.e. nsHttpPipeline)->Close(), which on line 597 or so releases and nulls mConnection. If the OnStopRequest handler just gets around to calling TakeTransport() at this point the pipeline mConnection is null and stuff obviously breaks. The patch re-associates the transaction with the real http connection, which it started with anyhow, once it is complete so that the TakeTransport() call will work directly with the real connection.

Thanks for this explanation. It makes sense: there are probably more data to be read from the socket, so nsHttpConnection calls on nsHttpPipeline to read from it. But it is 'empty' and vanishes. I tried to change the code not to release mConnection in nsHttpPipeline::Close, but rather in the dtor, that seems to me a better solution also for any further problems with breaking this chain. No leaks with some few tests ran. I would like to give it a try, what do you think?

// we must no longer reference the connection! This needs to come // after we've closed all our transactions, since they might want // connection info as they close. NS_IF_RELEASE(mConnection); What do you think "must no longer refernece the connection" refers to? I feel like I once knew but I can't remember.

(In reply to comment #5) > What do you think "must no longer refernece the connection" refers to? I > feel like I once knew but I can't remember. Good point. It was introduced in bug 176919. I think it is precaution from leaks. But not sure about that. nsHttpPipeline::Close is called only when: - when there is a socket error, through nsHttpConnection::CloseTransaction - indirectly from nsHttpChannel::Cancel or nsHttpChannel::OnStopRequest(NS_ERROR_*) (through mgr::CancelTransaction) when the channel's nsHttpTransaction is the first request only partially sent out or not a request of the pipe at all, probably presuming it was in the past We might release mConnection only when |reason| is a failure. If a transaction on a pipeline is closed with NS_BASE_STREAM_CLOSED then nsHttpConnection::Close masks the result as NS_OK. We probably don't want to keep the connection when the socket has failed.. Just a thought. However, that is not answer to your comment. I will keep figuring this out.

I think it is OK to don't release mConnection. In case of manual cancel (the second case) nsHttpConnection::CloseTransaction, mTransaction = nsnull; line seems to release the last reference to nsHttpPipeline object. In case of the socket error (the first case) it seems to be the same. Only problem I can see may occur when we try to reuse the pipeline by calling AddTransaction on it. We will set it as connection on transactions being added. That might be potentially wrong..

> > Only problem I can see may occur when we try to reuse the pipeline by > calling AddTransaction on it. We will set it as connection on transactions > being added. That might be potentially wrong.. I was just about to type that.. but that can be trivially fixed by checking mclosed.. I'll look at this more tomorrow. It would certainly be a simpler change. (I also sent it to try)

(In reply to comment #8) > I was just about to type that.. but that can be trivially fixed by checking > mclosed.. Exactly. > I'll look at this more tomorrow. It would certainly be a simpler change. (I > also sent it to try) Thank you. I wanted to ask you for this.

(In reply to comment #9) > > > I'll look at this more tomorrow. It would certainly be a simpler change. (I > > also sent it to try) > > Thank you. I wanted to ask you for this. The version I sent to try was the usual set of pipeline patches (that are green) with the take transport patch rewritten to just move the release of mconnection to the pipeline dtor. I didn't deal with addTransaction. It was not good :) http://tbpl.mozilla.org/?tree=Try&rev=ebd0177e4d3b I'll try and figure out why and maybe resubmit a new version that deals with addtransaction.

I might know why, see the following: This is push to m-c with releasing the connection in dtor (no other change): http://tbpl.mozilla.org/?tree=Try&rev=c9b94897fe05 It looks quit green so far, so something in the pipeline series creates the leak. This is a push of removal of the assignment to nsHttpConnection::mPipelineData introduced in bug 603512, pushed over your try push (referenced in comment 10): http://tbpl.mozilla.org/?tree=Try&rev=0e1d1f618353 So far no results, but I'm curious if it will fail or not (maybe I will completely break it with my change!). I'm just reviewing bug 603512, and one of the review requests is removal of that member. If we really need to access the pipeline we can do it much better then by adding a new reference. Review will come soon.

(In reply to comment #11) > I'm just reviewing bug 603512, and one of the review requests is removal of > that member. [mpipelinedata] yes, that's an artifact of a work in progress that accidentally stuck. It does not appear to be used anywhere in the series so it can go away without any other work.

(In reply to comment #12) > yes, that's an artifact of a work in progress that accidentally stuck. It > does not appear to be used anywhere in the series so it can go away without > any other work. That was an incorrect statement on my part. It is used. Some investigation is necessary.

OK, I think mPipelineData is historic only. It dates back to December in my patches and is obviated by a few other patch tweaks that have come since then. At least I think I understand why it was in there now. So pedning testing, that can be tossed away.

So: Moving release of mConnection to nsHttpPipeline dtor on current mozilla-central code: http://tbpl.mozilla.org/?tree=Try&rev=c9b94897fe05 All green, except few known failures, so it definitely looks doable. Moving release of mConnection to nsHttpPipeline dtor AND removing mPipelineData assignment on Larch tip: http://tbpl.mozilla.org/?tree=Try&rev=0e1d1f618353 Mochitest 5 regularly leaks 8 nsHttpConnections objects while not leaking any nsHttpPipelines! Otherwise seems to be green. I think it is reasonable to track that down. Patrick, will you have time to discover what test(s) is(are) causing it? I can otherwise do that and then give you results to check on cause.

(In reply to comment #15) > > Moving release of mConnection to nsHttpPipeline dtor AND removing > mPipelineData assignment on Larch tip: > http://tbpl.mozilla.org/?tree=Try&rev=0e1d1f618353 > Mochitest 5 regularly leaks 8 nsHttpConnections objects while not leaking > any nsHttpPipelines! Otherwise seems to be green. > > I think it is reasonable to track that down. yeah, I'll take a look.

Comment on attachment 542096 [details] [diff] [review] v1 Comment 16 outlines some suggestion for a bit cleaner and more general solution. Dropping the review flag for now.

Comment on attachment 542096 [details] [diff] [review] v1 try-server likes this. It is a new-pipelining pre-req and fixes bugs in the current pipelining code.

Comment on attachment 542096 [details] [diff] [review] v1 disregard comment 18 - I simply made it on the wrong bug.

(In reply to Honza Bambas (:mayhemer) from comment #15) > Mochitest 5 regularly leaks 8 nsHttpConnections objects while not leaking > any nsHttpPipelines! Otherwise seems to be green. > > I think it is reasonable to track that down. > I figured that one out yesterday - it was a bug in the type and state patch that kept an extra refernece on httpconnections for the STICKY path. not relevant to this bug.

This should be all set now, I've fixed the remainder of the tree to work with it. Very simple.

Comment on attachment 581260 [details] [diff] [review] patch v2 Review of attachment 581260 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab. Thanks!