Chatzilla doesn't respons to ping's correctly

RESOLVED FIXED

Status

Other Applications
ChatZilla
RESOLVED FIXED
16 years ago
14 years ago

People

(Reporter: Chris Plant, Assigned: Robert Ginda)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

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

16 years ago
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.
(Assignee)

Comment 3

16 years ago
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.
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 4

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

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

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

16 years ago
Created attachment 93251 [details] [diff] [review]
Ping response patch

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

Updated

16 years ago
Keywords: patch, review
(Assignee)

Comment 8

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

no need for that comment.  take it out and r=rginda
Attachment #93251 - Flags: review+

Comment 9

16 years ago
Created attachment 93300 [details] [diff] [review]
Updated diff
Attachment #93251 - Attachment is obsolete: true

Updated

16 years ago
Attachment #93300 - Flags: review?(rginda)

Comment 10

16 years ago
Comment on attachment 93300 [details] [diff] [review]
Updated diff

rginda already reviewed it, just carry it forward.
Attachment #93300 - Flags: review?(rginda) → review+

Updated

16 years ago
Keywords: review
(Assignee)

Comment 11

16 years ago
Comment on attachment 93300 [details] [diff] [review]
Updated diff

bracing is off.  please put each brace on its own line.
Attachment #93300 - Flags: superreview-

Comment 12

16 years ago
Created attachment 107892 [details] [diff] [review]
Updated to use spaced-out bracing.
Attachment #93300 - Attachment is obsolete: true

Comment 13

16 years ago
Created attachment 107893 [details] [diff] [review]
Updated to fix brain-fade from last patch.
Attachment #107892 - Attachment is obsolete: true

Updated

16 years ago
Attachment #107893 - Flags: superreview?(rginda)
(Assignee)

Updated

16 years ago
Attachment #107893 - Flags: superreview?(rginda) → review+
(Assignee)

Comment 14

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

16 years ago
checked in
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
Product: Core → Other Applications
You need to log in before you can comment on or make changes to this bug.