87370 - Multipart documents do not always fire the OnStopRequest

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Kevin McCluskey (gone)]

To reproduce:

Go to
http://bugzilla.mozilla.org/query.cgi

enter Mozilla0.9.2 as the target milestone
then submit query.

The nsParser listener's OnStopRequest is called once. It is not called for the 
second document. Tracing into the nsMultiMixedConv implementation the 
mPartChannel is null when the second call to nsMultiMixedConv::OnStopRequest is 
made so it does not pass the OnStopRequest over to the parser.



NS_IMETHODIMP
nsMultiMixedConv::OnStopRequest(nsIRequest *request, nsISupports *ctxt,
                                nsresult aStatus) {

  ...

    if (mPartChannel) {  <== this is null for some reason

Comment 1

[Kevin McCluskey (gone)]

This bug is blocks 76722, because with the patch enabled in bug 76722 the 
throbber will spin forever on Mozilla queries because the OnStopRequest for the 
second document is never called.

Comment 2

[harishd]

This bug also blocks ( I think ) bug 49539.

I think the patch in bug 70398 should do the trick.

Comment 3

[Doug Turner (:dougt)]

Attachment: allows multiple onStopRequests

Comment 4

[Doug Turner (:dougt)]

Attachment: same as above without dos line endings.

Comment 5

[Doug Turner (:dougt)]

scott, this is the patch that harishd and I spoke to you on the phone last week 
about.  It basically removes that Bool that prevents multiple onStopRequests 
from the URILoader.  Can I get a SR from your?

Rick, please review this for a r=?


Kevin, when is this needed by?  0.9.3?

Comment 6

[Kevin McCluskey (gone)]

I need it for 0.9.3 so I can enable the patch in bug 76722 for 0.9.3. 

Comment 7

[Kevin McCluskey (gone)]

I have been running with patch 39919 and it fixes the "throbber never stops
spinning" issue that is blocking 76722

Comment 8

[Doug Turner (:dougt)]

kevin, can you reproduce 84774?

Comment 9

[harishd]

patch, id 39919, looks good to me. r=harishd

Comment 10

[Scott MacGregor]

sr=mscott

Comment 11

[Kevin McCluskey (gone)]

patch 39919 fixes bug 84774

Comment 12

[Doug Turner (:dougt)]

fix checked in:

Checking in nsURILoader.cpp;
/cvsroot/mozilla/uriloader/base/nsURILoader.cpp,v  <--  nsURILoader.cpp
new revision: 1.79; previous revision: 1.78
done

Comment 13

[rpotts (gone)]

hey doug,

your patch introduces a possible re-enterent call of OnStopRequest(...) which
mOnSgtopFired was preventing :-(

I'm attaching a patch which handles this situation.

-- rick

Comment 14

[rpotts (gone)]

Attachment: patch to avoid reenterant calls to OnStopRequest(...)

Comment 15

[Scott MacGregor]

sr=mscott on rick's patch

Comment 16

[rpotts (gone)]

I'm also talking with mscott about the ramifications of removing the early
return from ProcessCanceledCase(...).

Currently, we will *not* call OnStopRequest(...) if the channel ends up being
canceled :-( I believe that this is *very* bad because the consumer expects to
always be called!

Comment 17

[Darin Fisher]

yes, that is really bad!  what can be done about it?

Comment 18

[rpotts (gone)]

your patch 43077 looks okay r=dougt

Comment 19

[rpotts (gone)]

So, after talking it over with mscott, we both agreed that calling
ProcessCanceledCase(..) inside of OnStopRequest(...) was unnecessary/bad.

Since it will still be called in OnStartRequest(...) the situation caused by bug
#40116 (which the code was added for) will still be handled correctly.

So, i'm attaching a new patch (if you liked the last one you'll love this one)
which also removes the call to ProcessCanceledCase(...) in OnStopRequest.

Comment 20

[rpotts (gone)]

Attachment: same as last patch - but removes call to ProcessCanceledCase() from OnStopRequest too...

Comment 21

[rpotts (gone)]

-> rpotts.  since I have the patch :-)

Comment 22

[Doug Turner (:dougt)]

r=dougt

Comment 23

[rpotts (gone)]

I've just checked the patch (40400) into the trunk...  I'm just waiting to check
in into the branch too.

Comment 24

[rpotts (gone)]

I forgot to mention that I have verified that bug #40116 has not been
re-introduced by removing the call to ProcessCanceledCase() from OnStopRequest(...).

I did this by placing a breakpoint in ProcessCanceledCase() and going back and
forth on http://news.bbc.co.uk/ until I hit the breakpoint...  Then, I verified
that we didn't crash :-)

--rick

Comment 25

[Scott MacGregor]

sr=mscott

Comment 26

[rpotts (gone)]

a=choffman

I've just landed both patches (dougt and mine) onto the branch.