955742 - Joining multiple chats at once opens spurious MUC

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[aleth [:aleth]]

*** Original post on bio 2289 at 2013-12-21 19:04:00 UTC ***

An additional MUC is opened whose name is the comma-separated parameter passed to Join Chat.

Comment 1

[aleth [:aleth]]

*** Original post on bio 2289 at 2013-12-26 17:24:48 UTC ***

This isn't actually a regression. It only happens when using the Join Chat dialog, and it happens because that dialog does not support the comma-separated syntax for multiple MUCs at once. The fact that it works at all for IRC is accidental (we send "JOIN #channel1,#channel2" and irc.mozilla.org copes). It's only recently become visible because we now open a "#channel1,#channel2" tab.

I'm tempted not to fix this in the hope that nhnt11 will make Join Chat go away sooner rather than later anyway.

Comment 2

[Patrick Cloke [:clokep]]

I don't think we should fix this, the syntax that we use for e.g. autojoin just doesn't make sense in that dialog (since it has a separate password and channel name field).

Comment 3

[Florian Quèze [:florian]]

so why are we opening a broken tab? If invalid syntax is entered by the user, could we reject it or put an error message in the console instead of doing something obviously wrong?

Comment 4

[aleth [:aleth]]

Attachment: joinsanitize.diff

This stops us from opening broken channels. Left for the future, a Join Chat dialog replacement that doesn't fail silently etc.

Comment 5

[Patrick Cloke [:clokep]]

Comment on attachment 8477416 [details] [diff] [review]
joinsanitize.diff

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

::: chat/protocols/irc/irc.js
@@ +1442,5 @@
>    // aComponents implements prplIChatRoomFieldValues.
>    joinChat: function(aComponents) {
>      let channel = aComponents.getValue("channel");
> +    // Mildly sanitize input.
> +    channel = channel.trimLeft().split(",")[0].split(" ")[0];

Does this break having multiple things in auto-join or does the UI do a split(',') somewhere?

What is the split on space for?

Why trimLeft() instead of trim()?

@@ +1448,5 @@
>        this.ERROR("joinChat called without a channel name.");
>        return null;
>      }
>  
> +

Nit: No reason for a double empty line here.

Comment 6

[aleth [:aleth]]

Attachment: joinsanitize.diff 2

> Does this break having multiple things in auto-join or does the UI do a
> split(',') somewhere?

This has nothing to do with autojoin as joinChat does not takes more than one channel parameter. Our funky autojoin/join command syntax is just the reason people have tried to use the Join Chat dialog to join multiple channels at once, which is not supported.

> What is the split on space for?

People trying to put passwords in the channel field.

> Why trimLeft() instead of trim()?

The split on space takes care of trimRight.

Comment 7

[Patrick Cloke [:clokep]]

Comment on attachment 8478995 [details] [diff] [review]
joinsanitize.diff 2

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

r+ with an expanded comment describing what / why this is being done.

::: chat/protocols/irc/irc.js
@@ +1442,5 @@
>    // aComponents implements prplIChatRoomFieldValues.
>    joinChat: function(aComponents) {
>      let channel = aComponents.getValue("channel");
> +    // Mildly sanitize input.
> +    channel = channel.trimLeft().split(",")[0].split(" ")[0];

Expand the comment saying what this is actually doing.

Comment 8

[Patrick Cloke [:clokep]]

Doh, I guess I wanted a better comment. Oh well, I've committed this. Don't worry about it!

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