Multipart documents do not always fire the OnStopRequest

RESOLVED FIXED in mozilla0.9.3

Status

()

Core
Networking
P1
blocker
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: Kevin McCluskey (gone), Assigned: rpotts (gone))

Tracking

Trunk
mozilla0.9.3
x86
Windows NT
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: patch ready, need a=, URL)

Attachments

(4 attachments)

(Reporter)

Description

17 years ago
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
(Reporter)

Updated

17 years ago
Severity: normal → blocker
(Reporter)

Comment 1

17 years ago
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.
Blocks: 76722

Comment 2

17 years ago
This bug also blocks ( I think ) bug 49539.

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

Comment 3

17 years ago
Created attachment 39915 [details] [diff] [review]
allows multiple onStopRequests

Comment 4

17 years ago
Created attachment 39919 [details] [diff] [review]
same as above without dos line endings.

Comment 5

17 years ago
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?
Status: NEW → ASSIGNED
(Reporter)

Comment 6

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

Updated

17 years ago
Whiteboard: patch ready, need r=,sr=,a=
Target Milestone: --- → mozilla0.9.3

Updated

17 years ago
Priority: -- → P1
(Reporter)

Comment 7

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

Comment 8

17 years ago
kevin, can you reproduce 84774?

Comment 9

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

Comment 10

17 years ago
sr=mscott
(Reporter)

Comment 11

17 years ago
patch 39919 fixes bug 84774

Comment 12

17 years ago
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
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
(Assignee)

Comment 13

17 years ago
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 14

17 years ago
Created attachment 40337 [details] [diff] [review]
patch to avoid reenterant calls to OnStopRequest(...)

Comment 15

17 years ago
sr=mscott on rick's patch
(Assignee)

Comment 16

17 years ago
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

17 years ago
yes, that is really bad!  what can be done about it?
(Assignee)

Comment 18

17 years ago
your patch 43077 looks okay r=dougt
(Assignee)

Comment 19

17 years ago
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.
(Assignee)

Comment 20

17 years ago
Created attachment 40400 [details] [diff] [review]
same as last patch - but removes call to ProcessCanceledCase() from OnStopRequest too...
(Assignee)

Comment 21

17 years ago
-> rpotts.  since I have the patch :-)
Assignee: dougt → rpotts
Status: REOPENED → NEW

Comment 22

17 years ago
r=dougt
(Assignee)

Comment 23

17 years ago
I've just checked the patch (40400) into the trunk...  I'm just waiting to check
in into the branch too.
Whiteboard: patch ready, need r=,sr=,a= → patch ready, need a=
(Assignee)

Comment 24

17 years ago
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

17 years ago
sr=mscott
(Assignee)

Updated

17 years ago
Blocks: 87739
(Assignee)

Comment 26

17 years ago
a=choffman

I've just landed both patches (dougt and mine) onto the branch.
Status: NEW → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.