Re: bug#59976: ERC 5.4.1: erc-networks--id gets clobbered in erc server buffer on /query name conflict

[J.P.]

Mike Kazantsev <mk.fraggod@gmail.com> writes:

> Hi.

Hi Mike,

Thanks for the detailed bug report. I mostly just have questions for now
but am committed to finding a solution, so please bear with me.

> I've needed to use case-sensitive channel names (with matrix2051 ad-hoc
> ircd), wanted to test updated ERC client for that, and stumbled upon
> what looks like a bug there:
>
>   When IRC server is named "slack", and then it sends you a message from
>   user named "slack", ERC tries to open query-buffer "slack", ends up
>   running erc--open-target("slack") which does kill-all-local-variables()
>   when enabling erc-mode in the buffer.
>
>   That removes erc-networks--id value there and from then on ERC keeps
>   spamming "error in process filter" about erc-networks--id being nil.
>
>
> - What I'm trying to do:
>
>   - (erc-tls ...)  ;; cleates "slack" server buffer and some channels.
>   - Open "slack" buffer

You mean opened manually, like by issuing a /query slack, right? So this
is in addition to the observation above where "it sends you a message
from user named 'slack'"?

>   - Send "/ping myuser" IRC-command there
>

You also mention "case-sensitive channel names." It'd be nice to know
what those look like when they first arrive off the wire. (BTW, if
you're expecting ERC to interoperate with a case-sensitive network, like
one for which "#FOO" and "#foo" are distinct channels, then that may
need a separate bug report.)

> - Expected result:
>
>   Command actually results in PRIVMSG from "slack" user on this
>   specific ircd, so I expect to see that open a separate query-buffer
>   where that message is displayed.
>
>   (message in question - though probably doesn't matter:
>   "msg: ^APING 1670781447.080567^A could not be sent channel_not_found")
>
>
> - Actual result:
>
>   - Emacs slows down while printing following errors to MiniBuffer and 
> *Messages*:
>
>       error in process filter: or: Wrong type argument: erc-networks--id, nil
>       error in process filter: Wrong type argument: erc-networks--id, nil
>
>   - As far as I can tell that IRC connection becomes unusable, and
>     errors like above get signaled from then on randomly, likely on
>     some commands sent from ircd.
>
>
> - Workaround: changing announced name of the server to something else
>   helps - "slack" query-buffer gets created and is separate from server 
> buffer.

You mean by changing `erc-server-announced-name'?

> Was able to understand what seem to be the issue here by enabling
> logging for erc-networks--id changes to *Messages* like this:
>
>   (defun debug-watch-log (sym newval op where)
>     (message "Variable update: %s = %S -> %S [%S %S]"
>       sym (symbol-value sym) newval op where)
>     (backtrace))
>   (add-variable-watcher 'erc-networks--id #'debug-watch-log)
>
> (run M-x cancel-debug-on-variable-change afterwards to disable this)

Thanks for the detective work. In this case, I'm pretty sure the root
cause is not directly related to that variable, despite all appearances.

BTW, the most helpful insights are usually to be found in the contents
of the *erc-protocol* buffer, which can be generated by calling
`erc-toggle-debug-irc-protocol' before connecting for the first time in
an Emacs session. If you're not comfortable sharing such info on the
tracker, please consider doing so out of band.

> I think some kind of disambiguation or conflict-detection for
> query-channel names might be either missing or not working atm,
> but is needed to avoid this happening unintentionally, or maybe
> even on purpose (e.g. to annoy someone by cutting their IRC connection).

Agreed.

Upon detecting the collision, perhaps the query buffer needs to become
"slack!~slack@example.com" (or "slack@slack/me") and/or the server
buffer "slack/me". Additionally, we could have an option that says to
always use full n!u@h's when naming query buffers instead of adapting
dynamically.

That said, at the moment, we only support unambiguous "network IDs" via
the ":id" keyword parameter of `erc' and `erc-tls'. IOW, calling these
with args like

  :server "127.0.0.1"
  :port 2051
  ...
  :id 'Slack

is supposed to always work. But, I can definitely see how that doesn't
in some cases (e.g., a lowercase `:id'). As you say, you may have no
control over who sends you a query, which is still pertinent in the case
of an explicit `:id' so long as it consists entirely of valid nick
characters.

Unfortunately, the best we can do for ERC 5.5 (Emacs 29) is to mention
somewhere, like in (info "(erc) Network Identifier"), that users really
worried about this issue should choose an `:id' containing characters
disallowed in nicks by their network (or just something improbable and
unlikely to be guessed). But, hopefully, we can address this in a more
DWIM-like fashion in an upcoming ELPA release, such as ERC 5.6.

> I'm running ERC 5.4.1 from current 0e5d059 git master on Emacs 28.2
> (replacing "erc" dir in /usr/share/emacs), so it is also possible that
> maybe some change in Emacs 29+ prevents this behavior, but it seems unlikely.

Unlikely, yes. But I sometimes worry about ERC's loaddefs when people
manually shadow/replace instead of building alongside Emacs or
installing from devel:

  (push '("devel" . "https://elpa.gnu.org/devel/";) package-archives)

(Not saying you need to do either, of course.) Thanks again for the very
nice bug report.

J.P.

P.S. I've attached some patches for Emacs 29 that address issues
discovered while briefly looking into this bug; the first is totally
unrelated and the latter two only tangentially so (in case anyone wants
to take a gander).

0001-Be-carefuller-updating-browse-url-var-in-erc-compat.patch
Description: Text Data

0002-Actually-accept-non-symbols-as-IDs-in-erc-open.patch
Description: Text Data

0003-Limit-casemapping-to-appropriate-ranges-in-ERC.patch
Description: Text Data