Closed
Bug 101128
Opened 23 years ago
Closed 23 years ago
ftp sending spurious OnStopRequest before OnStart
Categories
(Core Graveyard :: Networking: FTP, defect, P1)
Tracking
(Not tracked)
VERIFIED
FIXED
mozilla0.9.5
People
(Reporter: bbaetz, Assigned: bbaetz)
References
Details
(Keywords: verifyme, Whiteboard: [PDT+])
Attachments
(1 file)
3.30 KB,
patch
|
dougt
:
review+
darin.moz
:
superreview+
|
Details | Diff | Splinter Review |
I noticed that TestProtocols is now broken for ftp urls. Whats happening is that with dougt's fix for bug 92582, we send PASV again. This causes the first connection to be dropped by the server, so the socketTransport ends up with PR_POLL_ERR set. This somehow causes us to end up in DataRequestForwarder::OnStopRequest, which then talks to our listener, which is the channel. In TestProtocols, this causes our outstanding request count to go to 0, so we exit. In mozilla, you can see the "failed to load url" text on the console, and everything somehow keeps working (I'm a bit surprised at that, BTW) So: a) Why are we getting the OnStop? A stack trace shows that its been proxied from somewhere - the socket transport thread, maybe? b) How do we fix it, without assuming that the extra connection will be closed?
Comment 1•23 years ago
|
||
really hard to believe that this has benign side effects.
Assignee | ||
Comment 2•23 years ago
|
||
Yeah, this is bad. mStatus is set in the first onStop, so when the real OnStart comes along, we get the wrong status. This breaks the fix for bug 19073. Does DataRequestForwarder::DelayedOnStartRequest need to get the status from somewhere? If so, where? dougt: would my patch for bug 19073 remove the need for delaying the onstart?
Comment 3•23 years ago
|
||
bradley: your patch(es) for bug 19073 do not touch FTP... perhaps you meant something else?
Assignee | ||
Comment 4•23 years ago
|
||
darin: No. The 2nd patch in 19073 checks the status of the channel. If we've already gone through onstop before onstart, then this will be the bogus error from the original data channel, and so we won't create a new document, and the new content won't be displayed.
Comment 5•23 years ago
|
||
ic... but, i don't believe that your second patch will fly... like i said before, it likely blocks code in webshell from learning of UNKNOWN_HOST and other errors. but, again... i'm not sure... mscott or rpotts would be able to clear up the connections between the uri loader and docshell better than me.
Assignee | ||
Comment 6•23 years ago
|
||
darin: no, my patch changes onstart, and the onstop still gets fired. Since the error dialogs are fired from the onstop... We shoud take this discussion to the other bug.
Assignee | ||
Comment 7•23 years ago
|
||
Actually, after thinking about this a bit more, I'm not sure how it all works :) Without my changes, how does http://foo.bar/ not go to a new page?
Assignee | ||
Comment 8•23 years ago
|
||
Thie breaks the 'loading' thingy in the XUL viewer, since the onstop comes streight away.
Assignee | ||
Comment 9•23 years ago
|
||
And this totally breaks the ftp cache - see bug 102300.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla0.9.5
Assignee | ||
Comment 10•23 years ago
|
||
OK, I have a patch. This is still broken if we get the onstart for the second PASV connection before we get the onstop for the first. I couldn't work out how to tell the sockettransport to go away and never bother me again, or to get it to disconnect now, and send the OnStop. The ftp servers I tested disconnect the first connection when we send PASV, but theres no reason that they have to, AFAICS. Its certainly better than what we have now, though.
Assignee | ||
Comment 11•23 years ago
|
||
Comment 12•23 years ago
|
||
Comment on attachment 51420 [details] [diff] [review] proposed fix looks good.
Attachment #51420 -
Flags: review+
Comment 13•23 years ago
|
||
Comment on attachment 51420 [details] [diff] [review] proposed fix sr=darin
Attachment #51420 -
Flags: superreview+
Assignee | ||
Comment 14•23 years ago
|
||
I checked this in last night, but forgot to mark it fixed. Oops.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 15•23 years ago
|
||
Looks like this fix also fixed 102715 (ability to send ftp pages). I'd like to try to get this fix on the branch. Darin/Bradley, can you guys tell me what you think the risk analysis of your fix is? Is it pretty low or moderate?
Assignee | ||
Comment 16•23 years ago
|
||
I'd say its safe. Theres a very small possiblity that this won't fix all cases, but the cases it doesn't fix (if any) won't display listings anyway. And as a bonus, this fixes the ftp cache so that it works. Of course, I don't know how much time you have left on the branch.
Comment 18•23 years ago
|
||
I just checked this into the 094 branch. I hope you don't mind me doing that Bradley. thanks for the help everyone! Benc, can you reverify this fix in tomorrow's branch builds?
Keywords: nsbranch+
Assignee | ||
Comment 19•23 years ago
|
||
mscott: Thanks - I was going to ask someone to do it for me.
Comment 20•23 years ago
|
||
I ran a brief ftp functional and went through dougs list on the branch and the trunk. branch: by type: ftp.cohesive.com - "Cannot open passive connection: Permission denied." ftp.cea.fr - refused anonymous passwd b/c it is not real email. ftp.jgaa.com - throbber forever. ftp.asus.com - "Cannot open connection" on first try. a couple other sites refused email, but we know the cause of that. trunk: same, generally seemed faster. No connectivity differences. the trunk gives that annoying about:blank result to any error state or dialog. Is this what you are looking for?
Comment 21•23 years ago
|
||
+verifyme: I'm ready to mark VERIFIED, any other testing needed?
Keywords: verifyme
Comment 22•23 years ago
|
||
Looks like bug 102715 referenced in this bug report is also verified. I think we can mark this verified, ben.
Updated•2 months ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•