70147 - nsMultiMixedConv::OnStopRequest is not being fired after hitting the stop button.

Status: VERIFIED WORKSFORME

(Core :: Networking, defect)

Description

[Judson Valeski]

1. run a bugzilla query.
2. while the query is in progress, hit the stop or reload button.
3. do 1 and 2 a couple of times and you'll either crash or you'll get corrupted
data in the window.

This is because the stop or reload operation is not propegating the
OnStopRequest through to the nsMultiMixedConv::OnStopRequest() callback.
Somewhere in the chain of callbacks, the multimixec converter is getting lost,
leading to the docshell getting confused because it gets a new request before
the previous request was cleaned up (via OnStops) being fired.

Comment 1

[Doug Turner (:dougt)]

It looks like nsDocumentOpenInfo::OnStopRequest is called.  The request is 
checked if it was canceled.  ProcessCanceledCase does return true.  This 
basically cause the OnStopRequest never to fire for the stream converter.  Are 
clients in general not getting OnStopRequest for cancelled requests?  This seams 
very broken.

Comment 2

[Doug Turner (:dougt)]

ccing folks.

Comment 3

[Doug Turner (:dougt)]

Basic stuff that is happening:

AsyncOpen on the http channel is called.  That creates a nsHTTPFinalListener 
who's mListener is the nsDocumentOpenInfo.  OnStart is called and propogates 
through the nsHTTPFinalListener to the nsDocumentOpenInfo.  The 
nsDocumentOpenInfo DispatchContent is called then we retarget the output. 
RetargetOutput sets nsHTTPFinalListener's m_targetStreamListener.  When 
the cancel button is depressed, the nsHTTPFinalListener OnStopRequest calls to 
the nsDocumentOpenInfo's OnStop and that prevents the OnStopRequest from ever 
getting to the m_targetStreamListener.

Looking at the blame of:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/uriloader/base/nsURILoader.c
pp

It looks like not propgating the canceled onStop is intented:

1.49 <[e-mail]> 16 Jun 2000 14:21
 Bug #40116 --> check to see if the channel was canceled before
 propogating calls to the content listener...
 r=valeski

Comment 4

[Judson Valeski]

ugh, we we still have an impedence missmatch between the necko architecture
(life of a "request" is dictated by an OnStart/OnStop pair), and the way
consumers want to define "stop" or "cancel." It's sounding like the front end
wants to use the fact that the stop button was depressed as the signal that
things are cancelled, and that it doesn't want to handle OnStops() that come up
from necko after things are cancelled.

IMO, (I said this over 2 years ago :-)) consumers should conform to the OnStop()
coming up from necko in a cancel case. not my call though, someone else probably
has a better idea of how hard that would be.

stream converters are clearly being considered by the uriloader as general
consumers. that's just fine except in for the way we shield consumers from
cancellation generated OnStop()s. stream converters need the OnStop (not just
multimixed).

the alternative to making consumers conform to OnStop()s seems to be to move the
stream converter layer somewhere else. that move would disrupt unknown content
type handling (byte scanning) which is tightly integrated to the uriloader. I
personally like the stream converter where it is, in the uriloader, and believe
the path of least resistance (perhaps founded on lack of knowledge) is to make
consumers fall in line w/ cancellation onstop()s.

Couldn't consumers who don't want the OnStop() propegated just stick a bool in
their OnStop() to skip the cancellation based onstop?

consumerFoo::OnStopRequest(...) {
 // I'm a consumer that doesn't like to listen to OnStops
 // generated as a result of a cancellation or reload.
 if (!UI-based-cancel) {
  //  this OnStopRequest was generated by either an
  // error in the request, or the transaction has
  // successfully completed.
  ...
  // go on about our business, cleaning up whatever we need to cleanup, etc.
 } else {
  // this onstop was generated as a result of some high level user action
  // ignore it.
  ...
  // do nothing.
 }
 ...
}

UI-based-cancel wouldn't even have to be an explicit flag. presumably a UI based
cancel (stop button or reload button) just clears state in the consumer. in that
case, the cleared state (a nulled pointer for example) can just be checked.

if (i've-already-cleared-my-state) don't do squat.

I know we've brought this up before, and there's a reason we're not propegating
those stops, but perhaps we should try again?

Comment 5

[R.K.Aa.]

Similarly, bad things also happens when clicking a link before a page is loaded.
Moz freeze here 10-20 times a day because of this bug, and has to be killed.
It's becoming a chore to use it. I filed bug 68483 about it all, in less
technical terms. Perhaps mark that dup of this one.

Comment 6

[Judson Valeski]

0.9 nomination.

Comment 7

[Doug Turner (:dougt)]

who is the owner of the uriloader? 

Comment 8

[Gagan]

mscott?

Comment 9

[Cathleen]

guys, can we get some traction on this bug??
we think this is another one of the bugs causing infinite throber...

Comment 10

[Doug Turner (:dougt)]

mscott is more doomed then me.  reassign to myself.  I will take a look for 
0.9.1

Comment 11

[scottputterman]

reassigning to dougt based on previous comments.

Comment 12

[Doug Turner (:dougt)]

This doesn't happen any more...  what changed???

I can't get it to crash or display corrupt data.

Comment 13

[benc]

found while looking for more necko/UI testcases...
VERIFIED/wfm, per dougt + passage of time.