1122096 - Reconnecting password-protected channels sometimes fails

Status: VERIFIED FIXED

(Chat Core :: IRC, defect)

Comment 1

[aleth [:aleth]]

We are sending the correct JOIN commands. It seems inspircd is happy with
JOIN #c1,#c2 ,pwd2
but not with
JOIN #c1,#c2,#c3 ,,pwd3
This didn't get noticed in testing because the password-protected channel I was using to test with had at some point lost its password protection without anyone noticing.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

(In reply to aleth [:aleth] from comment #1)
> This didn't get noticed in testing because the password-protected channel I
> was using to test with had at some point lost its password protection
> without anyone noticing.

I had that problem my password-protected channel a month or two ago!  I wonder if it's a general problem with password-protected channels on irc.mozilla.org, or using the IRC server in question (if your channel is on a different network).  It's also possible that it was a one-time problem affecting all password-protected channels on irc.mozilla.org.

Comment 3

[aleth [:aleth]]

Attachment: passwordjoin.diff

As we'll have to work around this even if inspircd fix their bug, here's a patch. This will break the tests of course, but I'd like f+ before updating those.

Comment 4

[Patrick Cloke [:clokep]]

Comment on attachment 8549850 [details] [diff] [review]
passwordjoin.diff

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

Can't we just sort aItems based on whether there's a key first?

Comment 5

[aleth [:aleth]]

Attachment: passwordjoin.diff using sort()

It's debatable whether this is a simplification.

Comment 6

[Patrick Cloke [:clokep]]

Comment on attachment 8549867 [details] [diff] [review]
passwordjoin.diff using sort()

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

::: chat/protocols/irc/irc.js
@@ +1027,5 @@
> +        if (!k1 && k2)
> +          return 1;
> +        if (k1 && !k2)
> +          return -1;
> +        return 0;

How about return (k1 || "").length - (k2 || "").length? :)

Regardless, I actually like this better, it seems to make sense to me to sort *first*.

Comment 7

[aleth [:aleth]]

Attachment: passwordjoin.diff v3

Includes tests.

I dislike your proposal because we don't really want to be sorting by password length ;) More seriously, the other is more readable.

Comment 8

[Patrick Cloke [:clokep]]

Comment on attachment 8549924 [details] [diff] [review]
passwordjoin.diff v3

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

Can you please add a test that has multiple passwords that get moved to the front? Maybe a little unnecessary, but I'd prefer to cover our bases here.

::: chat/protocols/irc/irc.js
@@ +1029,5 @@
> +        if (k1 && !k2)
> +          return -1;
> +        return 0;
> +      });
> +      let channels = items.map(([channel, key]) => channel);

Part of the comment above the added text talks about grouping channels and keys separately. So...that should go above this line.

Comment 9

[aleth [:aleth]]

Attachment: passwordjoin.diff v4

Comment 10

[Patrick Cloke [:clokep]]

Comment on attachment 8550197 [details] [diff] [review]
passwordjoin.diff v4

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

Looks good, thanks!

Comment 11

[aleth [:aleth]]

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

Comment 12

[aleth [:aleth]]

Attachment: removecommas.diff

As my confidence in IRC server command parsing is somewhat undermined, here's a followup to remove the trailing commas stemming from non-password-protected channels from the JOIN commands we send, just in case that confuses some parser code out there.

Comment 13

[Patrick Cloke [:clokep]]

Comment on attachment 8550726 [details] [diff] [review]
removecommas.diff

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

Judging from the conversation on github, it seems like this is the smart thing to do.

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Verified fixed in today's nightly build (20150117042232).

Comment 15

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/7f67bedec639