93353 - Existing sockets do not forward notification to nested events

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Doug Turner (:dougt)]

Existing socket do not forward notification to nested event's

Suppose during PL_ProcessPendingEvents, one of the pending events wants to
create and pump an event queue.  If an sockettransport exists, all its
notifications will be fowarded to the parent event queue.  The message pump will
effectively do nothing with these new events.  This results in a nice deadlock.

What we need to do is resolve the event queue in the socket notification proxies
before posting.  

Patches to follow.

This needs to find its way onto the embedding branch!

Comment 1

[Doug Turner (:dougt)]

Attachment: netlib change.

Comment 2

[Doug Turner (:dougt)]

Attachment: Makes ResolveEventQueue work consistantly

Comment 3

[Doug Turner (:dougt)]

a couple of comments about the code.

xpcom - 
ResolveEventQueue was not getting the youngest event queue if the parameter
|queueOrConstant| was not a constant.  

netlib - 

The explict assignments in nsRequestObserverProxy::nsRequestObserverProxy were
required so I don't crash.  Without them the mRawPtr was bad!!  Why, I don't know.  


Darin, Chris, I need some review love.  

Comment 4

[Judson Valeski]

cc'ing danm for r= as well. adding topembed keyword

Comment 5

[rpotts (gone)]

hey doug...

this patch makes sense and i hope it does the trick :-)

a quick comment on the netlib patch...
is it possible to cache the eventQ service in nsAsyncStreamListener.cpp rather
than call do_GetService(...) every time we post an event :-)

other than that... r=rpotts

Comment 6

[Doug Turner (:dougt)]

Rick, I was under the impression that these objects get deleted after the event
is posted/handled; there is only one call to do_GetService for the object.  

Comment 7

[Doug Turner (:dougt)]

Commented to danm in an email that I was concerned about having the client
retarget (resolve) event queues prior to calling post.  Maybe we should move
this resolution inside nsIEventQueue::PostEvent?  This would mean that when you
post an event, it always finds the youngest queue.  Dan, can you comment on this
approach?

Comment 8

[Doug Turner (:dougt)]

Darin and I had a conversation about this problem.  We concluded that this is
not the proper fix as it would break all chance for modality.  Handling network
event after an event queue is pushed will potentially result in user code being run.

The better fix it to make FTP's control connection not hold onto the event queue
while this socket is in an idle state.

Comment 9

[Darin Fisher]

Always posting events to the youngest queue on a thread seems wrong to me. 
Doing so would make it impossible to have a truly modal dialog.  Consider this
scenario:

  1) client calls AsyncOpen
  2) client receives OnStartRequest and examines, for example, the http headers
  3) client pops up a modal dialog (pushing an event queue) asking the user what 
     they want to do with the data
  4) client receives OnDataAvailable before user has pressed OK on the modal 
     dialog <-- CLIENT CODE BREAKS HORRIBLY BECAUSE IT NEEDS THE USER'S RESPONSE 
     IN ORDER TO CONTINUE!

Making necko behave this way seems wrong to me, and I think that it really
shouldn't be necessary to resolve the FTP deadlock.  I believe that everything
should work just fine provided event queues are only held onto for the lifetime
of a channel.

The FTP connection object keeps AsyncRead and AsyncWrite requests suspended
while it is idle, and then it reuses these requests when the next channel comes
along and needs to use the FTP connection.  This is problematic since the latter
FTP channel could have been created/opened after a new event queue was pushed
(as in the case of this bug).  So, I think the FTP connection should let the
AsyncRead/Write requests complete when the first channel is done.  This will NOT
close the connection, but it will cause the socket transport to release any
proxies and hence any event queues.  Then when the next FTP channel comes along,
new AsyncRead/Write requests could be issued.

I think this is a much better solution to the problem because it would allow us
to still have modal dialogs.

Comment 10

[Doug Turner (:dougt)]

Attachment: makes FTP release read and write requests.

Comment 11

[Doug Turner (:dougt)]

I just attached a proposed change to ftp which should make this problem go away.
  We have to double check that we don't leak control connections.  Darin, do I
need to reset the Reuse flag?


Reassigning to darin because I am going on vacation.

Comment 12

[Dan M]

My 2¢: I don't know the FTP code well enough to review Doug's patch, but Darin's 
analysis and suggested fix sound great to me. Most clients of the event queue 
system should probably always post to the current youngest event queue, but not 
Necko. In fact, the whole nasty stacked queue system was invented just for this 
purpose.

Comment 13

[Darin Fisher]

Attachment: minor revision: mPort wasn't getting assigned

Comment 14

[Darin Fisher]

r/sr=darin

Comment 15

[Darin Fisher]

fixed-on-trunk

-> dougt

Comment 16

[Darin Fisher]

backed out on the trunk... this patch leaks nsFtpControlConnection objects.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No it doesn't.  You only increased the size of them.

Comment 18

[Darin Fisher]

dbaron: thanks for clarifying that.  at any rate, the bloat increase with this
patch was very noticeable and unexpected.  i'd like to understand why the size
increased so dramatically before this patch lands.  perhaps storing mHost as a
nsCAutoString is wrong?  hmm... that seems suspect.

Comment 19

[Doug Turner (:dougt)]

darin, I want to check this back into the trunk when the tree opens.  Are you
okay with this?

Comment 20

[Darin Fisher]

i'd like to see the nsAutoString and nsCAutoString nsFtpControlConnection member
variables replaced with nsXPIDL[C]String's which would greatly reduce the size
of the control connection object.  any objections?

Comment 21

[Doug Turner (:dougt)]

Fix checked in:

Checking in nsFtpConnectionThread.cpp;
/cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpConnectionThread.cpp,v  <-- 
nsFtpConnectionThread.cpp
new revision: 1.188; previous revision: 1.187
done
Checking in nsFtpConnectionThread.h;
/cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpConnectionThread.h,v  <-- 
nsFtpConnectionThread.h
new revision: 1.84; previous revision: 1.83
done
Checking in nsFtpControlConnection.cpp;
/cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpControlConnection.cpp,v  <-- 
nsFtpControlConnection.cpp
new revision: 1.19; previous revision: 1.18
done
Checking in nsFtpControlConnection.h;
/cvsroot/mozilla/netwerk/protocol/ftp/src/nsFtpControlConnection.h,v  <-- 
nsFtpControlConnection.h
new revision: 1.14; previous revision: 1.13
done

Comment 22

[Axel Hecht]

Hrm. Making this a smoketest blocker. Ftp is dead.

Tested on solaris, ftp://ftp.mozilla.org just finishes, without any display.
Backing this out helped, that is, I get the index page.

See the timeout on bloattests, too.

Axel

Comment 23

[Marek Z. Jeziorek]

please check-into the branch. thanks

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Bugzilla was down two hours ago, so...

about 2 hours ago I checked in a one-line change that fixed FTP being broken.  I
figured out the change to make by diffing the versions that darin backed out and
the version dougt checked in.  There were two differences:  the changes in the
string types, and the dropping of the line
  mHost = host;

So I readded the line, which, with mHost as an nsXPIDLCString, now has to be (or
at least the best way I could think of writing it was):
  mHost.Adopt(nsCRT::strdup(host));

Comment 25

[Doug Turner (:dougt)]

David, after your correction, this should not be a smoketest blocker, correct?

Comment 26

[Axel Hecht]

I tested with dbarons checking, and I still get 
Error loading URL ftp://ftp.mozilla.org/ : 804b0002

:-(

Comment 27

[Axel Hecht]

cool, spam.
I'm too dumb to revert some files. At least, I didn't have the checkin.
Now I do. Now it works. Sorry for the confusion.

Axel

Comment 28

[Doug Turner (:dougt)]

Fixed on 0.9.2 branch.  Marking fixed.

Comment 29

[cls]

ftp is still dead on BeOS :

########## PageCycler starting: ftp://ftp.netscape.com/Welcome
###!!! ASSERTION: NS_ENSURE_TRUE(NS_SUCCEEDED(LoadURI(uri, 0, aLoadFlags))) fail
ed: '(!((LoadURI(uri, 0, aLoadFlags)) & 0x80000000))', file ../../../mozilla/doc
shell/base/nsDocShell.cpp, line 2229

Comment 30

[Doug Turner (:dougt)]

cls, file a seperate bug for this.  this is a new problem.  

Comment 31

[benc]

Any verification needed for this? I can ask david to look at it if you want?

Comment 32

[Tracy Walker [:tracy]]

mass remove verifyme requests greater than 4 months old