957557 - Plenty of JS errors after removing IRC buddies

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[Florian Quèze [:florian]]

After switching my status to 'offline', 5 IRC buddies are still shown as online in my contacts window.

The only unusual thing I've done during this session is I removed 2 IRC buddies from my buddy list.

In the following errors, 'nick1' is the nick of one of the 2 buddies I removed.

Timestamp: 07/01/14 00:22:54
Error: Error running command QUIT with handler RFC 2812:
{"rawMessage":":nick1![e-mail] QUIT :Quit: Leaving.","command":"QUIT","params":["Quit: Leaving."],"nickname":"nick1","user":"Adium","host":"moz-BBE3ABD.mv.mozilla.com","source":"[e-mail]"}
this._account is undefined
Source File: resource:///modules/jsProtoHelper.jsm
Line: 238
Source Code:
prpl-irc

Timestamp: 07/01/14 00:22:54
Warning: Unhandled IRC message:
:nick1![e-mail] QUIT :Quit: Leaving.
Source File: resource://gre/components/irc.js
Line: 691
Source Code:
prpl-irc

Timestamp: 07/01/14 00:35:13
Error: Error running command 600 with handler WATCH:
{"rawMessage":":concrete.mozilla.org 600 flo-retina nick1 Adium moz-BBE3ABD.mv.mozilla.com 1389051313 :logged online","command":"600","params":["flo-retina","nick1","Adium","moz-BBE3ABD.mv.mozilla.com","1389051313","logged online"],"servername":"concrete.mozilla.org"}
this._account is undefined
Source File: resource:///modules/jsProtoHelper.jsm
Line: 238
Source Code:
prpl-irc

Timestamp: 07/01/14 00:35:13
Warning: Unhandled IRC message:
:concrete.mozilla.org 600 flo-retina nick1 Adium moz-BBE3ABD.mv.mozilla.com 1389051313 :logged online
Source File: resource://gre/components/irc.js
Line: 691
Source Code:
prpl-irc

Timestamp: 07/01/14 00:35:13
Error: Error running command 598 with handler WATCH:
{"rawMessage":":concrete.mozilla.org 598 flo-retina nick1 Adium moz-BBE3ABD.mv.mozilla.com 1389051313 :Away","command":"598","params":["flo-retina","nick1","Adium","moz-BBE3ABD.mv.mozilla.com","1389051313","Away"],"servername":"concrete.mozilla.org"}
this._account is undefined
Source File: resource:///modules/jsProtoHelper.jsm
Line: 238
Source Code:
prpl-irc

Timestamp: 07/01/14 00:35:13
Warning: Unhandled IRC message:
:concrete.mozilla.org 598 flo-retina nick1 Adium moz-BBE3ABD.mv.mozilla.com 1389051313 :Away
Source File: resource://gre/components/irc.js
Line: 691
Source Code:
prpl-irc

Timestamp: 07/01/14 00:35:13
Warning: Unhandled IRC message:
:concrete.mozilla.org 598 flo-retina nick1 Adium moz-BBE3ABD.mv.mozilla.com 1389051313 :Away
Source File: resource://gre/components/irc.js
Line: 691
Source Code:
prpl-irc


When disconnecting:
(repeated 4 times):
Timestamp: 08/01/14 11:25:11
Error: this._account is undefined
Source File: resource:///modules/jsProtoHelper.jsm
Line: 238

(once; last error):
Timestamp: 08/01/14 11:25:11
Error: NS_ERROR_XPC_JAVASCRIPT_ERROR_WITH_DETAILS: '[JavaScript Error: "this._account is undefined" {file: "resource:///modules/jsProtoHelper.jsm" line: 238}]' when calling method: [prplIAccount::disconnect]
Source File: resource://gre/components/imAccounts.js
Line: 693

Comment 1

[aleth [:aleth]]

This is most likely because something like this is missing:

diff --git a/chat/protocols/irc/irc.js b/chat/protocols/irc/irc.js
--- a/chat/protocols/irc/irc.js
+++ b/chat/protocols/irc/irc.js
@@ -762,16 +762,23 @@ ircAccountBuddy.prototype = {
+
+  remove: function () {
+    // remove buddy from account._buddies
+    // remove buddy from ISON/WATCH/MONITOR tracking
+    GenericAccountBuddyPrototype.remove.call(this);
+  }

Comment 2

[aleth [:aleth]]

Attachment: 957557.patch

I _think_ this covers it -- please check I haven't forgotten anything else that needs to happen when removing a buddy.

Comment 4

[Patrick Cloke [:clokep]]

Comment on attachment 8366715 [details] [diff] [review]
957557.patch

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

I didn't notice anything crazy in here and can't think of what this might break.

I do wonder what would happen if we have an open conversation w/ the buddy though?

::: chat/protocols/irc/irc.js
@@ +1088,5 @@
>      // Put the username as the first to be checked on the next ISON call.
>      this.trackQueue.unshift(aNick);
>    },
> +  untrackBuddy: function(aNick) {
> +    let index = this.trackQueue.indexOf(aNick);

Will this run into normalization issues? What happens if a buddy changes their nick?

@@ +1089,5 @@
>      this.trackQueue.unshift(aNick);
>    },
> +  untrackBuddy: function(aNick) {
> +    let index = this.trackQueue.indexOf(aNick);
> +    if (i < 0) {

I think you mean index here, was this tested?

@@ +1090,5 @@
>    },
> +  untrackBuddy: function(aNick) {
> +    let index = this.trackQueue.indexOf(aNick);
> +    if (i < 0) {
> +      this.ERROR("Trying untrack a nick that was not being tracked: "+ aNick);

Nit: Trying to untrack...

::: chat/protocols/irc/ircWatchMonitor.jsm
@@ +6,5 @@
>   * This implements the WATCH and MONITOR commands: ways to more efficiently
>   * (compared to ISON) keep track of a user's status.
>   *
>   *   MONITOR (supported by Charybdis)
> + *     https://github.com/atheme/charybdis/blob/master/doc/monitor.txt

Thanks. :)

@@ +77,5 @@
>    }
>    this.sendMessage("WATCH", params);
>  }
> +function untrackBuddyWatch(aNick) {
> +  this.watchLength--;

--this.watchLength;

@@ +306,5 @@
>    }
>    this.sendMessage("MONITOR", ["+", params.join(",")]);
>  }
> +function untrackBuddyMonitor(aNick) {
> +  this.monitorLength--;

--this.monitorLength

Comment 5

[aleth [:aleth]]

Attachment: 957557.patch 2

(In reply to Patrick Cloke [:clokep] from comment #4)
> I do wonder what would happen if we have an open conversation w/ the buddy
> though?

I guess we'll find out ;) Certainly that should be handled by the UI. On the IRC side things look OK.

> ::: chat/protocols/irc/irc.js
> @@ +1088,5 @@
> > +  untrackBuddy: function(aNick) {
> > +    let index = this.trackQueue.indexOf(aNick);
> 
> Will this run into normalization issues? 

This patch uses the same string for tracking as for untracking.

> What happens if a buddy changes their nick?

Good point - that's not handled anywhere at the moment and might be a separate bug. (Do we even want to try to track this? It smells edge-casey.)

> > +    if (i < 0) {
> 
> I think you mean index here, was this tested?

It's been tested, but of course that if clause is only meant to be satisfied if there is a bug we don't know about yet.

> > +  this.watchLength--;
> 
> --this.watchLength;

I dislike this when it's on a line of its own for some reason, but OK.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/acb32ddefce0