955595 - Figure out how to handle ISUPPORT info for LIST

Status: RESOLVED FIXED

(Chat Core :: IRC, defect)

Description

[aleth [:aleth]]

*** Original post on bio 2156 at 2013-09-06 12:00:00 UTC ***

Decide what to do with iSUPPORT ELIST and SAFELIST
http://www.irc.org/tech_docs/005.html

Comment 1

[Patrick Cloke [:clokep]]

*** Original post on bio 2156 at 2013-10-11 19:21:32 UTC ***

SAFELIST tells us you will not be kicked for requesting LIST, the conservative thing to do would be to not automatically send LIST unless we've received SAFELIST (which not breaking /list, by the way).

I don't think ELIST is useful unless the UI supports a way to query based on different things...but even then this isn't really useful because we're requesting everything from the server and caching it, we're not querying the server every time.

Comment 2

[aleth [:aleth]]

*** Original post on bio 2156 at 2013-10-12 15:24:05 UTC ***

Do we want to be conservative? We should probably check if most of the major networks at least actually provide ISUPPORT SAFELIST.

Comment 3

[Patrick Cloke [:clokep]]

*** Original post on bio 2156 at 2013-10-24 10:48:53 UTC ***

I think what I'd like to see is the following:
- Change chat.irc.automaticList to be an integer, it will have three values (0, 1, 2), default it to 1:
    0: Never use list.
    1: Use list if the server reports safelist.
    2: Always use list.
- Change the code around [1] to check this preference + whether the 005 supports SAFELIST or not (so pretty much cache this value at [2]).

Thoughts?

[1] http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/irc.js#933
[2] http://lxr.instantbird.org/instantbird/source/chat/protocols/irc/ircISUPPORT.jsm#186

Comment 4

[Florian Quèze [:florian]]

*** Original post on bio 2156 at 2013-10-24 12:43:49 UTC ***

(In reply to comment #3)

> Thoughts?

Seems reasonable.

Comment 5

[Patrick Cloke [:clokep]]

Attachment: Patch v1

Comment 6

[(inactive) Nihanth Subramanya [:nhnt11]]

Comment on attachment 8474144 [details] [diff] [review]
Patch v1

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

Looks good to me, thanks!

Comment 7

[aleth [:aleth]]

Comment on attachment 8474144 [details] [diff] [review]
Patch v1

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

Thanks!

I do hope all standard networks send SAFELIST...

Comment 8

[Patrick Cloke [:clokep]]

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

Comment 9

[aleth [:aleth]]

I believe we've made a mistake here and SAFELIST doesn't mean what we thought it did. I noticed that we weren't getting any LIST results on moznet due to the server not sending SAFELIST. The description of SAFELIST in the link above is

"The LIST is sent in multiple iterations so send queue won't fill and kill the client connection."

Another description, in an older version of inspircd documentation, says [1]

"This token indicates that channel listing is throttled so that it will not flood your client off the server."

So it appears SAFELIST does *not* tell you that you're not going to be kicked for LIST - it tells you that the server is going to try very hard not to flood *the client*.

Therefore I think we should back out this patch as it does the wrong thing.

To check further, I asked on #inspircd:

18:19:15 - aleth: Hi! I was wondering about the status of SAFELIST. The module m_safelist seems to have disappeared, what's the story behind that?
18:19:46 - aleth: Also a heads-up that the chatspike.net SSL cert has expired
18:59:52 - SaberUK: aleth: its not needed anymore due to internal changes
19:00:55 - aleth: SaberUK: Thanks. Just to confirm, the original purpose of the ISUPPORT flag was to tell clients they wouldn't be flooded for LIST,  not that they wouldn't be kicked for LIST?
19:02:54 - SaberUK: aleth: i cant remember
19:03:29 - SaberUK: it was removed like five years ago

So it seems we are conditioning whether we automatically list or not on a flag provided by an obsolete module that hasn't been around for years.

[1] http://webcache.googleusercontent.com/search?q=cache:0Ifx2TktOocJ:https://wiki.inspircd.org/RPL_ISUPPORT_Tokens+&cd=1&hl=en&ct=clnk

Comment 10

[Patrick Cloke [:clokep]]

(In reply to aleth [:aleth] from comment #9)
> "The LIST is sent in multiple iterations so send queue won't fill and kill
> the client connection."

I read this as "The send queue won't be filling forcing us to disconnect the client." Perhaps that's not what it actually means, however. [1] seems to match what I think this means, for what it's worth.

Obviously those guys should know what this does more than we do though... I'm OK backing this out, but I'm confused at what this flag actually means...

[1] http://tools.ietf.org/html/draft-brocklesby-irc-isupport-03#section-3.15

Comment 11

[aleth [:aleth]]

(In reply to Patrick Cloke [:clokep] from comment #10)
> (In reply to aleth [:aleth] from comment #9)
> > "The LIST is sent in multiple iterations so send queue won't fill and kill
> > the client connection."
> 
> I read this as "The send queue won't be filling forcing us to disconnect the
> client." Perhaps that's not what it actually means, however. [1] seems to
> match what I think this means, for what it's worth.
> 
> Obviously those guys should know what this does more than we do though...
> I'm OK backing this out, but I'm confused at what this flag actually means...

It's definitely confusing, and that's the way I read it too, originally.

Either way we should back it out as fully updated servers which don't have any flood problem no longer send SAFELIST.

Comment 12

[Patrick Cloke [:clokep]]

Attachment: Revert Patch v1

This reverts the patch AND returns true for SAFELIST to just ignore it.

Comment 13

[aleth [:aleth]]

Comment on attachment 8510014 [details] [diff] [review]
Revert Patch v1

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

Ah, good idea.

Comment 14

[Patrick Cloke [:clokep]]

https://hg.mozilla.org/comm-central/rev/223659495f31