Closed Bug 595316 Opened 9 years ago Closed 9 years ago

Crash in HTTP Pipelines - nsHttpConnectionMgr.cpp 697

Categories

(Core :: Networking: HTTP, defect)

1.9.2 Branch
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mcmanus, Assigned: mcmanus)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Attachment #474178 - Flags: review?(jduell.mcbugs)
Blocks: 447866
Might be fennec blocker?  I can move review to top of my plate.
tracking-fennec: --- → ?
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.
Attachment #474178 - Flags: review?(jduell.mcbugs) → review+
update of review comments, transferring r+

sending to try server
Assignee: nobody → mcmanus
Attachment #474178 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #476541 - Flags: review+
tracking-fennec: ? → 2.0b1+
I can a+ this patch if you attach a test case
tracking-fennec: 2.0b1+ → 2.0b2+
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.
tracking-fennec: 2.0b2+ → ---
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
Attachment #480619 - Flags: review?(unusualtears)
Attachment #480619 - Flags: review?(unusualtears) → review?(honzab.moz)
Attachment #476541 - Attachment is obsolete: true
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.
Status: REOPENED → ASSIGNED
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
Attachment #480619 - Flags: review?(honzab.moz) → review+
Blocks: 603503
update bitrot - inherits r+
Attachment #480619 - Attachment is obsolete: true
Depends on: 623948
updated just for bitrot - inherits r+ honzab
Attachment #495132 - Attachment is obsolete: true
Status: ASSIGNED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.