ftp sending spurious OnStopRequest before OnStart

VERIFIED FIXED in mozilla0.9.5



17 years ago
17 years ago


(Reporter: bbaetz, Assigned: bbaetz)




Firefox Tracking Flags

(Not tracked)


(Whiteboard: [PDT+])


(1 attachment)



17 years ago
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

17 years ago
really hard to believe that this has benign side effects.

Comment 2

17 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

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

Comment 4

17 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

17 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.

Comment 6

17 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.

Comment 7

17 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?

Comment 8

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


17 years ago
Blocks: 102300

Comment 9

17 years ago
And this totally breaks the ftp cache - see bug 102300.
Priority: -- → P1
Target Milestone: --- → mozilla0.9.5

Comment 10

17 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.

Comment 11

17 years ago
Created attachment 51420 [details] [diff] [review]
proposed fix

Comment 12

17 years ago
Comment on attachment 51420 [details] [diff] [review]
proposed fix

looks good.
Attachment #51420 - Flags: review+

Comment 13

17 years ago
Comment on attachment 51420 [details] [diff] [review]
proposed fix

Attachment #51420 - Flags: superreview+

Comment 14

17 years ago
I checked this in last night, but forgot to mark it fixed. Oops.
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 15

17 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? 

Comment 16

17 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 17

17 years ago
pls check it in to the branch - PDT+
Whiteboard: [PDT+]

Comment 18

17 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+

Comment 19

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

Comment 20

17 years ago
I ran a brief ftp functional and went through dougs list on the branch and the


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.

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

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

Comment 22

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

Comment 23

17 years ago
You need to log in before you can comment on or make changes to this bug.