~sircmpwn/aerc

This thread contains a patchset. You're looking at the original emails, but you may wish to use the patch review UI. Review patch
6 3

[PATCH] Don't propagate negative offsets when drawing

Connor Kuehl <cipkuehl@gmail.com>
Details
Message ID
<20210901165925.53796-1-cipkuehl@gmail.com>
DKIM signature
pass
Download raw message
Patch: +4 -0
I've been observing a 100% reproducible panic when running aerc in a tab
in my terminal emulator (Terminal.app on macOS).

To reproduce:
1. Run aerc in a tab
2. Open another tab in the terminal emulator session so aerc does not
   have the entire window to itself.
3. Close all other tabs in the terminal emulator session so aerc's tab
   is the last one.
4. Observe the panic

goroutine 1 [running]:
runtime/debug.Stack()
    /opt/homebrew/Cellar/go/1.17/libexec/src/runtime/debug/stack.go:24 +0x88
runtime/debug.PrintStack()
    /opt/homebrew/Cellar/go/1.17/libexec/src/runtime/debug/stack.go:16 +0x20
main.PanicTermFix(0x0)
    /Users/connor/git/aerc/aerc.go:212 +0x48
panic({0x10341cf40, 0x1400013c360})
    /opt/homebrew/Cellar/go/1.17/libexec/src/runtime/panic.go:1052 +0x2ac
git.sr.ht/~sircmpwn/aerc/lib/ui.(*Context).Subcontext(0x14000420240, 0x0, 0xffffffffffffffff, 0x82, 0x1)
    /Users/connor/git/aerc/lib/ui/context.go:47 +0x1b0
git.sr.ht/~sircmpwn/aerc/lib/ui.(*Grid).Draw(0x140000d84d0, 0x14000420240)
    /Users/connor/git/aerc/lib/ui/grid.go:143 +0x2a4
git.sr.ht/~sircmpwn/aerc/widgets.(*Aerc).Draw(0x140000d8580, 0x14000420240)
    /Users/connor/git/aerc/widgets/aerc.go:177 +0x34
git.sr.ht/~sircmpwn/aerc/lib/ui.(*UI).Tick(0x140003aa000)
    /Users/connor/git/aerc/lib/ui/ui.go:113 +0x2ac
main.main()
    /Users/connor/git/aerc/aerc.go:197 +0x9f8
aerc crashed: Attempted to create context with negative offset

In my terminal emulator, when the other tabs are closed, the window size
changes to remove the space that the tabs took up. This is when aerc
will panic.

To avoid panicking, just skip over the cells with negative offsets since
they are "off screen" anyway.

Signed-off-by: Connor Kuehl <cipkuehl@gmail.com>
---
  Frankly, I am not 100% certain this patch is the best way to resolve
  this. I am sending it anyway to hopefully cultivate some discussion or
  guidance on the best way forward, since this doesn't strike me as
  resolving the root cause that is resulting in negative offsets in the
  first place. I am not (yet) an expert on aerc internals :-)

 lib/ui/grid.go | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/lib/ui/grid.go b/lib/ui/grid.go
index f505ce0..2d19571 100644
--- a/lib/ui/grid.go
+++ b/lib/ui/grid.go
@@ -123,6 +123,10 @@ func (grid *Grid) Draw(ctx *Context) {
		cols := grid.columnLayout[cell.Column : cell.Column+cell.ColSpan]
		x := cols[0].Offset
		y := rows[0].Offset
		if x < 0 || y < 0 {
			continue
		}

		width := 0
		height := 0
		for _, col := range cols {
-- 
2.30.1 (Apple Git-130)
Connor Kuehl <cipkuehl@gmail.com>
Details
Message ID
<CDYVA74S6OUG.2P1BCBEQ2DTNI@gilgalad.local>
In-Reply-To
<20210901165925.53796-1-cipkuehl@gmail.com> (view parent)
DKIM signature
pass
Download raw message
On Wed Sep 1, 2021 at 11:59 AM CDT, Connor Kuehl wrote:
> I've been observing a 100% reproducible panic when running aerc in a tab
> in my terminal emulator (Terminal.app on macOS).
>
> To reproduce:
> 1. Run aerc in a tab
> 2. Open another tab in the terminal emulator session so aerc does not
> have the entire window to itself.
> 3. Close all other tabs in the terminal emulator session so aerc's tab
> is the last one.
> 4. Observe the panic

I'll also note that this doesn't appear to happen in GNOME terminal,
though I don't know if that information is terribly interesting here or
not.

Connor
Details
Message ID
<CDZ6X8STZ8Q4.2BOPIBNHIG6VH@ARCHe-Moritz>
In-Reply-To
<CDYVA74S6OUG.2P1BCBEQ2DTNI@gilgalad.local> (view parent)
DKIM signature
pass
Download raw message
> I'll also note that this doesn't appear to happen in GNOME terminal,
> though I don't know if that information is terribly interesting here or
> not.

Then that might be a reason for me not being able to reproduce it. I am
using KiTTY and neither opening tabs, nor splitting the window caused a
panic for me. Regarding whether this is the best way, I would actually
say it is the best fix we currently have. I am still going through the
codebase, and understanding the inner workings.

I think having an unresponsive grid while some invalid values are given
is preferable over a panic.

One Proposed change would be to add this check with the similar guard in
lib/ui/grid.go:140, just to keep all of them in one place.
Details
Message ID
<CDZCZQUB7N2O.2YW7ZOFSYXMRC@dpatterson>
In-Reply-To
<CDZ6X8STZ8Q4.2BOPIBNHIG6VH@ARCHe-Moritz> (view parent)
DKIM signature
fail
Download raw message
DKIM signature: fail
Perhaps we shouldn't be panicking here at all? If we returned an error
here and forced the caller to handle that then we could probably avoid a
lot of these hard to reproduce panics. Although context.Subcontext does
get called in a lot of locations so perhaps the churn wouldn't be worth
it.
Details
Message ID
<CDZD1YWPD0CY.YG0P4DCNW88@ARCHe-Moritz>
In-Reply-To
<CDZCZQUB7N2O.2YW7ZOFSYXMRC@dpatterson> (view parent)
DKIM signature
pass
Download raw message
On Thu Sep 2, 2021 at 1:22 PM CEST, Daniel Patterson wrote:
> Perhaps we shouldn't be panicking here at all? If we returned an error
> here and forced the caller to handle that 
The best way to handle errors, is to handle them gracefully IMO. Since
the size is outside of the scope of aerc, so aerc should do the next
reasonable thing: nothing.

We could try to show an error, but what would this mean to the user? (If
they can at all read it) Close to nothing, if the user doesn't ignore
the error outright.

> then we could probably avoid a lot of these hard to reproduce panics.
Panics are there for a reason, so getting rid of panic doesn't really
help here if we don't adress the underlying reason.

I personally like the patch so far as it does what I'd consider
"gracefuly handling" of the error: ignore the faulty state and try again
later with different values.
Details
Message ID
<CDZDJNSZVJX4.136Z4JRECPM6R@dpatterson>
In-Reply-To
<CDZD1YWPD0CY.YG0P4DCNW88@ARCHe-Moritz> (view parent)
DKIM signature
fail
Download raw message
DKIM signature: fail
On Thu Sep 2, 2021 at 12:25 PM BST, Moritz Poldrack wrote:

> We could try to show an error, but what would this mean to the user? (If
> they can at all read it) Close to nothing, if the user doesn't ignore
> the error outright.
I think you're misunderstanding me, I don't mean to display an error to
the user, but instead to return an error from the context.Subcontext
function and allow grid.Draw to decide what to do in that case.

> Panics are there for a reason, so getting rid of panic doesn't really
> help here if we don't adress the underlying reason.
Indeed, if a certain piece of code decides that a subcontext failing to
be produced is unrecoverable, then they can feel free to panic, and the
error return should probably be descriptive enough that panic(err) is
all that is required in that case.

> I personally like the patch so far as it does what I'd consider
> "gracefuly handling" of the error: ignore the faulty state and try again
> later with different values.
I think the patch is completely fine, but I'm just imagining that if
that function gets called from anywhere other than grid.Draw then we
could run into the exact same issue again.
Connor Kuehl <cipkuehl@gmail.com>
Details
Message ID
<CDZS0ZSKBWI8.32O1PY6M1TIDE@gilgalad.local>
In-Reply-To
<CDZ6X8STZ8Q4.2BOPIBNHIG6VH@ARCHe-Moritz> (view parent)
DKIM signature
pass
Download raw message
On Thu Sep 2, 2021 at 1:37 AM CDT, Moritz Poldrack wrote:
> One Proposed change would be to add this check with the similar guard in
> lib/ui/grid.go:140, just to keep all of them in one place.

Can do. Although, its current position spares aerc the effort of
iterating over each row and column before it reaches this check, since
it will check first and continue before reaching those for-loops.

I will try to spend more time this weekend looking into why the grid is
being constructed with negative offsets in the first place. If I find a
better solution, I'll send that, otherwise, I can send a v2 with your
suggestion.

Connor
Reply to thread Export thread (mbox)