101128 - ftp sending spurious OnStopRequest before OnStart

Status: VERIFIED FIXED

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

Description

[Bradley Baetz (:bbaetz)]

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

[Darin Fisher]

really hard to believe that this has benign side effects.

Comment 2

[Bradley Baetz (:bbaetz)]

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

[Darin Fisher]

bradley: your patch(es) for bug 19073 do not touch FTP... perhaps you meant
something else?

Comment 4

[Bradley Baetz (:bbaetz)]

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

[Darin Fisher]

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.

Comment 6

[Bradley Baetz (:bbaetz)]

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.

Comment 7

[Bradley Baetz (:bbaetz)]

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?

Comment 8

[Bradley Baetz (:bbaetz)]

Thie breaks the 'loading' thingy in the XUL viewer, since the onstop comes
streight away.

Comment 9

[Bradley Baetz (:bbaetz)]

And this totally breaks the ftp cache - see bug 102300.

Comment 10

[Bradley Baetz (:bbaetz)]

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 11

[Bradley Baetz (:bbaetz)]

Attachment: proposed fix

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 51420 [details] [diff] [review]
proposed fix

looks good.

Comment 13

[Darin Fisher]

Comment on attachment 51420 [details] [diff] [review]
proposed fix

sr=darin

Comment 14

[Bradley Baetz (:bbaetz)]

I checked this in last night, but forgot to mark it fixed. Oops.

Comment 15

[Scott MacGregor]

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? 

Comment 16

[Bradley Baetz (:bbaetz)]

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 17

[Jaime Rodriguez, Jr.]

pls check it in to the branch - PDT+

Comment 18

[Scott MacGregor]

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?

Comment 19

[Bradley Baetz (:bbaetz)]

mscott: Thanks - I was going to ask someone to do it for me.

Comment 20

[benc]

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

[benc]

+verifyme: I'm ready to mark VERIFIED, any other testing needed?

Comment 22

[lchiang]

Looks like bug 102715 referenced in this bug report is also verified.  I think
we can mark this verified, ben.

Comment 23

[benc]

VERIFIED.