79311 - socket doesn't always return data after connecting

Status: RESOLVED FIXED

(Other Applications Graveyard :: ChatZilla, defect)

Description

[Eric Murphy]

(Applies to all trunk builds since somewhere around April 25, to current, and
also 0.9)

For some reason, every build since late April had a non-working Chatzilla with
the problem orginating with its TCP Socket. Chatzilla will display that it's
attempting to connect, and just sits there.

I am working on Jabberzilla, and I use almost the same JavaScript socket code as
Chatzilla, and the extact same problem affects my client.

However, in trying numerous builds over the past few weeks, Chatzilla HAS worked
twice. It would connect, and then I would try to connect again in the same
session with no luck. I restart Moz, and it will not work. I restart Moz several
times, and it will not work.

I have verified this problem on two different machines running Windows 2000. I
have not had the chance to try another OS. This seems really strange because I
have not talked to anyone else with this same problem.

I will try to do some diagnosis here tonight, and post my results here.

Comment 1

[Keyser Sose]

I have seen this too... The thing is that it actually connects but doesnt
display the response (I have watched it connect with another irc client) I can
even join channels with it but it never displays any response. Very odd, Marking
NEW.

Comment 2

[Eric Murphy]

OK, being a busy schoolboy, I just figured out that I made two bad assumptions
because of my lack of time and energy.

FIRST:
A couple of weeks ago, Jabberzilla was exibiting the same behavior as Chatzilla,
so I made the assumption that there was something wrong with Necko affecting
both Chatzilla and Jabberzilla.

I just tested the latest trunk and 0.9 builds, and Jabberzilla DOES work again.
Shame on me for not testing my own software. I assumed that broken Chatzilla =
broken Jabberzilla.

SECOND:
I assumed that since Chatzilla will not connect to irc.mozilla.org, it will not
connect with anything. It connects with irc.mozdev.org just fine. Weird huh?

Going to change the component to Chatzilla and reset the summary. Sorry for any
confusion.

Comment 3

[neeti]

Reassigning to Chatzilla owner

Comment 4

[Robert Ginda]

I've seen this for a few weeks now, but han't had time to lok into it.  I
suspect necko as well.  I've noticed that if I /disconnect and /connect again
it'll eventually work, and sometimes it Just Works the first time.

cc'ing darin because I think it'll boil down to a necko problem.  I'll try to
get better information soon.

Comment 5

[Eric Murphy]

OK, today I tried this all again, and guess what? My statements about my
assumptions were wrong. Now I cannot get Chatzilla and Jabberzilla to do
anything once again. Too weird.

Build: 2001 05 07 04

IMPORTANT:

Chatzilla would not connect. When I closed the Chatzilla window, I noticed this:

chrome://chatzilla/content/lib/js/connection-xpcom.js line 95: this._inputStream
 has no properties

This seems to be the culprit.

Comment 6

[Robert Ginda]

this._inputStream has no properties when you close the window because the
connection attempt failed in a strange way, not the other way around.

Comment 7

[Eric Murphy]

OK. The only other thing I want to say is that Chatzilla don't work for me
probably 95% of the time, considering all the attempts I have made over the past
few weeks.

I don't know what kind of a failure rate others are getting.

Comment 8

[Samuel Sieb]

Changing OS to all.
There is a workaround:
Try to /attach.  If it doesn't work, do a /disconnect.  Then do the /attach again.
This always works for me.

Comment 9

[Eric Murphy]

Yep, this works for me too. Thanks for the tip.

Comment 10

[Samuel Sieb]

The "this._inputStream" error occurs when I try /disconnect when /attach fails.
When I /disconnect after a successful /attach, I get this error:
-*- chatzilla: onStartRequest: [xpconnect wrapped nsIRequest], [xpconnect
wrapped nsISupports]
Document http://bugzilla.mozilla.org/show_bug.cgi?id=79311 loaded successfully
************************************************************
* Call to xpconnect wrapped JSObject produced this error:  *
[Exception... "Not Connected. [nsIStreamListener::onDataAvailable]"  nsresult:
"0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "<unknown>"  data: no]
************************************************************
-*- chatzilla: onStopRequest: [xpconnect wrapped nsIRequest], [object Object],
2153185310 

Comment 11

[Samuel Sieb]

This also happens when I reconnect after being disconnected.
If I manually disconnect, then the first /attach works.

Comment 12

[Robert Ginda]

Setting target to 0.9.1, I'll research the cause now.

Comment 13

[Robert Ginda]

I've only been able to reproduce this once in the last fifteen or so times I've
tried it, although there have been times where it fails three or four times in a
row in the past.  It's definatley a tough one to track down.

Darin, I had another report of someone who saw themselves connected, but got no
indication of it in chatzilla.  The one time I was able to reproduce this, the
stream listener I pass to nsSocketTrasnport::AsyncRead never recieved
notifications.  Eyeballing the AsyncRead code, I don't see an obvious problem in
hooking up the observer (it sounds like an uninitialized variable to me.)  Do
you have any thoughts?

Comment 14

[Robert Ginda]

I've had more luck reproducing this on my windows machine, but still no leads on
what's causing it.  I stepped into every call made from AsyncRead and things
seem to go OK, yet my observer never gets called.  No onStartRequest, no
onDataAvailable.

Comment 15

[Robert Ginda]

Almost forgot... The reports are correct, we do actually make the connection,
and chatzilla can *send* data over the socket, it just doesn't get any back.

Comment 16

[Robert Ginda]

I'm making a bit of progress here.  Darin noted that mixing asyncRead with
openOutputStream (what chatzilla does) is a Bad Thing.  I've reworked
connection-xpcom.js to use asyncWrite()s, and that *seems* to fix the problem. 
I'm attaching the patch to this bug, but only so that others can verify that it
fixes the problem.  It introduces a new problem that I'd like to fix before we
check anthing in.

Now that we're using asyncWrite, when the user clicks on the window manager's
[x] icon to close chatzilla, the window goes away before we get the chance to
send the QUIT message to the server.  The net result is that it looks to
everyone else on IRC that we just crashed (your quit message is replaced by
something like "Remote closed connection".)  I should be able to work around
this by returning false from onClose until we've closed all of our open connections.

Comment 17

[Robert Ginda]

Attachment: asyncWrite patch, pass one.

Comment 18

[Eric Murphy]

The attachment has garbage in it. Could you repost it?

Comment 19

[Robert Ginda]

Garbage?  Looks fine to me.  What kind of garbage?

Comment 20

[Eric Murphy]

Here is part of what I see:



Index: js/lib/connection-xpcom.js
===================================================================
RCS file: /cvsroot/mozilla/extensions/irc/js/lib/connection-xpcom.js,v
retrieving revision 1.19
diff -u -r1.19 connection-xpcom.js
--- connection-xpcom.js	2001/04/10 07:25:55	1.19
+++ connection-xpcom.js	2001/05/12 00:31:34
@@ -21,13 +21,8 @@
  *  Robert Ginda, [e-mail], original author
  *  Peter Van der Beken, [e-mail], necko-only version
  *
- * depends on utils.js, XPCOM, and the XPCOM component
- *  component://misc/bs/connection
+ * depends on utils.js, and XPCOM/XPConnect in the JS environment
  *
- * sane wrapper around the insane bsIConnection component.  This
- * component needs to be replaced, or at least fixed, so this wrapper
- * will hopefully make it easy to do this in the future.
- *
  */
 
 function toScriptableInputStream (i)
@@ -61,7 +56,8 @@
 
 }
 
-CBSConnection.prototype.connect = function(host, port, bind, tcp_flag)
+CBSConnection.prototype.connect =
+function bc_connect(host, port, bind, tcp_flag)
 {
     if (typeof tcp_flag == "undefined")
 		tcp_flag = false;
@@ -71,14 +67,23 @@
     this.bind = bind;
     this.tcp_flag = tcp_flag;
 
-    this._channel = this._sockService.createTransport (host, port, null, -1,
-                                                       0, 0);
-    if (!this._channel)
-        throw ("Error opening channel.");
-
-    this._outputStream = this._channel.openOutputStream(0, -1, 0);
-    if (!this._outputStream)
-        throw ("Error getting output stream.");
+    this._transport = this._sockService.createTransport (host, port, null, -1,
+                                                         0, 0);
+    if (!this._transport)
+        throw ("Error cratinf transport.");
+
+    if (jsenv.HAS_NSPR_EVENTQ) 
+    {   /* we've got an event queue, so start up an async write */
+        this._write_req =
+            this._transport.asyncWrite (StreamProvider, null, 0, -1, 0);
+    }
+    else
+    {   /* no nspr event queues in this environment, we can't use async writes,
+         * so set up the output stream. */
+        this._outputStream = this._transport.openOutputStream(0, -1, 0);
+        if (!this._outputStream)
+            throw ("Error getting output stream.");
+    }    
 
     this.connectDate = new Date();
     this.isConnected = true;
@@ -87,9 +92,9 @@
   
 }
 
-CBSConnection.prototype.disconnect = function()
-{
-    

Comment 21

[Robert Ginda]

what's the problem with that?  it's a diff.  you need to apply it with the
`patch` command.

Comment 22

[Eric Murphy]

Well, I'm not familiar with these :-) It's not pretty. Could you just post the
JS file? Then I can add it to the XPI myself.

Is it worth rebuilding it for just one JS file? LOL. Sorry, I don't have the 
source tree on my machine either.

Thanks.

Comment 23

[timeless]

rginda: the last line of your file is:
 function onNotImplemented()
that's not in what erikc wrote.

eric:
wget -O 34159 http://bugzilla.mozilla.org/showattachment.cgi?attach_id=34159
cd mozilla/extensions/irc
patch < 34159

Comment 24

[Robert Ginda]

Eric, If you're going to try this change, you'll need to apply the patch.  There
are changes to multiple files, and I'm not about to attach them all.  Diffs may
be ugly, but if you're going to hack mozilla you'll need to figure them out, sorry.

I'll attach connection-xpcom.js in entirety, but dropping it in your
chatzilla.xpi will *not* work.

timeless, eric only pasted part of the attachment.

Comment 25

[Robert Ginda]

Attachment: all of connection-xpcom.js

Comment 26

[Robert Ginda]

I'm not going to be able to clean this patch up enough to check in by the 0.9.1
deadline, moving to 0.9.2.

Comment 27

[Eric Murphy]

I updated my Jabberzilla socket code to use only asyncWrite, but it is still not
working correctly. In fact, it is even more unreliable. It worked correctly only
once out of many tries.

It's acting like it cannot write any data after receiving data. I'll play with
it some more, but I'm skeptical this fixes my problem.

Comment 28

[Eric Murphy]

OK, I got it working reliably. I changed some little things here and there.
Still not sure what the exact problem was.

I cannot get it to connect after disconnecting, without restarting Mozilla. This
is probably something wrong with my code.

Later.

Comment 29

[Eric Murphy]

OK, I got it working great now. The reconnect was a problem with my code being
in a component. Needed to reinitialize everything, after making it null. 

Now it even handles server lag well (I was having problems with that before).

Thanks for getting this done so quickly, Rob!

Comment 30

[Robert Ginda]

new patch, all done.  Looking for an r= so I can check this in.  Changes are
(copied from ChangeLog) :

connection-xpcom.js:
- according to darinf (the current necko guy), using openOutputStream with
asyncRead is a bad thing.  Most of the changes in this file involve migrating
from usage of openOutputStream to asyncWrite.
- Changes also include fixing the function declaration syntax to match the rest
of the code (two lines, named functions.)

irc-debug.js:
- check nextLine before using it.

irc.js:
- changes to work with new socket interface.

utils.js:
- fix HAS_XPCOM test (XPCDOM broke it.)
- add jsenv.HAS_NSPR_EVENTQ

mybot.js:
- not built -
- add dummy escape/unescape if it isn't there

handlers.js:
- return false the first time through onClose(), and disconnect from all
servers.  This makes sure we keep the window around long enough to send the QUIT
messages.
- close window if client.userClose is set and we disconnected from the last server.

static.js:
- copy client.userAgent code from chatzilla 0.8.2
- implement getConnectionCount()

Comment 31

[Robert Ginda]

Attachment: latest patch

Comment 32

[Robert Ginda]

In addition to fixing the problem discussed in this bug, the patch:
* causes connection attempts to be made asynchronously.  This keeps chatzilla from
  hanging the UI while it initiates socket connections.
* adds "connection timed out" and "unknown host" error messages, using the status
  passed in from onStopRequest.
* adds client.userAgent, from the 0.8.2 code.

Comment 33

[Steve]

Hi
Build 2001-06-03-20
Still doesn't work for on windows ME.
Went to http://www.hacksrus.com/~ginda/chatzilla/
for 0.8.2
Tried the download install: failed.
Did a manual download and followed the install
instructions, result same as before.
It seems to connect but doesn't
display the response or any thing else.

Comment 34

[Robert Ginda]

There was a problem with the 0.8.2 xpi posted to hacksrus.  Please *clear your
cache* using the edit->prefs->advanced->cache screen, and try again.  I've
installed and used it successfully on my WinNT box, running build 2001060320.

Comment 35

[Robert Ginda]

Here comes an updated patch.  New fixes are...

- Remove undefined exception checks in connection-xpcom.js
- Remove undefined |msg| test in irc.js:CIRCServer.prototype.onStreamClose()
- Route data-available event immediatley to avoid out-of-order events (we used
to process the stream close before the last message from the server, making it
look like we got data from a disconnected socket.)
- Applied the patch from 75226 to fix the initial slider position.  Sure it's
got nothing to do with the bug, but it's a small patch that does alot of good.
- Updated ChangeLog to reflect the new changes.

I'll update the 0.8.2 xpi on hacksrus in the next few minutes.

Comment 36

[Robert Ginda]

Attachment: latest latest patch

Comment 37

[Robert Ginda]

One more patch...

in chatzilla-service.js:
- fix signature of handleContent() for bug 80993
- add stub allowPort() method to match dougt's last checkin.

Yes, I know these have nothing to do with the bug at hand, but it's easier to
get it all in as a single patch.

Comment 38

[Robert Ginda]

Attachment: most latest patch

Comment 39

[Samuel Sieb]

r=[e-mail]

Comment 40

[Brendan Eich [:brendan]]

rs=[e-mail]

Comment 41

[Asa Dotzler [:asa]]

a= [e-mail] for checkin.
(on behalf of drivers)

Comment 42

[Robert Ginda]

Waiting on an open tree for checkin.

Comment 43

[Robert Ginda]

Checked into trunk and branch.