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?
really hard to believe that this has benign side effects.
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?
bradley: your patch(es) for bug 19073 do not touch FTP... perhaps you meant something else?
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.
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.
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.
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?
Thie breaks the 'loading' thingy in the XUL viewer, since the onstop comes streight away.
And this totally breaks the ftp cache - see bug 102300.
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla0.9.5
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.
Comment on attachment 51420 [details] [diff] [review] proposed fix looks good.
Attachment #51420 - Flags: review+
Comment on attachment 51420 [details] [diff] [review] proposed fix sr=darin
Attachment #51420 - Flags: superreview+
I checked this in last night, but forgot to mark it fixed. Oops.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
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?
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.
pls check it in to the branch - PDT+
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?
mscott: Thanks - I was going to ask someone to do it for me.
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?
+verifyme: I'm ready to mark VERIFIED, any other testing needed?
Looks like bug 102715 referenced in this bug report is also verified. I think we can mark this verified, ben.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.