Update canvas.go - Length check added to fix panic out of range

[Agustincou]

Description:

I recently started using PopUp messages in my app, and in a specific scenario, I encountered a panic due to an index out of range error when using a PopUp simultaneously with a Dialog in the same window.

The fix is simple. Despite the questionable usage of PopUps and Dialogs in this particular case, I believe that adding a length check contributes to stability improvement.

Panic message:

panic: runtime error: index out of range [1] with length 1

goroutine 27 [running]:
fyne.io/fyne/v2/internal/driver/common.(*overlayStack).remove(0xc002a8f710, {0x7ff676ac6118, 0xc00bb8a900})
C:/Users/Agustin/go/pkg/mod/fyne.io/fyne/v2@v2.4.3/internal/driver/common/canvas.go:585 +0xd7
fyne.io/fyne/v2/internal/driver/common.(*overlayStack).Remove(0xc002a8f710, {0x7ff676ac6118, 0xc00bb8a900})
C:/Users/Agustin/go/pkg/mod/fyne.io/fyne/v2@v2.4.3/internal/driver/common/canvas.go:574 +0x12e
fyne.io/fyne/v2/widget.(*PopUp).Hide(0xc00bb8a900)
C:/Users/Agustin/go/pkg/mod/fyne.io/fyne/v2@v2.4.3/widget/popup.go:28 +0x56

Checklist:

• Tests included.
• Lint and formatter run with no errors.
• Tests all pass.

[dweymouth]

Hmm, this seems like a band-aid fix where there's actually a deeper issue that needs to be solved. Likely there is some missing locking somewhere or else an assumption is being violated in how we're using the overlay stack.

[Agustincou]

Yes I believe the same. In my case I have almost no experience in the internal workings of Fyne, I have been using it for several months, but I do not have enough knowledge to find the underlying problem.

I recognize that the solution I propose is superficial and attacks the consequence, which may not be ideal.

Maybe someone with more knowledge can give us a hand with this kickoff that I'm trying to give or maybe they are already analyzing it.

[andydotxyz]

By just looking at it there does seem to be some sort of race. I had thought it might be an out of order free (stack assumes top most will go first) but I can't see how that is the case here.

I recently started using PopUp messages in my app, and in a specific scenario, I encountered a panic

Perhaps you can share this scenario so that someone can replicate it and understand what is going on to help out with the PR?

[Agustincou]

Hello Andy, I am going to try to extract a simplified code that allows me to replicate the problem and if I can do so, I will share it here.

In short, my code executes asynchronously in an anonymous function a "ShowPopup()" and then a "defer popUp.Hide()" based on the canvas of a window, which between the execution of "Show" and "Hide" performs a " dialog.Show()". The error seems to occur most of the time, but not always, which is a symptom of a possible race condition.

These days I'm going to be a bit busy, so it may take me a while to replicate it in a simple way.
As soon as I can I will try to isolate and replicate the problem.

Thanks for the answers

[andydotxyz]

If you are hiding/showing two different overlays in different threads there is clearly a race condition in your code. However it should only result in the topmost being undefined and not a crash in Fyne code.

[Agustincou]

Hello,

I have managed to replicate the race condition problem, forcing the execution of asynchronous threads.

In the case of having "Windows" operating system, repeatedly press "Ctrl + F" and eventually the problem happens:

Example of console output:

Shortcut pressed
Shortcut pressed
Shortcut pressed
panic: runtime error: index out of range [1] with length 1

goroutine 38 [running]:
fyne.io/fyne/v2/internal/driver/common.(*overlayStack).remove(0xc0001282d0, {0x7ff6ee638158?, 0xc0000f7500?})
        C:/Users/Agustin/go/pkg/mod/fyne.io/fyne/v2@v2.4.3/internal/driver/common/canvas.go:585 +0x92
fyne.io/fyne/v2/internal/driver/common.(*overlayStack).Remove(0xc0001282d0, {0x7ff6ee638158, 0xc0000f7500})
        C:/Users/Agustin/go/pkg/mod/fyne.io/fyne/v2@v2.4.3/internal/driver/common/canvas.go:574 +0xa5
fyne.io/fyne/v2/widget.(*PopUp).Hide(0xc0000f7500)
        C:/Users/Agustin/go/pkg/mod/fyne.io/fyne/v2@v2.4.3/widget/popup.go:28 +0x42
fyne.io/fyne/v2/dialog.(*dialog).hideWithResponse(0xc00011c540, 0x0?)
        C:/Users/Agustin/go/pkg/mod/fyne.io/fyne/v2@v2.4.3/dialog/base.go:102 +0x25
fyne.io/fyne/v2/dialog.(*dialog).Hide(0x0?)
        C:/Users/Agustin/go/pkg/mod/fyne.io/fyne/v2@v2.4.3/dialog/base.go:49 +0x15
created by main.main.func1 in goroutine 23
        C:/experiments/bug-report/main.go:29 +0x176
exit status 2

The code to replicate:

package main

import (
	"fmt"

	"fyne.io/fyne/v2"
	"fyne.io/fyne/v2/app"
	"fyne.io/fyne/v2/dialog"
	"fyne.io/fyne/v2/driver/desktop"
	"fyne.io/fyne/v2/widget"
)

func main() {
	a := app.New()
	w := a.NewWindow("Bug example")

	popToShow := widget.NewPopUp(widget.NewLabel("PopUp Label"), w.Canvas())
	dialogToShow := dialog.NewCustom("Custom Dialog", "Dismiss", widget.NewLabel("Test Content"), w)

	w.Canvas().AddShortcut(
		&desktop.CustomShortcut{KeyName: fyne.KeyF, Modifier: fyne.KeyModifierShortcutDefault},
		func(shortcut fyne.Shortcut) {
			fmt.Println("Shortcut pressed")

			go popToShow.Show()
			go dialogToShow.Show()

			go popToShow.Hide()
			go dialogToShow.Hide()
		},
	)

	w.Resize(fyne.NewSize(300, 300))
	w.SetContent(widget.NewLabel("Test Content"))
	w.ShowAndRun()
}

[andydotxyz]

		func(shortcut fyne.Shortcut) {
			fmt.Println("Shortcut pressed")

			go popToShow.Show()
			go dialogToShow.Show()

			go popToShow.Hide()
			go dialogToShow.Hide()
		},

In that code you have race conditions, there is no way that you can expect the Hide to always be called after Show if they all launch in different goroutines. If the problem still occurs with the following then we can do something with it:

		func(shortcut fyne.Shortcut) {
			fmt.Println("Shortcut pressed")

			go func() {
				popToShow.Show()
				dialogToShow.Show()

				popToShow.Hide()
				dialogToShow.Hide()
			}()
		},

[andydotxyz]

The "happy path" should be left aligned - i.e. this should really be a check for if the count is too low then returning early.
In general this sort of safety is probably a good thing as we shouldn't really panic even if developer triggers a bad path.