Don't propagate negative offsets when drawing v1 PROPOSED

Connor Kuehl: 1
 Don't propagate negative offsets when drawing

 1 files changed, 4 insertions(+), 0 deletions(-)

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

Moritz Poldrack <ich@moritz-poldrack.de>

3 years ago

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.

Daniel Patterson <me@danielpatterson.dev>

3 years ago

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.

Next

Next

lot of these hard to reproduce panics. Although context.Subcontext does

Moritz Poldrack <ich@moritz-poldrack.de>

3 years ago

Panics are there for a reason, so getting rid of panic doesn't really
help here if we don't adress the underlying reason.

Daniel Patterson <me@danielpatterson.dev>

3 years ago

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.

Next

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.

Daniel Patterson <me@danielpatterson.dev>

3 years ago

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.

get called in a lot of locations so perhaps the churn wouldn't be worth
it.

[PATCH] Don't propagate negative offsets when drawing Export this patch

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

Connor Kuehl <cipkuehl@gmail.com>

3 years ago

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.

Moritz Poldrack <ich@moritz-poldrack.de>

3 years ago

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.

Connor

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)