Closed Bug 152279 Opened 22 years ago Closed 22 years ago

Transport shouldn't fire status and progress after stop button been clicked for stopping a document load

Categories

(Core :: Networking, defect)

x86
Windows 2000
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: antonio.xu, Assigned: antonio.xu)

References

()

Details

Attachments

(1 file)

From Sun Browser team:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:0.9.9+)
BuildID: Mozilla 1.1 Alpha 

I found sometimes SocketTransport still fire status and progress after stop 
button been clicked for stopping a document load. It will leave over wrong 
status and progress after document done.I had researched it.  I found when stop 
loading document when click stop button. Sometime socket thread will still fire 
status and progress after loading been stoped . I think it is due to mozilla 
cancel channel later than stop sockettransport, so sockerransport will still 
fire status and progress after cancel channel until it is stopped.

Reproducible: sometime
Steps to Reproduce:
1. Go to the URL listed.
2. click stop button when image loading halflings
3. Observe the status bar.

if you can not reproduce it, please try to reload+shift and click stop button.
times<=15

Actual Results:  "Transferring date from..." remains in status bar.

Expected Results:  Status bar text should change to "Document: Stop".
I had researched this bug.  I found sometime sockettranport still fire status
and progress to nsHttpChannel after cancel httpchannel, i think it is due to
sockettransport stop later than document done.	When document done it will fire
status "Document stop" , but at the same time, SocketTransport haven't been
stopped, so it still fire status.  My fix is when httpchannel have been stopped
, it should suppress any status and progress from sockettransport. please rv &
sr my patch. Thank you
Is there also an issue with ftp?  

Please test your patch against publishing in Composer.
Comment on attachment 87920 [details] [diff] [review]
new patch for this bug,please rv & sr

>Index: nsHttpChannel.cpp

>+    if (mProgressSink && !mCanceled)
>         mProgressSink->OnStatus(this, mListenerContext, status, statusText);

this patch makes sense, but i think it'd be better to check mIsPending instead
of mCanceled.  mIsPending is by definition TRUE between AsyncOpen and
OnStopRequest,
which is exactly the interval in which nsIProgressEventSink notifications make
sense.
Attachment #87920 - Flags: needs-work+
Sorry, I can not give up my ideal.  Because if we use mIsPending instead of 
mCanceled, it will make channel to suppress status and progress from socket 
after onstoprequest run, so I think it is too late.  I have changed patch and 
test it according to Darin's advice, but httpchannel still can not suppress the 
status and progress from sockettransport.  I found channel be stopped by 
socketrequest::onstop, so using mIsPending is too late. I have to insist on my 
opinion.
that's very interesting... i wonder why that is necessary.  why is it that
progress cannot be sent out after a consumer cancel's a channel?  that seems
contrary to the interface as it was designed.
It sounded to me like the load group was stopped but the image was still sending
data.  According to Antonio the stop signal was sent synchronously before the
asynchronous network loader had a chance to receive the event and really stop. 
That is a very real problem--it should wait for the loader to stop.

In other words, we should not be passing data along at all after the load has
been cancelled.
hmm... yeah, on second thought anto's patch does make sense.  we suppress
OnDataAvailable events when mCanceled is TRUE, so it makes sense to do the same
with status/progress events.
Comment on attachment 87920 [details] [diff] [review]
new patch for this bug,please rv & sr

sr=darin (nice work!)
Attachment #87920 - Flags: needs-work+ → superreview+
Comment on attachment 87920 [details] [diff] [review]
new patch for this bug,please rv & sr

r=jkeiser
Attachment #87920 - Flags: review+
checked in
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment on attachment 87920 [details] [diff] [review]
new patch for this bug,please rv & sr

a=chofmann for 1.0.1. add the fixed1.0.1 keyword after checking in.
Attachment #87920 - Flags: approval+
checked into MOZILLA_1_0_BRANCH
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: