Unhandled IRC messages: 598 and 599

RESOLVED FIXED in 1.6

Status

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: florian, Assigned: aleth)

Tracking

Thunderbird Tracking Flags

(thunderbird34 fixed, thunderbird35 fixed, thunderbird_esr3134+ fixed)

Details

(Whiteboard: [1.6-blocking], )

Attachments

(1 attachment, 3 obsolete attachments)

I'm seeing these since the moznet servers have been migrated to the new network:

Warning: Unhandled IRC message:
:fripp.mozilla.org 598 clokep Instantbird moz-uogj6q.cable.rcn.com 1412591115 :I am currently away from the computer.
Source File: resource://gre/components/irc.js
Line: 691
Source Code:
prpl-irc: ircSocket.prototype.onDataReceived

Warning: Unhandled IRC message:
:fripp.mozilla.org 599 clokep Instantbird moz-uogj6q.cable.rcn.com 1412591148 :is no longer away
Source File: resource://gre/components/irc.js
Line: 691
Source Code:
prpl-irc: ircSocket.prototype.onDataReceived


Interestingly, it seems we do have code to handle these messages: http://mxr.mozilla.org/comm-central/source/chat/protocols/irc/ircWatchMonitor.jsm#196
Whiteboard: [1.6-blocking]
Posted patch fixirc.diff (obsolete) — Splinter Review
This fixes the problem on moznet. I'm a bit surprised 598 and 599 have less parameters than the others though, e.g. compare

:fripp.mozilla.org 599 clokep_work Instantbird moz-tej.o7i.10.19.IP 1413303402 :is no longer away

and

:fripp.mozilla.org 604 aleth-build aleth Instantbird moz-0grteb.pools.vodafone.de 1413300162 :is online

I hope that isn't a server quirk.
Attachment #8505057 - Flags: feedback?(clokep)
Does this just fix the warnings or is there a bigger issue this fixes? (Disappearing nicks perhaps?)

This fix scares me a lot since it implies different servers are behaving differently. How was this tested?
(In reply to Patrick Cloke [:clokep] from comment #2)
> Does this just fix the warnings or is there a bigger issue this fixes?
> (Disappearing nicks perhaps?)

The status of buddies in the contact list isn't being updated correctly, as we try to find a buddy using the username instead of the nick.
 
> This fix scares me a lot since it implies different servers are behaving
> differently. How was this tested?

Indeed, that's why it's f?. So far it's been tested on moznet only and I don't know yet how other WATCH implementations behave. Could be an inspircd bug.
Posted patch fixirc.diff 2 (obsolete) — Splinter Review
Assignee: nobody → aleth
Attachment #8505057 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8505057 - Flags: feedback?(clokep)
Attachment #8507395 - Flags: review?(clokep)
Comment on attachment 8507395 [details] [diff] [review]
fixirc.diff 2

Review of attachment 8507395 [details] [diff] [review]:
-----------------------------------------------------------------

I wonder if we should include in the comment what version this is fixed in for inspircd.

::: chat/protocols/irc/ircWatchMonitor.jsm
@@ +198,5 @@
>  
>      "598": function(aMessage) { // RPL_GONEAWAY
>        // <nickname> <username> <hostname> <awaysince> :<away reason>
> +      // We use a negative index as inspircd doesn't send the user's nick as
> +      // the first parameter (see bug 1078223).

I believe this is the referred the "target". So yes, they definitely should be including this.
Attachment #8507395 - Flags: review?(clokep) → review+
Posted patch fixirc.diff 3 (obsolete) — Splinter Review
Improved comment as inspircd just fixed this bug for inspircd v2.0.18.
Attachment #8507395 - Attachment is obsolete: true
Attachment #8507407 - Flags: review?(clokep)
Comment on attachment 8507407 [details] [diff] [review]
fixirc.diff 3

Review of attachment 8507407 [details] [diff] [review]:
-----------------------------------------------------------------

::: chat/protocols/irc/ircWatchMonitor.jsm
@@ +198,5 @@
>  
>      "598": function(aMessage) { // RPL_GONEAWAY
>        // <nickname> <username> <hostname> <awaysince> :<away reason>
> +      // We use a negative index as inspircd versions < 2.0.18 don't send
> +      // the user's nick asthe first parameter (see bug 1078223).

Nit: "as the" in both places. :) You can carry the review forward.
Attachment #8507407 - Flags: review?(clokep) → review+
Posted patch fixirc.diff 4Splinter Review
Attachment #8507407 - Attachment is obsolete: true
Attachment #8507417 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/d02cd637eea5

Should we back port this to tb 31?
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
I would say yes.
We'd like to uplift this to TB31 as it badly affects IRC on inspircd servers such as, since recently, moznet. There doesn't seem to be an appropriate flag to set as this bug is not in the TB component, so using needinfo.
Flags: needinfo?(standard8)
I would suggest that we extend the approval-comm-* flags here, as well as the tracking/status-thunderbird* ones. I've already extended the attachment flags, the tracking flags are bug 1089666.

The target milestone tracking will be a bit difficult, but we might just have to live with that being different for now.
Flags: needinfo?(standard8)
Comment on attachment 8507417 [details] [diff] [review]
fixirc.diff 4

[Approval Request Comment]
Regression caused by (bug #): Bug in inspircd v2.0, as used by moznet
User impact if declined:  incorrect presence for contacts
Testing completed (on c-c, etc.): works well
Risk to taking this patch (and alternatives if risky): none I can see
Attachment #8507417 - Flags: approval-comm-esr31?
Attachment #8507417 - Flags: approval-comm-beta?
Attachment #8507417 - Flags: approval-comm-aurora?
Attachment #8507417 - Flags: approval-comm-beta?
Attachment #8507417 - Flags: approval-comm-beta+
Attachment #8507417 - Flags: approval-comm-aurora?
Attachment #8507417 - Flags: approval-comm-aurora+
Attachment #8507417 - Flags: approval-comm-esr31? → approval-comm-esr31+
You need to log in before you can comment on or make changes to this bug.