While putting some new pipeline changes through their paces in my sandbox, I found this existing crash bug on the trunk and release branches if you use pipelining (e.g. mobile): nsHttpConnectionMgr was doing: // check if the transaction already has a sticky reference to a connection. // if so, then we can just use it directly. XXX check if alive?? // XXX add a TakeConnection method or something to make this clearer! nsConnectionHandle *handle = (nsConnectionHandle *) trans->Connection(); The problem is that trans->Connection() returns a nsAHttpConnection which might actually be a nsHttpPipeline or nsConnectionHandle concrete object - casting it always to be a handle causes corruptions when it is really a pipeline. The fix implements the TakeConnection() nsAHttpConnection method the old comment envisions.

Might be fennec blocker? I can move review to top of my plate.

I can steal a review here...

(In reply to comment #2) > I can steal a review here... I would appreciate it. I must admit I find bugzilla more confusing than the code base.

Comment on attachment 474178 [details] [diff] [review] patch implements virtual takeconnection method that does the right thingin the presence of a pipeline object Stolen review from Jason. >diff --git a/netwerk/protocol/http/nsAHttpConnection.h b/netwerk/protocol/http/nsAHttpConnection.h >-class nsHttpConnectionInfo; >+ >+#include "nsHttpConnection.h" What about to just declare nsHttpConnection class? No need to include the whole header here, I think. >+ // return our connection object to the caller and clear it internally >+ // do not drop our reference - the caller now owns it. >+ virtual nsHttpConnection *TakeHttpConnection() = 0; I don't think "do not drop our reference - the caller now owns it." comment has much meaning. Or instead of "our reference" wanted to say "connection reference"? >diff --git a/netwerk/protocol/http/nsHttpConnectionMgr.cpp b/netwerk/protocol/http/nsHttpConnectionMgr.cpp >@@ -773,32 +773,27 @@ nsHttpConnectionMgr::ProcessNewTransacti >+ // check if the transaction already has a sticky reference to a connection. >+ // if so, then we can just use it directly by transferring its reference >+ // to us Who are "us" here? If I understand, maybe rather say "transfer the reference to a local variable"? >- NS_ASSERTION(handle->mConn, "no connection"); Please move this assertion to nsConnectionHandle::TakeHttpConnection(). >+nsHttpConnectionMgr::nsConnectionHandle::TakeHttpConnection() >+ nsHttpConnection *rv = mConn; Please rename rv to conn. >diff --git a/netwerk/protocol/http/nsHttpPipeline.cpp b/netwerk/protocol/http/nsHttpPipeline.cpp >+nsHttpPipeline::TakeHttpConnection() >+{ >+ return NULL; nsnull here? Otherwise OK.

update of review comments, transferring r+ sending to try server

I can a+ this patch if you attach a test case

Just for info, I don't think we have an automated infrastructure that will allow test of HTTP pipelining.

(In reply to comment #7) > Just for info, I don't think we have an automated infrastructure that will > allow test of HTTP pipelining. that's my understanding too - though I am committed to adding one as part of the bigger pipelining project in the longer run. meanwhile, upon further review I don't think anymore that you need this change in fennec right now. The patch is correct and fixes a real problem with the datastructure definitions, but the crash I was seeing on the DoAuthRetry() path cannot happen in moz-central because it magically knows to protect itsself via (&= ~NS_HTTP_ALLOW_PIPELINING) before calling initiatetransaction().. on my branch that cap will prevent actual HTTP pipelining, but due to some consolidation it will not prevent all nsHttpPipeline allocations as it does on moz-central. (the datastructure is useful if you want to pipeline things subsequent to this transaction). I will roll this change into 447866 and close this bug when I update that patch.

Reopening this per: https://bugzilla.mozilla.org/show_bug.cgi?id=447866#c27

updated patch to fix two more instances of the type of problem, where nsHttpConnection code was just casting nsAHttpTransaction objects to nsHttpTransaction because it arbitrarily knew it to be correct. There really doesn't seem to be any point in having the abstract class if you're just going to cast it away when you're not even in the heirarchy. So this patch now provides methods on the base object for resolving what you need without the cast.

Comment on attachment 480619 [details] [diff] [review] Remove API-obscuring static casts Patrick, thanks for splitting these patches. It makes reviews much simpler. > nsHttpConnection::GetSecurityInfo(nsISupports **secinfo) >- // NOTE: this cast is valid since this connection cannot be processing a >- // transaction pipeline until after the first HTTP/1.1 response. >- nsHttpTransaction *trans = static_cast<nsHttpTransaction *>(mTransaction); >- >- val = trans->RequestHead()->PeekHeader(nsHttp::Host); >+ val = mTransaction->RequestHead()->PeekHeader(nsHttp::Host); We are protected against crashes here by a https proxy connection logic. See bug 422016 and the attached fix. However, this may change as part of your work. I believe we will discuss this later yet. This is a good patch for now. r=honzab

update bitrot - inherits r+

updated just for bitrot - inherits r+ honzab

pushed to cedar http://hg.mozilla.org/projects/cedar/rev/233dde6c5b44

Pushed to m-c http://hg.mozilla.org/mozilla-central/rev/233dde6c5b44