139057 - Chatzilla doesn't respons to ping's correctly

Status: RESOLVED FIXED

(Other Applications :: ChatZilla, defect)

Description

[Chris Plant]

PING test.monkeyircd.org to chatzilla should get something like PONG
test.monkeyircd.org back, instead I'm just getting PONG :.
The URL points to the packet log from ethereal.

Comment 1

[James Ross]

Right - this is because on lines 726 to 729 of \content\chatzilla\lib\irc\irc.js
check for a " :" in the command, and set e.meat (meant to contain the final
parameter of the IRC command) to blank if there is no " :" in it.

This then causes a bug when the PING command from a server passes it's only
parameter without a preceeding colon (which it's perfectly entitled to do in
most cases) - since e.meat is "" for this command, the responce generated by
1347 (same file) is simple "PONG :". This is clearly not what it's suppsoed to be.

This can be fixed in either of the two places, depending on how e.meat is
/supposed/ to work (i.e. should it be set to the last parameter if it's not got
a colon before it?).

(this bug should be set to OS/Platform: all)

Comment 2

[James Ross]

Ok - here's a psudo-patch (I don't have time to set up CVs, etc for this):

[ patch from line 1346 to 1349 ]
    /* non-queued send, so we can calcualte lag */
+    /* Changed as fix for bug #139057 */
+    this.connection.sendData ("PONG :" + e.params[e.params.length - 1] + "\n");
-    this.connection.sendData ("PONG :" + e.meat + "\n");
    this.connection.sendData ("PING :LAGTIMER\n");
    this.lastPing = this.lastPingSent = new Date();
[ end ]

I've tested this with a server which was known to cause this bug
(irc.monkeyircd.org), and it's fixed now.

Comment 3

[Robert Ginda]

The code probably sent |e.meat| because some irc servers send there data there.
 We'll probably have to send |e.meat| if it's there, or the last param if it's not.

Comment 4

[Robert Ginda]

for the record, |e.meat| is *always* the data after the colon.  If |e.meat| is
blank, the message had no <trailing> information.

Comment 5

[James Ross]

Right, so the last parameter is *either* in |e.meat| *or* the last |e.params|
item? Isn't that a bit inconsistant? (given the server can add a colon to the
last param even if it doesn't need to)

Comment 6

[James Ross]

If that is correct, the patch would need to be along these lines:

[ patch from line 1346 to 1349 ]
    /* non-queued send, so we can calcualte lag */
+    /* Changed as fix for bug #139057 */
+    if (e.meat) {
+        this.connection.sendData ("PONG :" + e.meat + "\n");
+    } else {
+        this.connection.sendData ("PONG :" + e.params[e.params.length - 1] + "\n");
+    }
-    this.connection.sendData ("PONG :" + e.meat + "\n");
    this.connection.sendData ("PING :LAGTIMER\n");
    this.lastPing = this.lastPingSent = new Date();
[ end ]

Comment 7

[James Ross]

Attachment: Ping response patch

Hopefully this will patch the Ping-response code to respond to all types of
pings correctly.

Comment 8

[Robert Ginda]

Comment on attachment 93251 [details] [diff] [review]
Ping response patch

no need for that comment.  take it out and r=rginda

Comment 9

[James Ross]

Attachment: Updated diff

Comment 10

[Samuel Sieb]

Comment on attachment 93300 [details] [diff] [review]
Updated diff

rginda already reviewed it, just carry it forward.

Comment 11

[Robert Ginda]

Comment on attachment 93300 [details] [diff] [review]
Updated diff

bracing is off.  please put each brace on its own line.

Comment 12

[James Ross]

Attachment: Updated to use spaced-out bracing.

Comment 13

[James Ross]

Attachment: Updated to fix brain-fade from last patch.

Comment 14

[Robert Ginda]

I'm not actually an sr, so I probably shouldn't mark this sr=.  As far as
chatzilla is concerend though, you don't need any more than an r= from me to
check in.

Comment 15

[Samuel Sieb]

checked in