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

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: Antonio.Xu, Assigned: Antonio.Xu)

Tracking

Trunk
x86
Windows 2000
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment)

992 bytes, patch
John Keiser (jkeiser)
: review+
Darin Fisher
: superreview+
chris hofmann
: approval+
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

16 years ago
Created attachment 87920 [details] [diff] [review]
new patch for this bug,please rv & sr

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

Comment 2

16 years ago
Is there also an issue with ftp?  

Please test your patch against publishing in Composer.

Comment 3

16 years ago
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+
(Assignee)

Comment 4

16 years ago
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.

Comment 5

16 years ago
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.

Comment 6

16 years ago
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.

Comment 7

16 years ago
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 8

16 years ago
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 9

16 years ago
Comment on attachment 87920 [details] [diff] [review]
new patch for this bug,please rv & sr

r=jkeiser
Attachment #87920 - Flags: review+

Comment 10

16 years ago
checked in
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 11

16 years ago
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+

Comment 12

16 years ago
checked into MOZILLA_1_0_BRANCH
You need to log in before you can comment on or make changes to this bug.