Closed Bug 955086 Opened 10 years ago Closed 10 years ago

Unhandled IRC message 475 ERR_BADCHANNELKEY

Categories

(Chat Core :: IRC, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: clokep, Assigned: clokep)

References

Details

Attachments

(1 file, 2 obsolete files)

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

:concrete.mozilla.org 475 clokep_js #testib :Cannot join channel (+k)
*** 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.
*** Original post on bio 1657 at 2012-08-21 22:00:34 UTC ***

Patch coming up...
Assignee: nobody → clokep
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
*** 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).
Attachment #8353581 - Flags: review?(bugzilla)
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 :)
Attachment #8353581 - Flags: review?(bugzilla) → review-
*** 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)
*** 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)
Attached patch Patch v2 (obsolete) — Splinter Review
*** 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).
Attachment #8353610 - Flags: review?(bugzilla)
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 ***
Attachment #8353581 - Attachment is obsolete: true
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"...
Attachment #8353610 - Flags: review?(bugzilla) → review-
Attached patch Patch v2.1Splinter Review
*** Original post on bio 1657 as attmnt 1853 at 2012-08-26 20:40:00 UTC ***

Good catch. :)
Attachment #8353612 - Flags: review?(bugzilla)
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 ***
Attachment #8353610 - Attachment is obsolete: true
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 ***
Attachment #8353612 - Flags: review?(bugzilla) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 1657 at 2012-08-28 00:52:58 UTC ***

Committed as http://hg.instantbird.org/instantbird/rev/44eb8c019f3c
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.3
You need to log in before you can comment on or make changes to this bug.