194402 - publishing doesn't complete; opens multiple FTP control connetctions

Status: VERIFIED FIXED

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

Description

[Chris Petersen]

Build: 2003-02-20-03(OS X Mach-0), 2003-02-20-04 (Win32)
Platform: All
Expected Results: The publishing process should finish the transfer of image amd
html data.
What I got: During the transfer of images to the server, the Composer
application crashes.

Steps to reproduce:

1) Go to www.apple.com
2) Select Edit page from File menu
3) In Composer, select Publish as
4) In the Settings tab, type a ftp address in the Publishing address field
5) Type a valid user name and password
6) In the Publish tab, type file name with a .html. Click Publish.
7) The file transfer should begin. After a dozen or so images have been
transfered, the application crashes.

Note: This doesn't happen at every site. I can't reproduce , for example,  with
google.com or a simple HTML page created in Composer. It could be related to
number on elements on the page perhaps.

Comment 1

[Chris Petersen]

Also, this problem occurs in the 7.02 (OS X and Linux) builds.

Comment 2

[Chris Petersen]

Attachment: Stack trace from crash reporter

Comment 3

[Chris Petersen]

Attachment: Talkback info under Linux from Sarah L

Comment 4

[sairuh (rarely reading bugmail)]

brade suggests assigning to darin...

Comment 5

[Darin Fisher]

 #0   0x00000000 in 0x0
 #1   0x0087a0c8 in nsSocketTransportService::RememberHost(nsACString const&,
unsigned short, PRIPv6Addr*)

ooh... this is a duplicate of bug 192207.

*** This bug has been marked as a duplicate of 192207 ***

Comment 6

[Chris Petersen]

Attachment: A reduced test case of apple's site

Attempting to publish this document results in a crash.

Comment 7

[Darin Fisher]

interesting, publishing this page seems to cause us to create an endless number
of FTP connections.  as a result we hit the hard limit on the number of allowed
socket connections.  bug 192207 is about the fact that we crash when we reach
that hard limit.  with my patch for bug 192207 in my tree, we no longer crash. 
however, publishing still fails.

i'm going to reopen this bug to track the publishing failure.  i'm removing the
crash keyword and updating the summary to reflect the real problem.

Comment 8

[Darin Fisher]

ok, the problem is that we don't do anything to gate or limit the number of
simultaneous FTP connections.  netstat indicates that a bunch of control and
data connections have been opened.  it looks like the old socket transport
service had some code to make things continue to work even when the hard limit
on the number of sockets is reached, but there was a big XXX comment indicating
that this condition was not fully implemented.  perhaps it just barely worked
before.  ultimately, i think we need the socket transport to recover gracefully
when the hard limit on the number of sockets is reached, and maybe we need FTP
to limit how many sockets it tries to open.

Comment 9

[Bradley Baetz (:bbaetz)]

ftp should be being limited by the per-host limit in teh socket transport
service, shouldn't it?

Comment 10

[Doug Turner (:dougt)]

both.

we should limit the control to 4-5 per site and a total number of controls to
12-15.  In prefs, of course.

Comment 11

[Kathleen :Brade]

I'm grasping at straws here...

What I don't fully understand at the moment is that the persist code is supposed
to be attempting to serialize publishing of multiple files.  Composer appears to
be setting the flag for non-local files that are published and I don't see any
changes in the past few weeks in the persist code in that vicinity.  Was it
working based on some old assumption for how necko worked?  

Long ago, I did setup my own (linux) ftp server and limit the number of
connections that could be made to 1 and I was able to successfully publish an
html file with several images.

Why are the connections not closing?  Perhaps I have the pref set for immediate
timeout?

Comment 12

[Darin Fisher]

-> 1.4

Comment 13

[benc]

brade: I'm going through the current ftp bugs, and I don't see any reports of
excessive ftp control connections from downloading, so perhaps it has only to do
with uploading? If you use different functions than downloading, I could see how
the connection management logic might break somehow.

Any idea when this regressed?

Comment 14

[Michael Buckland]

Discussed in edt.  Minusing.  Not a currrent concern for our embedding customers.

Comment 15

[Darin Fisher]

Attachment: v1 patch

this patch cleans up a lot of stuff.

1- socket transport now asks the socket transport service (STS) if it is ok to
"attach a new socket".	if the STS says yes, then the socket transport goes
ahead and does its normal stuff.  else, the socket transport asks the STS to
notify it when it can attach a new socket.  this allows us to limit the number
of sockets allocated by mozilla, which is a requirement on some platforms.

2- now, to play nicely with this, FTP also has to limit the number of control
connections it caches.	so, i made nsFtpProtocolHandler::mRootConnectionList
hold no more than 8 connections.  the list is ordered by age, so i just trim
from the head of the list.  well, actually... i look for the eldest connection
to the same host and trim that first.  if no matches, then i trim the eldest
overall.

3- finally, when publishing we don't close the data connection until the
application shuts down.  i fixed this by ensuring that mDPipe is closed when
the channel completes.	so, just like we have the channel telling the state
"DataConnectionEstablished" ... we now have the channel telling the state
"DataConnectionComplete".  i tried to avoid this explicit function call, but i
couldn't come up with anything nearly as clean and simple.

4- finally, i touched up a few things in the async stream copier... mainly
cosmetic.

Comment 16

[Doug Turner (:dougt)]

Comment on attachment 119315 [details] [diff] [review]
v1 patch

I don't understand this comment before or after this change.  Maybe we can
explain why we are cheating?

-    // we'll cheat a little bit here since we know that NS_AsyncCopy does pass
-    // an event queue.
+    // we'll cheat a little bit here since we know that NS_AsyncCopy does not
+    // pass an event queue.



I don't see the nsISocketEventHandler interface file.  

You may want to reorder these so that the more common interface is the first in
the list:

+NS_IMPL_THREADSAFE_ISUPPORTS4(nsSocketTransport,
+			       nsISocketEventHandler,
			       nsISocketTransport,
			       nsITransport,
			       nsIDNSListener)


don't you have a race in InitiateSocket?  Suppose the limit is reached on the
number of sockets created.  A new request comes in, and you return with
|notifyWhenCatAttachSocket|.  Sometime later a socket becomes available and
before the MSG_RETRY_INIT_SOCKET event is handled (but is already has been
posted), a different socket calls CanAttachSocket()


Jud made me to it, i swear! :

-	     (void) mFTPState->DataConnectionEstablished();
+	     mFTPState->DataConnectionEstablished();

I am worried about cycles  with the new ftp handler global.  Can you make that
we do clean up properly.

Extra points for making any references to "ftp" consistent wrt case. :-)

Comment 17

[Darin Fisher]

>I don't understand this comment before or after this change.  Maybe we can
>explain why we are cheating?

because we ignore the "eventQ" parameter (assuming it will always be null). 
i'll add a comment to that effect.


>I don't see the nsISocketEventHandler interface file.

it is included at the bottom of nsISocketTransportService.idl


>You may want to reorder these so that the more common interface is the first in
>the list:

i chose that ordering based on the fact that nsISocketEventHandler would
probably be the most commonly QI'd interface (given that it is part of the event
dispatch mechanism for the socket thread).


>don't you have a race in InitiateSocket?

yes, and that is ok.  it is by design.  to avoid that race would have required
re-entrant callbacks.  i would prefer to avoid that.  since each call to
InitiateSocket checks CanAttachSocket, everything will be fine.  sure, an older
socket request will sometimes lose out, but i don't think that will be a big
issue.  i'll document the code.

as for DataConnectionEstablished, it returns |void| now =)


>I am worried about cycles  with the new ftp handler global.

nsFtpProtocolHandler owns no references to the nsFtpState, so i think this is
safe.  plus, my logs confirm that each nsFtpState is destroyed, so i think this
should be safe.


extra points... no thanks! ;-)

Comment 18

[Mark Smith [:mcs]]

It sounds like this patch will also fix bug 193908 (because FTP data connections
will now be closed in a timely fashion). Darin, do you agree?

Comment 19

[Doug Turner (:dougt)]

fix the comment to be more verbose.  
You may want to document the fact that there is a race condition and mention
this bug.

Comment 20

[Darin Fisher]

mark: yeah, it should fix that too.

Comment 21

[Darin Fisher]

Attachment: v1.1 patch

Comment 22

[Darin Fisher]

fixed-on-trunk

Comment 23

[Mike Potter]

Talking to brade in #composer yesterday, there was a thought that perhaps this
patch would fix the problem with publishing to a webdav server.  We've tested
this here at OEone, and it doesn't fix the problems I reported in bug 201210.
Publishing to a webdav server is still broken with patch 1.1 applied.

Comment 24

[Darin Fisher]

right, this patch has nothing to do with HTTP.

Comment 25

[Chris Petersen]

Ok, using the reduced test case this problem is no longer occuring. In previous
builds, using this test case would reproduce the crash. Verified on the
2003-04-10-08 Macho and 2003-04-10-04 Win32 trunk builds.