Closed Bug 101128 Opened 23 years ago Closed 23 years ago

ftp sending spurious OnStopRequest before OnStart

Categories

(Core Graveyard :: Networking: FTP, defect, P1)

x86
Linux
defect

Tracking

(Not tracked)

VERIFIED FIXED
mozilla0.9.5

People

(Reporter: bbaetz, Assigned: bbaetz)

References

Details

(Keywords: verifyme, Whiteboard: [PDT+])

Attachments

(1 file)

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.
Blocks: 102300
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.
Attached patch proposed fixSplinter Review
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
Closed: 23 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+
Whiteboard: [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?
Keywords: nsbranch+
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?
Keywords: verifyme
Looks like bug 102715 referenced in this bug report is also verified.  I think
we can mark this verified, ben.

VERIFIED.
Status: RESOLVED → VERIFIED
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: