Reconnecting password-protected channels sometimes fails

VERIFIED FIXED in 1.6

Status

Chat Core
IRC
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: aleth, Assigned: aleth)

Tracking

({regression})

trunk
x86
Mac OS X
regression

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

3 years ago
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.
(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.
(Assignee)

Comment 3

3 years ago
Created attachment 8549850 [details] [diff] [review]
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.
Attachment #8549850 - Flags: feedback?(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?
Attachment #8549850 - Flags: feedback?(clokep) → feedback-
(Assignee)

Comment 5

3 years ago
Created attachment 8549867 [details] [diff] [review]
passwordjoin.diff using sort()

It's debatable whether this is a simplification.
Attachment #8549867 - Flags: feedback?(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*.
Attachment #8549867 - Flags: feedback?(clokep) → feedback+
(Assignee)

Comment 7

3 years ago
Created attachment 8549924 [details] [diff] [review]
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.
Attachment #8549850 - Attachment is obsolete: true
Attachment #8549867 - Attachment is obsolete: true
Attachment #8549924 - Flags: review?(clokep)
(Assignee)

Updated

3 years ago
Summary: Reconnecting password-protected channels that were not autojoined fails → Reconnecting password-protected channels sometimes fails
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.
Attachment #8549924 - Flags: review?(clokep) → review-
(Assignee)

Comment 9

3 years ago
Created attachment 8550197 [details] [diff] [review]
passwordjoin.diff v4
Attachment #8549924 - Attachment is obsolete: true
Attachment #8550197 - Flags: review?(clokep)
Comment on attachment 8550197 [details] [diff] [review]
passwordjoin.diff v4

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

Looks good, thanks!
Attachment #8550197 - Flags: review?(clokep) → review+
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 1.6
(Assignee)

Comment 12

3 years ago
Created attachment 8550726 [details] [diff] [review]
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.
Attachment #8550726 - Flags: review?(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.
Attachment #8550726 - Flags: review?(clokep) → review+
Verified fixed in today's nightly build (20150117042232).
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.