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)
Tracking
()
RESOLVED
FIXED
People
(Reporter: antonio.xu, Assigned: antonio.xu)
References
()
Details
Attachments
(1 file)
992 bytes,
patch
|
john
:
review+
darin.moz
:
superreview+
chofmann
:
approval+
|
Details | Diff | Splinter Review |
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•22 years ago
|
||
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•22 years ago
|
||
Is there also an issue with ftp? Please test your patch against publishing in Composer.
Comment 3•22 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•22 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•22 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•22 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•22 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•22 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•22 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•22 years ago
|
||
checked in
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Comment 11•22 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•22 years ago
|
||
checked into MOZILLA_1_0_BRANCH
You need to log in
before you can comment on or make changes to this bug.
Description
•