Patrick McManus [:mcmanus] Assignee
	Description • 14 years ago

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.

	Jason Duell
	Comment 1 • 14 years ago

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

	Honza Bambas (:mayhemer)
	Comment 2 • 14 years ago

I can steal a review here...

	Patrick McManus [:mcmanus] Assignee
	Comment 3 • 14 years ago

(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.

	Honza Bambas (:mayhemer)
	Comment 4 • 14 years ago

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.

	Patrick McManus [:mcmanus] Assignee
	Comment 5 • 14 years ago

update of review comments, transferring r+

sending to try server

	Brad Lassey [:blassey] (use needinfo?)
	Comment 6 • 14 years ago

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

	Honza Bambas (:mayhemer)
	Comment 7 • 14 years ago

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

	Patrick McManus [:mcmanus] Assignee
	Comment 8 • 14 years ago

(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.

	Patrick McManus [:mcmanus] Assignee
	Comment 9 • 14 years ago

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

	Patrick McManus [:mcmanus] Assignee
	Comment 11 • 14 years ago

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.

	Honza Bambas (:mayhemer)
	Comment 12 • 14 years ago

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

	Patrick McManus [:mcmanus] Assignee
	Comment 13 • 14 years ago

update bitrot - inherits r+

	Patrick McManus [:mcmanus] Assignee
	Comment 14 • 14 years ago

updated just for bitrot - inherits r+ honzab

	Patrick McManus [:mcmanus] Assignee
	Comment 15 • 14 years ago

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

	Benjamin Stover (:stechz)
	Comment 16 • 14 years ago

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