141796 - Mozilla crashed in http-pipelining code Trunk M1RC2 [@ nsHttpPipeline::GetRequestSize_Locked]

Status: VERIFIED FIXED

(Core :: Networking: HTTP, defect, P1)

Description

[Frankie]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.0.0+)
Gecko/20020502
BuildID:    20020428

Apologies for this horribly vague report. Too bad OS X doesn't have Talkback.
Beats me why it crashed. I had a few tabs open and a page loading. I wasn't
clicking or scrolling the mouse. Crash log follows.

Reproducible: Didn't try

Comment 1

[Frankie]

Attachment: console crash log, 0428 build

Comment 2

[Matthias Versen [:Matti]]

-> Networking:http
(nsHttpPipeline::GetRequestSize_Locked)

Have you enabled Http Pipelining ?

Comment 3

[Darin Fisher]

yup, that code is only executed when pipelining is enabled.

Comment 4

[Frankie]

Yes, I turned on pipelining a few months ago. 

Mozilla's Pipelining FAQ mainly talks about problems faced by the web server. It
doesn't say anything about potential browser crashes. Should it?

Comment 5

[Darin Fisher]

no, clearly you've discovered a mozilla bug.  mozilla should not crash even if
the server is misbehaving.

investigating this one for moz 1.0.1

Comment 6

[greer]

This one has turned up as a topcrasher in M1RC2 with 60+ crashes. Any chance 
for this one before the proposed M1.0.1?

         nsHttpPipeline::GetRequestSize_Locked
[d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHttpPipeline.cpp  line 
681] 
         nsHttpPipeline::OnDataWritable
[d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHttpPipeline.cpp  line 
432] 
         nsHttpConnection::OnDataWritable
[d:\builds\seamonkey\mozilla\netwerk\protocol\http\src\nsHttpConnection.cpp  
line 670] 
         nsSocketWriteRequest::OnWrite
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsSocketTransport.cpp  line 2958] 
         nsSocketTransport::doReadWrite
[d:\builds\seamonkey\mozilla\netwerk\base\src\nsSocketTransport.cpp  line 1165] 

Comment 7

[greer]

Attachment: Alternate Stack

Comment 8

[Darin Fisher]

Attachment: v1 patch

turns out that a transaction can be canceled (and therefore removed from the
pipeline) before the pipeline is even written to the network.  this situation
leads to a crash, because the code that writes the request to the pipeline
assumed all of the transactions would be valid.  so the fix is very trivial...
just some null pointer checks in the right places.

Comment 9

[rpotts (gone)]

Comment on attachment 83655 [details] [diff] [review]
v1 patch

looks good.  sr=[e-mail]

Comment 10

[rpotts (gone)]

btw... can the following assertion also be removed :-)

@@ -370,7 +370,8 @@
     // no need to be inside the lock
     for (PRInt8 i=0; i<mNumTrans; ++i) {
         NS_ASSERTION(mTransactionQ[i], "no transaction");
-        mTransactionQ[i]->SetConnection(this);
+        if (mTransactionQ[i])
+            mTransactionQ[i]->SetConnection(this);
     }
 }

since it is now acceptable for mTransactionQ[i] to be null :-)

thanks,
-- rick

Comment 11

[Darin Fisher]

rick:

actually in that case mTransactionQ[i] should not be null, but i'm adding a
runtime check for added safety.  it should not be null because there is no
opportunity to cancel any of the transactions between nsHttpPipeline::
AppendTransaction and nsHttpPipeline::SetConnection.

Comment 12

[jag (Peter Annema)]

Comment on attachment 83655 [details] [diff] [review]
v1 patch

r=jag

Comment 13

[Darin Fisher]

fixed-on-trunk

Comment 14

[Mike Shaver (:shaver emeritus)]

Comment on attachment 83655 [details] [diff] [review]
v1 patch

a=shaver+scc+jesup for the rc3 branch.	Booyah!

Comment 15

[Darin Fisher]

fixed-on-branch

Comment 16

[Tom Everingham]

last talkback reports of this crash were from build 2002051408 or 6 days ago

verified

Comment 17

[Tom Everingham]

verified1.0.0