The 'Unsplit' action causes the app to crash

[past-gen]

Description

If you use the 'Quit' action to close the split frame, it doesn't crash.

• Minimal reproducer ( may require multiple attempts ):

  1. VSplit ( or alternative )
  2. Unsplit ( focusing split frame )
  3. Unsplit ( focusing tab )
  4. HSplit ( or alternative )

• Error message:

  Micro encountered an error: runtime.errorString runtime error: invalid memory address or nil pointer dereference
  runtime/panic.go:262 (0x6a2ed8)
  runtime/panic.go:261 (0x6a2ea8)
  github.com/micro-editor/micro/v2/internal/views/splits.go:265 (0xb23f13)
  github.com/micro-editor/micro/v2/internal/views/splits.go:436 (0xb24f0a)
  github.com/micro-editor/micro/v2/internal/action/bufpane.go:653 (0xb4fce5)
  github.com/micro-editor/micro/v2/internal/action/bufpane.go:680 (0xb47df4)
  github.com/micro-editor/micro/v2/internal/action/actions.go:2026 (0xb47cfb)
  github.com/micro-editor/micro/v2/internal/action/bufpane.go:567 (0xb4f55a)
  github.com/micro-editor/micro/v2/internal/action/tab.go:224 (0xb4d931)
  github.com/micro-editor/micro/v2/internal/action/bufpane.go:183 (0xb4d759)
  github.com/micro-editor/micro/v2/internal/action/bufpane.go:33 (0xb4d6f1)
  github.com/micro-editor/micro/v2/internal/action/bufpane.go:546 (0xb4f2f9)
  github.com/micro-editor/micro/v2/internal/action/bufpane.go:464 (0xb4ecbc)
  github.com/micro-editor/micro/v2/internal/action/tab.go:337 (0xb6008a)
  github.com/micro-editor/micro/v2/internal/action/tab.go:141 (0xb5f193)
  github.com/micro-editor/micro/v2/cmd/micro/micro.go:542 (0xb94f6a)
  github.com/micro-editor/micro/v2/cmd/micro/micro.go:481 (0xb94ab0)
  internal/runtime/atomic/types.go:194 (0x68e45d)
  runtime/asm_amd64.s:1700 (0x6ca061)
  

• A little demonstration:

  wezterm-gui_JmPUM8t2Vc.mp4

Environment

• Version: 2.0.16-dev.15 ( 3a7403b )
• OS: Windows 10 22H2
• Terminal: WezTerm/Conhost/Alacritty

[JoeKar]

@Neko-Box-Coder
Can you please have a look too.
The parent of the node is nil in the moment the children are tried to access.
This might be a regression of #3708.

[Neko-Box-Coder]

I had a quick try and can reliably reproduce it now. I will take a more detailed look tomorrow.

The only thing that puzzles me is that if I create the splits quickly and unsplits them quickly, it will crash.
But if I perform the same amount of splits and unsplits slowly, it doesn't crash.

By quickly, I mean just holding down the split and unsplit keybind, and I can reproduce it easily on my machine.

This seems to me like we are having some sort of racing conditions. I thought we are running things single-threaded for the most part, at least for this scenario no?

[JoeKar]

This seems to me like we are having some sort of racing conditions. I thought we are running things single-threaded for the most part, at least for this scenario no?

It is single-threaded in this case and it doesn't really look racy, because I can reproduce it slowly with 100% chance...it is just the sequence, which matters:

1. open micro
2. VSplit
3. Unsplit
4. Unsplit <- error occurs already here
5. HSplit
6. 💥

or

1. open micro
2. HSplit
3. Unsplit
4. Unsplit <- error occurs already here
5. VSplit
6. 💥

After the second Unsplit the opposite of the first split must be triggered and boom.

EDIT:
The following patch seems to prevent it:

diff --git a/internal/views/splits.go b/internal/views/splits.go
index b2a20873..96ce7c71 100644
--- a/internal/views/splits.go
+++ b/internal/views/splits.go
@@ -465,6 +465,9 @@ func (n *Node) Unsplit() bool {
        if !n.IsLeaf() || n.parent == nil {
                return false
        }
+       if n.id == n.parent.id {
+               return false
+       }
        ind := 0
        for i, c := range n.parent.children {
                if c.id == n.id {

The root cause is that we really Unsplit one level too much.

[Neko-Box-Coder]

@JoeKar
Okay, I have just seen your patch after creating my PR lol.

The root cause is that we really Unsplit one level too much.

Yeah, pretty much so if we treat having a single root node after the initial split as an invalid state.

Explained it in detail in #3983

Our fixes are different. Your patch treats having the root node alone after the initial split as an invalid state, whereas my patch treats the initial state as a valid state. Both are fine. I am happy to switch to your patch if we want to.

[JoeKar]

Okay, I have just seen your patch after creating my PR lol.

No worries.

Our fixes are different. Your patch treats having the root node alone after the initial split as an invalid state, whereas my patch treats the initial state as a valid state. Both are fine. I am happy to switch to your patch if we want to.

Since the initial state is valid we shouldn't treat it as invalid after the first (un)splits.
So I'd go with your suggestion.

Explained it in detail in #3983

I'll continue to discuss the details in your PR. :)