955086 - Unhandled IRC message 475 ERR_BADCHANNELKEY

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[Patrick Cloke [:clokep]]

*** Original post on bio 1657 at 2012-08-21 21:38:00 UTC ***

:concrete.mozilla.org 475 clokep_js #testib :Cannot join channel (+k)

Comment 1

[Patrick Cloke [:clokep]]

*** Original post on bio 1657 at 2012-08-21 21:40:11 UTC ***

Also, we only store channel keys when you join a channel with it initially, not if the conversation is already open and then you join.

Comment 2

[Patrick Cloke [:clokep]]

*** Original post on bio 1657 at 2012-08-21 22:00:34 UTC ***

Patch coming up...

Comment 3

[Patrick Cloke [:clokep]]

Attachment: Patch

*** Original post on bio 1657 as attmnt 1822 at 2012-08-21 22:13:00 UTC ***

This kind of depends on bug 955064 (bio 1634), but just if you want to rejoin the room.

This implements the error handling for response 475 and saves the chatRoomFields even if we join after the conversation is open (an easy way to test this is to create a channel with a key required, attempt to join it and you'll get the error message, then attempt to join again with the key).

Comment 4

[aleth [:aleth]]

Comment on attachment 8353581 [details] [diff] [review]
Patch

*** Original change on bio 1657 attmnt 1822 at 2012-08-22 11:23:10 UTC ***

This looks good! (I was worried at first about moving the _chatRoomFields setter out of the constructor, but a closer look at the code suggests this is indeed better.)

>+          // Ensure chatRoomFields information is available for reconnection.
>+          let nName = this.normalize(channelName);
>+          if (hasOwnProperty(this._chatRoomFieldsList, nName))
>+            conversation._chatRoomFields = this._chatRoomFieldsList[nName];
>+          else {
>+            WARN("Opening a MUC without storing its prplIChatRoomFieldValues first.");
>+            conversation._chatRoomFields =
>+              this.getChatRoomDefaultFieldValues(this.name);
>+          }
>           delete this._chatRoomFieldsList[this.normalize(conversation.name)];

Please move the "delete this._chatRoomFieldsList" inside the if clause, and use nName there. r+ with that change :)

Comment 5

[Patrick Cloke [:clokep]]

*** Original post on bio 1657 at 2012-08-22 11:58:46 UTC ***

Comment on attachment 8353581 [details] [diff] [review] (bio-attmnt 1822)
Patch

>diff --git a/chat/protocols/irc/ircBase.jsm b/chat/protocols/irc/ircBase.jsm
>@@ -1249,9 +1259,14 @@ var ircBase = {
>     "475": function(aMessage) { // ERR_BADCHANNELKEY
>       // <channel> :Cannot join channel (+k)
>-      // TODO need to inform the user.
>+      let channelName = aMessage.params[1];
>+      let msg = _("error.wrongKey", channelName);
>+      let conversation = this.getConversation(aMessage.params[1]);
>+      conversation.writeMessage(aMessage.servername, msg, {system: true});
>+      // The stored information must be out of date, so remove it.
>+      delete conversation._chatRoomFields;
>       delete this._chatRoomFieldsList[this.normalize(aMessage.params[1])];
>-      return false;
>+      return true;
>     },

This should use channelName everywhere instead of aMessage.params[1] (and maybe use a shorter name...)

(Can I r- my own patch? :P)

Comment 6

[Florian Quèze [:florian]]

*** Original post on bio 1657 at 2012-08-22 12:00:13 UTC ***

(In reply to comment #5)

> (Can I r- my own patch? :P)

(yes :-P)

Comment 7

[Patrick Cloke [:clokep]]

Attachment: Patch v2

*** Original post on bio 1657 as attmnt 1851 at 2012-08-26 20:26:00 UTC ***

Fixes the comments (and adds a little more spacing into the 475 handler).

Comment 8

[Patrick Cloke [:clokep]]

Comment on attachment 8353581 [details] [diff] [review]
Patch

*** Original change on bio 1657 attmnt 1822 at 2012-08-26 20:26:17 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 9

[aleth [:aleth]]

Comment on attachment 8353610 [details] [diff] [review]
Patch v2

*** Original change on bio 1657 attmnt 1851 at 2012-08-26 20:35:12 UTC ***

>+            delete this._chatRoomFieldsList[this.normalize(nName)];
nName is already normalized.

Never quite sure whether to r- or r+ when it's "fine with trivial changes"...

Comment 10

[Patrick Cloke [:clokep]]

Attachment: Patch v2.1

*** Original post on bio 1657 as attmnt 1853 at 2012-08-26 20:40:00 UTC ***

Good catch. :)

Comment 11

[Patrick Cloke [:clokep]]

Comment on attachment 8353610 [details] [diff] [review]
Patch v2

*** Original change on bio 1657 attmnt 1851 at 2012-08-26 20:40:55 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 12

[aleth [:aleth]]

Comment on attachment 8353612 [details] [diff] [review]
Patch v2.1

*** Original change on bio 1657 attmnt 1853 at 2012-08-26 20:42:30 UTC was without comment, so any subsequent comment numbers will be shifted ***

Comment 13

[Patrick Cloke [:clokep]]

*** Original post on bio 1657 at 2012-08-28 00:52:58 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/44eb8c019f3c