fix(messages)!: vim.ui_attach message callbacks are unsafe

[luukvbaal]

Problem: Lua callbacks for "msg_show" events with vim.ui_attach() are
executed when it is not safe. The callback can cause Nvim to
crash, change internal variables which may yet be needed etc.
Solution: Batch "msg_show" events and call Lua callback when it is safe.
Instead of separate events, batch and pass an array of all
messages since the last redraw.

[zeertzjq]

Actually ext_messages callback may cause Nvim to crash in many cases, because code calling *msg functions don't expect it to change state.

[luukvbaal]

Hmm, does it? The messages are batched and (usually) flushed in normal_redraw() where changing state should be allowed?

But yeah if they are flushed where it isn't allowed I guess that should be prevented.
(P.S. On phone, not actually sure what I wrote is true)

[zeertzjq]

msg_start() calls msg_ext_ui_flush()

[luukvbaal]

Okay, yeah msg_start seems kind of redundant in the situation where ext_messages is used in core and the message grid is removed.

As in, flushing inside msg_start doesn't seem necessary. Perhaps we can insert a special chunk to indicate the start of a new message.

[zeertzjq]

The handler may call api functions that reset it, leaving :function active after an error.

Is there at least an interactive example where this happens?

[luukvbaal]

Is there at least an interactive example where this happens?

Still on mobile but if you checkout my ext PR and run nvim --clean, you should see the issue when trying to redefine an existing function. I.e.

:func Foo()<CR>
bar<CR>
endf<CR>
:func Foo()<CR>

I think it was try_start() in nvim_buf_set_lines() that unsets did_emsg.

[zeertzjq]

I think this should be solved in API calls instead, because did_emsg and called_emsg are not necessarily equivalent. For example, did_emsg is not incremented with :silent!, while called_emsg is. If ext_messages callback makes the value of did_emsg unreliable then it may also cause other problems like duplicate error messages.

[luukvbaal]

Actually ext_messages callback may cause Nvim to crash in many cases, because code calling *msg functions don't expect it to change state.

I think this should be solved in API calls instead

Should we try to avoid both issues at once by not flushing messages where it can lead to these problems?

I.e. instead of having msg_start/end() flush messages, insert some kind of special chunk to separate messages, or stash the events to emit them when it is safe. @bfredl

[luukvbaal]

The current commit avoids the :function problem this PR originally tried to fix. Also fixes this crash for example:

nvim --clean -S test.lua
-- emit any message

-- test.lua
vim.ui_attach(vim.api.nvim_create_namespace(''), {ext_messages = true}, function(event, ...)
  if event == "msg_show" then
    vim.fn.getchar()
  end
end)

[luukvbaal]

Status:

Postponed to 0.11 cycle and considering to change the signature for the vim.ui_attach ext_messages callbacks @folke. Not touching the event itself, since the issue does not apply to remote UIs. Not strictly necessary but vim.ui_attach is still experimental. If we are throttling the message callbacks, it makes more sense to emit a single message event with an array of all the emitted messages, rather than multiple events.

[folke]

@luukvbaal makes sense!

I'll fix the Noice side when needed :)